The init script for tt-rss (with USE=daemon) calls "chown -R" on a few directories. For simplicity, I have one instance in /var/www/localhost/htdocs/tt-rss, and the init script does (among other things), chown -R ":${TTRSSD_GROUP}" /var/www/localhost/htdocs/tt-rss/cache chmod -R g+w /var/www/localhost/htdocs/tt-rss/cache Afterwards, the cache directory is writeable by both the web server user ("apache", for me) and the "ttrssd" group, of which the "ttrssd" user is a member. Any of those users or groups can place a hard link in the cache directory: sudo su ttrssd -c \ 'ln /var/lib/portage/world /var/www/localhost/htdocs/tt-rss/cache/world' The next time the init script is run, it will call "chown -R" and "chmod -R" as root on the target of that hard link: $ sudo /etc/init.d/ttrssd start * Starting TT-RSS update daemon in "/var/www/localhost/htdocs/tt-rss" .. $ ls /var/lib/portage/world -rw-rw-r-- 2 root ttrssd 3.7K 2016-12-22 16:46 /var/lib/portage/world and thus the ttrssd user has gained root. I will do a full write-up of the issue soon, but I would sum up the solution as, * Never use chmod or chown with the "-R" flag (you don't know what you're calling it on). Calling them at all as root is a red flag. * Use checkpath to create individual files or directories with specified permissions. In your case, I think you want $TTRSSD_GROUP to be switchable on-the-fly, and unfortunately I don't see an easy way to do that. The web server needs to be able to write new files to the cache directory, and if you switch the group it's running as, then you'll need to change the permissions on that directory, too. Probably the only safe way to do that is to call chown, but *running as the old TTRSSD_GROUP* so that it can "give away" ownership but not steal it from anyone else. I don't think that's easily automated, though.
Thanks for you report. I'm not sure how to proceed. (In reply to Michael Orlitzky from comment #0) > The init script for tt-rss (with USE=daemon) calls "chown -R" on a few > directories. > > [...] > > and thus the ttrssd user has gained root. > > I will do a full write-up of the issue soon, but I would sum up the solution > as, > > * Never use chmod or chown with the "-R" flag (you don't know what you're > calling it on). Calling them at all as root is a red flag. > > * Use checkpath to create individual files or directories with > specified permissions. > > In your case, I think you want $TTRSSD_GROUP to be switchable on-the-fly, > and unfortunately I don't see an easy way to do that. I'm not sure we do need that. I use the default group and I guess many users do too. > The web server needs > to be able to write new files to the cache directory, and if you switch the > group it's running as, then you'll need to change the permissions on that > directory, too. Probably the only safe way to do that is to call chown, but > *running as the old TTRSSD_GROUP* so that it can "give away" ownership but > not steal it from anyone else. I don't think that's easily automated, though. It appears as if other webapps suffer from this too. As I don't have a solution, I will wait for a distro-wide or even wider discussion to happen.
(In reply to Thomas Kahle from comment #1) > > > > In your case, I think you want $TTRSSD_GROUP to be switchable on-the-fly, > > and unfortunately I don't see an easy way to do that. > > I'm not sure we do need that. I use the default group and I guess many > users do too. If that's the case, then the comment in the init script suggests one solution: # NOTE: This can't be done by webapp-config if we want runtime # configurable TTRSSD_GROUP for dir in ${INSTANCE_FOLDERS}; do <bad stuff> If the group is never going to change, then webapp-config should be able to set everything up with the correct permissions before you use it. Unfortunately I don't use the webapp stuff myself, so I can't be too helpful in that regard. So long as you do so in a non-writable directory owned by root:root, calling chown or chmod once (via webapp-config) should be safe.
CCing the new maintainer... Chewi: the tl;dr for this is that having the TTRSSD_GROUP be configurable opens a big security hole. When TTRSSD_GROUP changes, you need to call "chown" as root to fix things, but that creates a root exploit. Thomas wasn't even sure that the TTRSSD_GROUP needed to be a variable, since everyone probably uses the default. If that's the case, getting rid of the variable and all the permission-fixing code in the init script should fix this.
Okay, I'll deal with it ASAP. I haven't done anything with this package yet as I was focusing on phpBB first but it shouldn't be a problem.
The init script should not call `chown -R`. Instead, the permissions of relevant directories should be set to a fixed user:group by the ebuild, and init scripts should then rely on that fixed user:group.
(In reply to Jason A. Donenfeld from comment #5) > The init script should not call `chown -R`. Instead, the permissions of > relevant directories should be set to a fixed user:group by the ebuild, and > init scripts should then rely on that fixed user:group. It's not possible to handle this in the ebuild as INSTANCE_DIRS is dynamic and defined in /etc/conf.d/ttrss. It may even have multiple directories. I'll see about handling this in webapp-config.
Users are requesting a version bump so I'm taking a fresh look at this. I've realised that the current approach is kind of broken anyway, particularly for the cache directory. Each of the three directories has slightly different characteristics. lock: This is the simplest one. I believe these files are exclusively written by the updater and not the web server so they don't need to be group writable. Only the directory needs to be group writable as it is owned by the web server. feed-icons: These files can be written by the updater or the web server but they are never modified after creation. They may be replaced but they are unlinked first. As such, these files also don't need to be group writable, only the directory does. cache: This is the complicated one. All sorts of things get written here by the updater and the web server and they are frequently modified after creation. These files therefore do need to be group writable and calling chmod on starting the daemon doesn't totally avoid permission issues here. Short of patching the code all over the place, the only way to guarantee smooth operation is to use ACLs to ensure all new files are created group writable. I have good experience with approach. I think this should be applied by a webapp-config hook script so that the permissions are in place right from the start, before the daemon is launched. If ACL support is unavailable then it will output a warning. By handling this in webapp-config, TTRSSD_GROUP will no longer be configurable but it has already been stated above that this is acceptable. With this approach, it should no longer be necessary to call chown/chgrp/chmod recursively. I may use find to apply the correct permissions under cache but only on real directories, not symlinks. This should be safe as hard linked directories seem to be outlawed these days and they could only be created by root before. How does this sound?
(In reply to James Le Cuirot from comment #7) > How does this sound? As mentioned in original report, one alternative might also be to use checkpath c.f hardening in bug 540006 which wouldn't rely on dependency for ACLs.
(In reply to Kristian Fiskerstrand from comment #8) > (In reply to James Le Cuirot from comment #7) > > > How does this sound? > > As mentioned in original report, one alternative might also be to use > checkpath c.f hardening in bug 540006 which wouldn't rely on dependency for > ACLs. checkpath only checks when the daemon is started. A file may get written by the web server and the updater may attempt to modify it at any point after that. I believe ACLs are the only way to ensure that group writable permissions are applied to the files immediately on creation. I thought about changing the umask but while this might work for the updater, it is not really appropriate for the web server, especially when using Apache mod_php.
I raised this issue on gentoo-dev without mentioning the package but I felt I couldn't reply further without revealing it. I'm CC'ing mjo and replying below. On Wed, 10 Jan 2018 18:31:21 -0500 Michael Orlitzky <mjo@gentoo.org> wrote: > On 01/10/2018 05:18 PM, James Le Cuirot wrote: > > > > The init script used to call chown/chmod -R, which is > > obviously bad. I've compromised by only calling these on the > > directories themselves (ignoring symlinks). I believe this is safe > > because it's not possible to create hard linked directories these days? > > Would you agree? > > Are you still using chown and chmod? If so, you should switch to > checkpath -- chown and chmod don't even try to avoid hard links. I would > be surprised to see a "chown" or "chmod" in an init script that can't be > replaced by something better. > > The race condition that we're talking about here is trying to squeeze > the last 1% of security out of checkpath; it's already much safer than > chown/chmod. > > For example, if your script is calling chown and chmod on two > directories /foo and /foo/bar, then whoever owns /foo can kill /foo/bar > entirely and replace it with a hard link to /etc/passwd. When the > service restarts, chown and chmod won't care that you think /foo/bar > should be a directory instead. What I wasn't able to say before was that this is a webapp with a daemon and even without this vulnerability, the package was previously handling the permissions badly. Doing this in the init script is too late as ACLs are required when installing the webapp to keep the permissions straight. I'm using a hook script when the webapp is installed and checkpath is not available here. It also doesn't handle ACLs. I understand the point you made above. I initially thought I was safe because I'm using "find -type d" but I guess this is still subject to the race condition. What if I append / to all the arguments? I think that is safe? ---- #!/bin/bash -e cd "${MY_INSTALLDIR}" if [[ $1 = install ]]; then while IFS= read -r -d '' line; do CACHE_DIRS+=( "${line}/" ) done < <(find cache/ -maxdepth 1 -type d -print0) chgrp ttrssd feed-icons/ lock/ "${CACHE_DIRS[@]}" chmod g+ws feed-icons/ lock/ "${CACHE_DIRS[@]}" if ! setfacl -m d:g::rwX "${CACHE_DIRS[@]}"; then echo "WARNING: ACLs are not available on this filesystem. Either enable them or set TTRSSD_USER to your PHP user in /etc/conf.d/ttrssd to avoid permission issues." fi fi
Do I understand that this script will be run only once, at install time? Is it also true that everything in MY_INSTALLDIR is root:root to begin with? If so, I would run these two lines first, chmod g+ws feed-icons/ lock/ "${CACHE_DIRS[@]}" setfacl -m d:g::rwX "${CACHE_DIRS[@]}" because at this point, those lines are only granting permissions to the "root" group, and that's nothing to worry about. Then, run chgrp (actually, chown...) last, to finally make things writable by the ttrssd group: # CACHE_DIRS only goes one level deep, so get all of the children # before "cache" itself to rule out the possibility that a directory # inside "cache" can be replaced. chown --no-dereference --from=root:root :ttrssd feed-icons/ lock/ cache/* chown --no-dereference --from=root:root :ttrssd cache I've used chown instead of chgrp above, even though we're only changing the group, because GNU chown has the nice "--from" flag that provides some extra peace of mind. If the cache/* glob gives you trouble, you might change CACHE_DIRS to CACHE_SUBDIRS and use "-mindepth 1" in the "find" command. Then instead of cache/* you could use CACHE_SUBDIRS, at the expense of having to mention "cache/" explicitly in the other two places.
(In reply to Michael Orlitzky from comment #11) > Do I understand that this script will be run only once, at install time? > > Is it also true that everything in MY_INSTALLDIR is root:root to begin with? Unfortunately not. The script is also run on upgrades. More importantly, with webapps, all directories are given the web server group (e.g. nginx). Directories marked as server-owned are given the web server user and also made group writable. To support the non-daemon case, we mark the directories in that script as server-owned, so they initially look like this. drwxr-xr-x 21 root root 4096 Jan 14 09:58 . drwxrwxr-x 7 nginx nginx 4096 Jan 14 09:58 cache drwxrwxr-x 2 nginx nginx 4096 Jan 14 09:58 feed-icons drwxrwxr-x 2 nginx nginx 4096 Jan 14 09:58 lock I guess this makes your suggestions irrelevant but thanks anyway, this is useful advice that I could apply elsewhere if I encounter this again. On a side note, I do wonder whether webapp-config itself is applying permissions safely but I do not maintain this and I'm not about to start. ;)
(In reply to James Le Cuirot from comment #12) > > I guess this makes your suggestions irrelevant but thanks anyway Yes it does =) Of the three operations, chmod, chgrp, and setfacl, only the "chgrp" requires root permissions. Probably the best you can do is to change groups as root, chown --no-dereference --from=nginx:nginx :ttrssd feed-icons/ lock/ "${CACHE_DIRS[@]}" There's still a tiny race condition built into "chown --from", but I don't see a safer way to do it. At that point, nginx still owns everything, and you only want to chmod/sefacl the things that the owns. You can do that with "su nginx -s /bin/sh -c ...", to perform the chmod/setfacl *as* the nginx user, whence the kernel will prevent him from following any links to sensitive stuff. > On a side note, I do wonder whether webapp-config itself is applying > permissions safely but I do not maintain this and I'm not about to start. ;) Shhhhhhhhh I have enough to do already =P
(In reply to Michael Orlitzky from comment #13) > Of the three operations, chmod, chgrp, and setfacl, only the "chgrp" > requires root permissions. Probably the best you can do is to change groups > as root, > > chown --no-dereference --from=nginx:nginx :ttrssd feed-icons/ lock/ > "${CACHE_DIRS[@]}" I could do this since nginx (or apache or whatever) is available as VHOST_SERVER_UID/VHOST_SERVER_GID but I want this script to straighten out existing installations that currently have bad permissions. The ttrssd group used to be configurable so the group could potentially be anything. While this would ideally be tighter, I think the bottom line is that I'm only applying changes to real directories (no symlinks) rather than files (hence no hardlinks) and that should be inconsequential.
(In reply to James Le Cuirot from comment #14) > I want this script to straighten out > existing installations that currently have bad permissions. The ttrssd group > used to be configurable so the group could potentially be anything. This is basically impossible AFAIK, you're in a bad spot =( > I think the bottom line is that I'm only > applying changes to real directories (no symlinks) rather than files (hence > no hardlinks) and that should be inconsequential. They're directories when you find them, but not necessarily when you call chown on them; that situation is the raison d'être for the "--from" flag: https://www.gnu.org/software/coreutils/manual/html_node/chown-invocation.html You might think that the window for exploitation will be small, but since the "cache" directory is writable by the person you're trying to defend against, it may not be. For example, the nginx user can create a million empty directories in "cache". The next time this is run, he will have a few minutes (while "find" is running) to replace the first directory with a hard link to /etc/passwd. But keeping things in perspective, this is already an improvement over the existing init script. Maybe you can add the "--from" flag at a later point, once you're not worried about fixing old installations. Or, another thing that might work would be to reset everything to root:root first, and to wipe the ACLs. The existing owner could still trick you with a link during that phase, but he wouldn't be able to do anything dangerous, since the target of the link would become root:root (and that doesn't help him). Then afterwards, it's safe to do things as in Comment 11.
(In reply to Michael Orlitzky from comment #15) > They're directories when you find them, but not necessarily when you call > chown on them; that situation is the raison d'être for the "--from" flag: I honestly think the trailing / has us covered here. The think the --from option would only be strictly needed if we really wanted to safely operate on files. I checked this out with strace. $ touch file $ mkdir dir $ strace -e trace=file chmod g+ws file/ execve("/usr/bin/chmod", ["chmod", "g+ws", "file/"], 0x7ffd05d07620 /* 87 vars */) = 0 access("/etc/ld.so.preload", R_OK) = 0 open("/etc/ld.so.preload", O_RDONLY|O_CLOEXEC) = 3 open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3 open("/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3 open("/usr/lib64/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3 stat("file/", 0x55d5edf1b348) = -1 ENOTDIR (Not a directory) stat("file/", 0x55d5edf1b348) = -1 ENOTDIR (Not a directory) ... chmod: cannot access 'file/': Not a directory +++ exited with 1 +++ $ strace -e trace=file chmod g+ws dir/ execve("/usr/bin/chmod", ["chmod", "g+ws", "dir/"], 0x7fff72824ae0 /* 87 vars */) = 0 access("/etc/ld.so.preload", R_OK) = 0 open("/etc/ld.so.preload", O_RDONLY|O_CLOEXEC) = 3 open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3 open("/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3 open("/usr/lib64/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3 stat("dir/", {st_mode=S_IFDIR|0755, st_size=6, ...}) = 0 fchmodat(AT_FDCWD, "dir/", 02775) = 0 +++ exited with 0 +++
(In reply to James Le Cuirot from comment #16) > (In reply to Michael Orlitzky from comment #15) > > They're directories when you find them, but not necessarily when you call > > chown on them; that situation is the raison d'être for the "--from" flag: > > I honestly think the trailing / has us covered here. I just now saw that in the loop, sorry. You might be right, I can't figure out a way to exploit it with a hard link. But, what if there are files under cache/ with an old TTRSSD_GROUP? You won't fix them this way. Also be careful that chgrp and chmod follow symlinks by default when not operating recursively. The call to chgrp can be modified with "--no-dereference", but there's no such option to chmod and setfacl. So the version in Comment 10 needs to worry about cache/dir being replaced by a symlink, too. The same trick should work: create a million directories in cache/, and then replace the first one with a symlink to the directory of your choosing. The danger here is lessened because the target will get g+ws and a default ACL, but won't have its group changed if you add --no-dereference to the chgrp.
(In reply to Michael Orlitzky from comment #17) > But, what if there are files under cache/ with an old TTRSSD_GROUP? You > won't fix them this way. I had thought of that. Given that the permissions have already been somewhat broken, I don't think this causes a fatal error but I was considering adding a note about it. I don't think it would be safe to try and delete them in the script. Under a setup without ACLs, it might delete the files every time. > Also be careful that chgrp and chmod follow symlinks by default when not > operating recursively. The call to chgrp can be modified with > "--no-dereference", but there's no such option to chmod and setfacl. So the > version in Comment 10 needs to worry about cache/dir being replaced by a > symlink, too. The same trick should work: create a million directories in > cache/, and then replace the first one with a symlink to the directory of > your choosing. The danger here is lessened because the target will get g+ws > and a default ACL, but won't have its group changed if you add > --no-dereference to the chgrp. Thanks, I had missed that particular race. How frustrating. I'll have a think but this is probably the best we can do.
(In reply to James Le Cuirot from comment #18) > Thanks, I had missed that particular race. How frustrating. I'll have a > think but this is probably the best we can do. It feels a little ugly but setting "chattr +i" on cache prevents the immediate contents from being changed while still allowing you to call chgrp, chmod, and setfacl on the child directories. I could set that before calling find, avoiding the race. I would add a trap for "chattr -i" in case the script bails out early. This is a bit like setting cache to root:root beforehand but arguably less disruptive.
It occurs to me that I've over-complicated this because the cache directory itself does not need to be writable, only the child directories do. I've just grepped the PHP sources to make sure. I can therefore make that root:nginx 755 like the other directories and assume that the contents have not been tampered with, negating the need for find. Sorry if I've wasted your time here but I certainly still think it's been an interesting discussion!
(In reply to James Le Cuirot from comment #20) > Sorry if I've wasted your time here but I certainly still think it's been an > interesting discussion! Not at all, this is my idea of fun. And I learned two new tricks, * chattr +i (depends on the FS/kernel, but very clever) * using the trailing slash to avoid operating on files I'm glad it all worked out.
Security team, this has now been bumped and the old version has been dropped as there was no stabilisation to do. Sorry that I forgot about the notice above, this is different to other security bugs I have dealt with. I did ask for feedback though and also discussed it thoroughly here with mjo, who fully understand the issue.
Unrestricting and reassigning to security@ per bug #705894
unrestricting per bug 705894
(In reply to James Le Cuirot from comment #22) > Security team, this has now been bumped and the old version has been dropped > as there was no stabilisation to do. Sorry that I forgot about the notice > above, this is different to other security bugs I have dealt with. I did ask > for feedback though and also discussed it thoroughly here with mjo, who > fully understand the issue. Thank you. No stable ebuilds => no GLSA. Closing.
