coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: cp, ln, mv, install: check for vulnerable target directories


From: Paul Eggert
Subject: Re: cp, ln, mv, install: check for vulnerable target directories
Date: Wed, 20 Sep 2017 00:06:14 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

Pádraig Brady wrote:

It would be better to have some discussion about such patches before landing.

Feel free to revert the whole thing if you'd prefer; I won't mind. (I would like the security hole to be smaller, though....(

I see it discounts sticky dirs, which is a bit surprising since they're
generally the most vulnerable since they're configured to be shared?

It's the other way round. If you do 'cp foo /tmp/d' and /tmp is world-writable and sticky and you own d (without following d if it is a symlink), then no other user can attack you by turning d into a symlink to a victim directory. So bleeding-edge cp doesn't fail in that case. It would fail if /tmp were world-writable and non-sticky, though, as that can be hijacked. In other words, sticky directories are safer than non-sticky ones.

Note the sticky dir case is already handled at the Linux kernel level
with /proc/sys/fs/protected_symlinks, as that's the main attack vector.

I wasn't aware of that feature. Yes, it closes the most-obvious big hole of symlink attacks via /tmp. However, it doesn't close the hole of a symlink in a non-sticky world-writable directory, which is odd as that is a wider hole than a sticky world-writable directory like /tmp. And I'm puzzled why it follows a symlink merely because the symlink and its parent directory have the same owner, as this lets a directory's owner to hijack attempts by other users to copy into the directory. In short, the protected_symlinks feature doesn't seem to be designed to thwart symlink attacks in general, just ones directly via /tmp.

Also, /proc/sys/fs/protected_symlinks is just a Linux feature, right? It's nicer to have something that also works in other POSIXish systems.

I feel this is protecting a less problematic case and will probably
break embedded system scripts for example which generally don't
care about permissions but run stuff as non root.

I'm not sure what running as non root has to do with it. The coreutils heuristic pretty much does the same thing for root as for non-root users.

I take your point about embedded systems. Though don't they generally have directories that are mode 755? If so, it shouldn't be much of a problem with this patch. If not, perhaps we should look at the permissions of /, and bypass all this checking if / is world-writable.

this doesn't protect
the case where root is writing to a root owned but a+w dir,

That's not the problem. The problem is when the destination's parent is a+w, not when the destination itself is an a+w dir. And the newly-installed patch defends against this:

  # ls -ld /tmp/d /tmp/d/passwd
  drwxrwxrwx 2 root     root     4096 Sep 19 23:31 /tmp/d
  lrwxrwxrwx 2 whoopsie whoopsie    7 Sep 19 23:31 /tmp/d/passwd -> /etc
  # src/cp passwd /tmp/d/passwd
  src/cp: vulnerable target directory '/tmp/d/passwd' (append '/' to use anyway)

whereas coreutils 8.28 cp charges ahead and overwrites /etc/passwd here.
 > it seems strange to match the dir owner with the writer.

It's because we're worried about the destination's parent, not the destination. And the troublesome case occurs when a non-trusted user can write to the destination's parent. We trust ourselves and root, so if no user other than ourselves or root can write to the parent, then it's safe. I.e., it's not safe if the parent is owned by somebody other than ourself or root, because that somebody-else could hijack us.

In the kernel protected_symlinks case the owner match gating is
on the symlink itself which makes sense.

Coreutils uses lstat on the destination, which means it's gating on the symlink itself too.

I'd be more inclined
to have another kernel value for /proc/sys/fs/protected_symlinks
that also provided the protection to non sticky dirs?

That would help. It'd also help if a kernel value could close the symlink-has-same-owner-as-parent-dir hole. (I'm curious as to why protected_symlinks seems to be missing the point here....)

A less invasive change might be to enable this only with -i
which Fedora at least enables by default for the root user.

Only the root user? I'm also worried about non-root users being hijacked.

Also there were many syntax-check failures in this patch.

Yes, I often forget to check those. I will try to do better....



reply via email to

[Prev in Thread] Current Thread [Next in Thread]