On Fri, Aug 13, 2021 at 10:18 AM Eric W. Biederman
[off-list ref] wrote:
Florian Weimer, would it be possible to get glibc's ld.so implementation to use
MAP_SHARED? Just so people reading the code know what to expect of the
kernel? As far as I can tell there is not a practical difference
between a read-only MAP_PRIVATE and a read-only MAP_SHARED.
There's a huge difference.
For one, you actually don't necessarily want read-only. Doing COW on
library images is quite common for things like relocation etc (you'd
_hope_ everything is PC-relative, but no)
So no. Never EVER use MAP_SHARED unless you literally expect to have
two different mappings that need to be kept in sync and one writes the
other.
I'll just repeat: stop arguing about this case. If somebody writes to
a busy library, THAT IS A FUNDAMENTAL BUG, and nobody sane should care
at all about it apart from the "you get what you deserve".
What's next? Do you think glibc should also map every byte in the user
address space so that user programs don't get SIGSEGV when they have
wild pointers?
Again - that's a user BUG and trying to "work around" a wild pointer
is a worse fix than the problem it tries to fix.
The exact same thing is true for shared library (or executable)
mappings. Trying to work around people writing to them is *worse* than
the bug of doing so.
Stop this completely inane discussion already.
Linus
On Fri, Aug 13, 2021, at 5:31 PM, Linus Torvalds wrote:
On Fri, Aug 13, 2021 at 10:18 AM Eric W. Biederman
[off-list ref] wrote:
quoted
Florian Weimer, would it be possible to get glibc's ld.so implementation to use
MAP_SHARED? Just so people reading the code know what to expect of the
kernel? As far as I can tell there is not a practical difference
between a read-only MAP_PRIVATE and a read-only MAP_SHARED.
There's a huge difference.
For one, you actually don't necessarily want read-only. Doing COW on
library images is quite common for things like relocation etc (you'd
_hope_ everything is PC-relative, but no)
So no. Never EVER use MAP_SHARED unless you literally expect to have
two different mappings that need to be kept in sync and one writes the
other.
I'll just repeat: stop arguing about this case. If somebody writes to
a busy library, THAT IS A FUNDAMENTAL BUG, and nobody sane should care
at all about it apart from the "you get what you deserve".
What's next? Do you think glibc should also map every byte in the user
address space so that user programs don't get SIGSEGV when they have
wild pointers?
Again - that's a user BUG and trying to "work around" a wild pointer
is a worse fix than the problem it tries to fix.
The exact same thing is true for shared library (or executable)
mappings. Trying to work around people writing to them is *worse* than
the bug of doing so.
Stop this completely inane discussion already.
I’ll bite. How about we attack this in the opposite direction: remove the deny write mechanism entirely.
In my life, I’ve encountered -ETXTBUSY intermittently, and it invariably means that I somehow failed to finish killing a program fast enough for whatever random rebuild I’m doing to succeed. It’s at best erratic — it only applies for static binaries, and it has never once saved me from a problem I care about. If the program I’m recompiling crashes, I don’t care — it’s probably already part way through dying from an unrelated fatal signal. What actually happens is that I see -ETXTBUSY, think “wait, this isn’t Windows, why are there file sharing rules,” then think “wait, Linux has *one* half baked file sharing rule,” and go on with my life. [0]
Seriously, can we deprecate and remove the whole thing?
[0] we have mandatory locks, too. Sigh.
On Fri, Aug 13, 2021 at 2:54 PM Linus Torvalds
[off-list ref] wrote:
And nobody really complained when we weakened it, so maybe removing it
entirely might be acceptable.
I guess we could just try it and see... Worst comes to worst, we'll
have to put it back, but at least we'd know what crazy thing still
wants it..
Linus
On Fri, Aug 13, 2021 at 2:49 PM Andy Lutomirski [off-list ref] wrote:
I’ll bite. How about we attack this in the opposite direction: remove the deny write mechanism entirely.
I think that would be ok, except I can see somebody relying on it.
It's broken, it's stupid, but we've done that ETXTBUSY for a _loong_ time.
But you are right that we have removed parts of it over time (no more
MAP_DENYWRITE, no more uselib()) so that what we have today is a
fairly weak form of what we used to do.
And nobody really complained when we weakened it, so maybe removing it
entirely might be acceptable.
Linus
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-08-14 02:00:29
On Fri, Aug 13, 2021 at 02:58:57PM -1000, Linus Torvalds wrote:
On Fri, Aug 13, 2021 at 2:54 PM Linus Torvalds
[off-list ref] wrote:
quoted
And nobody really complained when we weakened it, so maybe removing it
entirely might be acceptable.
I guess we could just try it and see... Worst comes to worst, we'll
have to put it back, but at least we'd know what crazy thing still
wants it..
Umm... I'll need to go back and look through the thread, but I'm
fairly sure that there used to be suckers that did replacement of
binary that way (try to write, count on exclusion with execve while
it's being written to) instead of using rename. Install scripts
of weird crap and stuff like that...
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-08-14 02:05:32
On Sat, Aug 14, 2021 at 01:57:31AM +0000, Al Viro wrote:
On Fri, Aug 13, 2021 at 02:58:57PM -1000, Linus Torvalds wrote:
quoted
On Fri, Aug 13, 2021 at 2:54 PM Linus Torvalds
[off-list ref] wrote:
quoted
And nobody really complained when we weakened it, so maybe removing it
entirely might be acceptable.
I guess we could just try it and see... Worst comes to worst, we'll
have to put it back, but at least we'd know what crazy thing still
wants it..
Umm... I'll need to go back and look through the thread, but I'm
fairly sure that there used to be suckers that did replacement of
binary that way (try to write, count on exclusion with execve while
it's being written to) instead of using rename. Install scripts
of weird crap and stuff like that...
... and before anyone goes off - I certainly agree that using that
behaviour is not a good idea and had never been one. All I'm saying
is that there at least used to be very random (and rarely exercised)
bits of userland relying upon that behaviour.
From: Christian Brauner <hidden> Date: 2021-08-14 07:53:54
On Sat, Aug 14, 2021 at 01:57:31AM +0000, Al Viro wrote:
On Fri, Aug 13, 2021 at 02:58:57PM -1000, Linus Torvalds wrote:
quoted
On Fri, Aug 13, 2021 at 2:54 PM Linus Torvalds
[off-list ref] wrote:
quoted
And nobody really complained when we weakened it, so maybe removing it
entirely might be acceptable.
I guess we could just try it and see... Worst comes to worst, we'll
have to put it back, but at least we'd know what crazy thing still
wants it..
Umm... I'll need to go back and look through the thread, but I'm
fairly sure that there used to be suckers that did replacement of
binary that way (try to write, count on exclusion with execve while
it's being written to) instead of using rename. Install scripts
of weird crap and stuff like that...
I'm not agains trying to remove it, but I think Al has a point.
Removing the write protection will also most certainly make certain
classes of attacks _easier_. For example, the runC container breakout
from last year using privileged containers issued CVE-2019-5736 would be
easier. I'm quoting from the commit I fixed this with:
The attack can be made when attaching to a running container or when starting a
container running a specially crafted image. For example, when runC attaches
to a container the attacker can trick it into executing itself. This could be
done by replacing the target binary inside the container with a custom binary
pointing back at the runC binary itself. As an example, if the target binary
was /bin/bash, this could be replaced with an executable script specifying the
interpreter path #!/proc/self/exe (/proc/self/exec is a symbolic link created
by the kernel for every process which points to the binary that was executed
for that process). As such when /bin/bash is executed inside the container,
instead the target of /proc/self/exe will be executed - which will point to the
runc binary on the host. The attacker can then proceed to write to the target
of /proc/self/exe to try and overwrite the runC binary on the host.
and then the write protection kicks in of course:
However in general, this will not succeed as the kernel will not
permit it to be overwritten whilst runC is executing.
which the attack can of course already overcome nowadays with minimal
smarts:
To overcome this, the attacker can instead open a file descriptor to
/proc/self/exe using the O_PATH flag and then proceed to reopen the
binary as O_WRONLY through /proc/self/fd/<nr> and try to write to it
in a busy loop from a separate process. Ultimately it will succeed
when the runC binary exits. After this the runC binary is
compromised and can be used to attack other containers or the host
itself.
But with write protection removed you'd allow such attacks to succeed
right away. It's not a huge deal to remove it since we need to have
other protection mechanisms in place already:
To prevent this attack, LXC has been patched to create a temporary copy of the
calling binary itself when it starts or attaches to containers. To do this LXC
creates an anonymous, in-memory file using the memfd_create() system call and
copies itself into the temporary in-memory file, which is then sealed to
prevent further modifications. LXC then executes this sealed, in-memory file
instead of the original on-disk binary. Any compromising write operations from
a privileged container to the host LXC binary will then write to the temporary
in-memory binary and not to the host binary on-disk, preserving the integrity
of the host LXC binary. Also as the temporary, in-memory LXC binary is sealed,
writes to this will also fail.
Note: memfd_create() was added to the Linux kernel in the 3.17 release.
However, I still like to pich the upgrade mask idea Aleksa and we tried
to implement when we did openat2(). If we leave write-protection in
preventing /proc/self/exe from being written to:
we can take some time and upstream the upgrade mask patchset which was
part of the initial openat2() patchset but was dropped back then (and I
had Linus remove the last remants of the idea in [1]).
The idea was to add a new field to struct open_how "upgrade_mask" that
would allow a caller to specify with what permissions an fd could be
reopened with. I still like this idea a great deal and it would be a
very welcome addition to system management programs. The upgrade mask is
of course optional, i.e. the caller would have to specify the upgrade
mask at open time to restrict reopening (lest we regress the whole
world).
But, we could make it so that an O_PATH fd gotten from opening
/proc/<pid>/exe always gets a restricted upgrade mask set and so it
can't be upgraded to a O_WRONLY fd afterwards. For this to be
meaningful, write protection for /proc/self/exe would need to be kept.
[1]: commit 5c350aa11b441b32baf3bfe4018168cb8d10cef7
Author: Christian Brauner [off-list ref]
Date: Fri May 28 11:24:15 2021 +0200
fcntl: remove unused VALID_UPGRADE_FLAGS
We currently do not maky use of this feature and should we implement
something like this in the future it's trivial to add it back.
Link: https://lore.kernel.org/r/20210528092417.3942079-2-brauner@kernel.org
Cc: Christoph Hellwig [off-list ref]
Cc: Aleksa Sarai [off-list ref]
Cc: Al Viro [off-list ref]
Cc: linux-fsdevel@vger.kernel.org
Suggested-by: Richard Guy Briggs [off-list ref]
Reviewed-by: Richard Guy Briggs [off-list ref]
Reviewed-by: Christoph Hellwig [off-list ref]
Signed-off-by: Christian Brauner [off-list ref]
From: David Hildenbrand <hidden> Date: 2021-08-14 09:06:57
On 14.08.21 04:02, Al Viro wrote:
On Sat, Aug 14, 2021 at 01:57:31AM +0000, Al Viro wrote:
quoted
On Fri, Aug 13, 2021 at 02:58:57PM -1000, Linus Torvalds wrote:
quoted
On Fri, Aug 13, 2021 at 2:54 PM Linus Torvalds
[off-list ref] wrote:
quoted
And nobody really complained when we weakened it, so maybe removing it
entirely might be acceptable.
I guess we could just try it and see... Worst comes to worst, we'll
have to put it back, but at least we'd know what crazy thing still
wants it..
Umm... I'll need to go back and look through the thread, but I'm
fairly sure that there used to be suckers that did replacement of
binary that way (try to write, count on exclusion with execve while
it's being written to) instead of using rename. Install scripts
of weird crap and stuff like that...
... and before anyone goes off - I certainly agree that using that
behaviour is not a good idea and had never been one. All I'm saying
is that there at least used to be very random (and rarely exercised)
bits of userland relying upon that behaviour.
Removing it completely is certainly more controversial than limiting it to the main executable. I'm mostly happy as long as we get rid of that nasty per-VMA handling, because that adds real complexity at places that are complicated enough.
Having the remaining deny_write_access()/allow_write_access() at sane places now (loading a new binary, exchanging exe_file) looks certainly much cleaner and I still consider it a valuable, simple sanity feature to have around. I don't think there is any sane use case for modifying the main executable, and it seems to be very easy to catch.
For example, besides users that rely on this behavior, in my thinking (see the cover letter), especially having a binary not getting changed while we're loading it sounds like a very good idea (not saying we would expose a way to exploit the kernel if we would allow for modifications while in the elf parser, but also not saying we wouldn't because I didn't check if there would be a way; at least we already allow it in the legacy library loader before mapping the segments with MAP_DENYWRITE). And if we decide to keep the behavior while loading the executable, keeping it while exe_file is set isn't much added code/complexity IMHO.
Long story short, I'd vote for keeping it in, and if we decide to rip it out completely, do it a a separate, more careful step.
--
Thanks,
David / dhildenb
From: David Laight <hidden> Date: 2021-08-14 19:52:14
From: Linus Torvalds
Sent: 14 August 2021 01:55
On Fri, Aug 13, 2021 at 2:49 PM Andy Lutomirski [off-list ref] wrote:
quoted
I’ll bite. How about we attack this in the opposite direction: remove the deny write mechanism
entirely.
I think that would be ok, except I can see somebody relying on it.
It's broken, it's stupid, but we've done that ETXTBUSY for a _loong_ time.
I think ETXTBUSY predates Linux itself.
But I can't remember whether the elf versions of sunos or svr4
implemented it for shared libraries.
I don't remember hitting it, so they may not have.
I'm actually surprised it ia an mmap() flag rather than an open() one.
Being able to open a file and guarantee it can't be changed seems a sane idea.
And not just for programs/libraries.
By the sound of it 'immutable' is no use.
You need to be able to unlink the file - otherwise you get into the
window's fiasco of not being able to update without 17 reboots.
FWIW MAP_COPY would only need to take one copy of the page - all the
users could share the same page (backed by a single page of swap).
Not that I'm suggesting it is a good idea at all.
I do wonder about /proc/self/exe though.
It gave the NetBSD Linux emulation a terrible problem.
Being able to open the inode of the program is fine.
The problem is the what readlink() returns - it is basically stale.
If a program open the link contents it could get anything at all.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: J. Bruce Fields <hidden> Date: 2021-08-18 15:42:22
On Fri, Aug 13, 2021 at 05:49:19PM -0700, Andy Lutomirski wrote:
I’ll bite. How about we attack this in the opposite direction: remove
the deny write mechanism entirely.
For what it's worth, Windows has open flags that allow denying read or
write opens. They also made their way into the NFSv4 protocol, but
knfsd enforces them only against other NFSv4 clients. Last I checked,
Samba attempted to emulate them using flock (and there's a comment to
that effect on the flock syscall in fs/locks.c). I don't know what Wine
does.
Pavel Shilovsky posted flags adding O_DENY* flags years ago:
https://lwn.net/Articles/581005/
I keep thinking I should look back at those some day but will probably
never get to it.
I've no idea how Windows applications use them, though I'm told it's
common.
--b.
On Fri, Aug 13, 2021, at 5:54 PM, Linus Torvalds wrote:
On Fri, Aug 13, 2021 at 2:49 PM Andy Lutomirski [off-list ref] wrote:
quoted
I’ll bite. How about we attack this in the opposite direction: remove the deny write mechanism entirely.
I think that would be ok, except I can see somebody relying on it.
It's broken, it's stupid, but we've done that ETXTBUSY for a _loong_ time.
Someone off-list just pointed something out to me, and I think we should push harder to remove ETXTBSY. Specifically, we've all been focused on open() failing with ETXTBSY, and it's easy to make fun of anyone opening a running program for write when they should be unlinking and replacing it.
Alas, Linux's implementation of deny_write_access() is correct^Wabsurd, and deny_write_access() *also* returns ETXTBSY if the file is open for write. So, in a multithreaded program, one thread does:
fd = open("some exefile", O_RDWR | O_CREAT | O_CLOEXEC);
write(fd, some stuff);
<--- problem is here
close(fd);
execve("some exefile");
Another thread does:
fork();
execve("something else");
In between fork and execve, there's another copy of the open file description, and i_writecount is held, and the execve() fails. Whoops. See, for example:
https://github.com/golang/go/issues/22315
I propose we get rid of deny_write_access() completely to solve this.
Getting rid of i_writecount itself seems a bit harder, since a handful of filesystems use it for clever reasons.
(OFD locks seem like they might have the same problem. Maybe we should have a clone() flag to unshare the file table and close close-on-exec things?)
But you are right that we have removed parts of it over time (no more
MAP_DENYWRITE, no more uselib()) so that what we have today is a
fairly weak form of what we used to do.
And nobody really complained when we weakened it, so maybe removing it
entirely might be acceptable.
Linus
From: David Hildenbrand <hidden> Date: 2021-08-26 21:47:15
On 26.08.21 19:48, Andy Lutomirski wrote:
On Fri, Aug 13, 2021, at 5:54 PM, Linus Torvalds wrote:
quoted
On Fri, Aug 13, 2021 at 2:49 PM Andy Lutomirski [off-list ref] wrote:
quoted
I’ll bite. How about we attack this in the opposite direction: remove the deny write mechanism entirely.
I think that would be ok, except I can see somebody relying on it.
It's broken, it's stupid, but we've done that ETXTBUSY for a _loong_ time.
Someone off-list just pointed something out to me, and I think we should push harder to remove ETXTBSY. Specifically, we've all been focused on open() failing with ETXTBSY, and it's easy to make fun of anyone opening a running program for write when they should be unlinking and replacing it.
Alas, Linux's implementation of deny_write_access() is correct^Wabsurd, and deny_write_access() *also* returns ETXTBSY if the file is open for write. So, in a multithreaded program, one thread does:
fd = open("some exefile", O_RDWR | O_CREAT | O_CLOEXEC);
write(fd, some stuff);
<--- problem is here
close(fd);
execve("some exefile");
Another thread does:
fork();
execve("something else");
In between fork and execve, there's another copy of the open file description, and i_writecount is held, and the execve() fails. Whoops. See, for example:
https://github.com/golang/go/issues/22315
I propose we get rid of deny_write_access() completely to solve this.
Getting rid of i_writecount itself seems a bit harder, since a handful of filesystems use it for clever reasons.
(OFD locks seem like they might have the same problem. Maybe we should have a clone() flag to unshare the file table and close close-on-exec things?)
It's not like this issue is new (^2017) or relevant in practice. So no need to hurry IMHO. One step at a time: it might make perfect sense to remove ETXTBSY, but we have to be careful to not break other user space that actually cares about the current behavior in practice.
--
Thanks,
David / dhildenb
From: Christian Brauner <hidden> Date: 2021-08-27 10:19:13
On Thu, Aug 26, 2021 at 11:47:07PM +0200, David Hildenbrand wrote:
On 26.08.21 19:48, Andy Lutomirski wrote:
quoted
On Fri, Aug 13, 2021, at 5:54 PM, Linus Torvalds wrote:
quoted
On Fri, Aug 13, 2021 at 2:49 PM Andy Lutomirski [off-list ref] wrote:
quoted
I’ll bite. How about we attack this in the opposite direction: remove the deny write mechanism entirely.
I think that would be ok, except I can see somebody relying on it.
It's broken, it's stupid, but we've done that ETXTBUSY for a _loong_ time.
Someone off-list just pointed something out to me, and I think we should push harder to remove ETXTBSY. Specifically, we've all been focused on open() failing with ETXTBSY, and it's easy to make fun of anyone opening a running program for write when they should be unlinking and replacing it.
Alas, Linux's implementation of deny_write_access() is correct^Wabsurd, and deny_write_access() *also* returns ETXTBSY if the file is open for write. So, in a multithreaded program, one thread does:
fd = open("some exefile", O_RDWR | O_CREAT | O_CLOEXEC);
write(fd, some stuff);
<--- problem is here
close(fd);
execve("some exefile");
Another thread does:
fork();
execve("something else");
In between fork and execve, there's another copy of the open file description, and i_writecount is held, and the execve() fails. Whoops. See, for example:
https://github.com/golang/go/issues/22315
I propose we get rid of deny_write_access() completely to solve this.
Getting rid of i_writecount itself seems a bit harder, since a handful of filesystems use it for clever reasons.
(OFD locks seem like they might have the same problem. Maybe we should have a clone() flag to unshare the file table and close close-on-exec things?)
It's not like this issue is new (^2017) or relevant in practice. So no need
to hurry IMHO. One step at a time: it might make perfect sense to remove
ETXTBSY, but we have to be careful to not break other user space that
actually cares about the current behavior in practice.
I agree. As I at least tried to show, removing write-protection can make
some exploits easier. I'm all for trying to remove this if it simplifies
things but for sure this shouldn't be part of this patchset and we
should be careful about it.
The removal of a (misguided or only partially functioning) protection
mechanism doesn't introduce but removes a failure point.
And I don't think removal and addition of a failure point usually have
the same consequences. Introducing a new failure point will often mean
userspace quickly detects regressions. Such regressions are pretty
common due to security fixes we introduce. Recent examples include [1].
Right after this was merged the regression was reported.
But when allowing behavior that used to fail like ETXTBSY it can be
difficult for userspace to detect such regressions. The reason for that
is quite often that userspace applications don't tend to do something
that they know upfront will fail. Attackers however might.
[1]: bfb819ea20ce ("proc: Check /proc/$pid/attr/ writes against file opener")
Christian