* Re: posix_spawn @ 2019-09-30 21:15 Joshua Hudson 2019-09-30 22:36 ` posix_spawn Rich Felker 0 siblings, 1 reply; 13+ messages in thread From: Joshua Hudson @ 2019-09-30 21:15 UTC (permalink / raw) To: musl >> posix_spawn can't be used in the originating location > why? chdir(); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-09-30 21:15 posix_spawn Joshua Hudson @ 2019-09-30 22:36 ` Rich Felker 2019-10-01 1:58 ` Joshua Hudson 0 siblings, 1 reply; 13+ messages in thread From: Rich Felker @ 2019-09-30 22:36 UTC (permalink / raw) To: Joshua Hudson; +Cc: musl On Mon, Sep 30, 2019 at 02:15:05PM -0700, Joshua Hudson wrote: > >> posix_spawn can't be used in the originating location > > > why? > > chdir(); See commit 74244e5b3ed4a61d99c5fc0967b69e5c9a753456: "add posix_spawn [f]chdir file actions". Rich ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-09-30 22:36 ` posix_spawn Rich Felker @ 2019-10-01 1:58 ` Joshua Hudson 2019-10-01 2:21 ` Rich Felker 0 siblings, 1 reply; 13+ messages in thread From: Joshua Hudson @ 2019-10-01 1:58 UTC (permalink / raw) To: Rich Felker; +Cc: musl Well that was a long dead end. posix_spawnp won't call setgroups. You now have a quirk and I need to actually detect musl libc. https://www.openwall.com/lists/musl/2019/06/29/6 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 1:58 ` Joshua Hudson @ 2019-10-01 2:21 ` Rich Felker 2019-10-01 2:41 ` Joshua Hudson 0 siblings, 1 reply; 13+ messages in thread From: Rich Felker @ 2019-10-01 2:21 UTC (permalink / raw) To: Joshua Hudson; +Cc: musl On Mon, Sep 30, 2019 at 06:58:15PM -0700, Joshua Hudson wrote: > Well that was a long dead end. posix_spawnp won't call setgroups. In a worst case, you use a helper executable (or shell script) to exec the final program you want running as the child with the changes you want made to its initial execution environment. This can work around any deficiency in posix_spawn capabilities. It adds a little bit of cost but it's nowhere near as big as the cost of fork duplicating the whole VM space and making it all COW. > You now have a quirk and I need to actually detect musl libc. Huh? This does not sound musl-specific. Rich ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 2:21 ` Rich Felker @ 2019-10-01 2:41 ` Joshua Hudson 2019-10-01 2:55 ` Rich Felker 0 siblings, 1 reply; 13+ messages in thread From: Joshua Hudson @ 2019-10-01 2:41 UTC (permalink / raw) To: Rich Felker; +Cc: musl > > You now have a quirk and I need to actually detect musl libc. > Huh? This does not sound musl-specific. Musl seems to be the only library that actually implements vfork shared memory that can't tolerate calling setuid() inside it. This patch should take care of the issue. diff --git a/src/thread/synccall.c b/src/thread/synccall.c index 648a6ad4..e152ccfe 100644 --- a/src/thread/synccall.c +++ b/src/thread/synccall.c @@ -48,6 +48,9 @@ void __synccall(void (*func)(void *), void *ctx) struct sigaction sa = { .sa_flags = SA_RESTART, .sa_handler = handler }; pthread_t self = __pthread_self(), td; int count = 0; + /* If we aren't in the process we think we're in, this is the best we + * can hope for. */ + if (__pthread_self()->tid != syscall(SYS_gettid)) goto single_threaded; /* Blocking signals in two steps, first only app-level signals * before taking the lock, then all signals after taking the lock, ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 2:41 ` Joshua Hudson @ 2019-10-01 2:55 ` Rich Felker 2019-10-01 7:05 ` Florian Weimer 0 siblings, 1 reply; 13+ messages in thread From: Rich Felker @ 2019-10-01 2:55 UTC (permalink / raw) To: Joshua Hudson; +Cc: musl On Mon, Sep 30, 2019 at 07:41:32PM -0700, Joshua Hudson wrote: > > > You now have a quirk and I need to actually detect musl libc. > > > Huh? This does not sound musl-specific. > > Musl seems to be the only library that actually implements vfork > shared memory that can't tolerate calling setuid() inside it. Oh, so you're still trying to do something that is documented as invalid... > This patch should take care of the issue. > > diff --git a/src/thread/synccall.c b/src/thread/synccall.c > index 648a6ad4..e152ccfe 100644 > --- a/src/thread/synccall.c > +++ b/src/thread/synccall.c > @@ -48,6 +48,9 @@ void __synccall(void (*func)(void *), void *ctx) > struct sigaction sa = { .sa_flags = SA_RESTART, .sa_handler = handler }; > pthread_t self = __pthread_self(), td; > int count = 0; > + /* If we aren't in the process we think we're in, this is the best we > + * can hope for. */ > + if (__pthread_self()->tid != syscall(SYS_gettid)) goto single_threaded; > > /* Blocking signals in two steps, first only app-level signals > * before taking the lock, then all signals after taking the lock, This is not safe and creates a false sense that something broken might work. Moreover it's a vulnerability to use it this way. You have a window where different tasks sharing VM space are executing with different privilege levels, and thereby one is able to seize execution of the other and achieve its privilege level. This is the whole situation that the robust multithreaded set*id() is designed to preclude. A better patch here would be: + if (__pthread_self()->tid != syscall(SYS_gettid)) a_crash(); to prevent forward progress in a process with dangerously corrupt state. Rich ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 2:55 ` Rich Felker @ 2019-10-01 7:05 ` Florian Weimer 2019-10-01 11:42 ` Rich Felker 0 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2019-10-01 7:05 UTC (permalink / raw) To: Rich Felker; +Cc: Joshua Hudson, musl * Rich Felker: > This is not safe and creates a false sense that something broken might > work. Moreover it's a vulnerability to use it this way. You have a > window where different tasks sharing VM space are executing with > different privilege levels, and thereby one is able to seize execution > of the other and achieve its privilege level. That's a non-sequitur. A shared address space does not necessarily mean that execution under one set of credentials will have unrestricted effects on executions under different credentials within the same address space. If the executions themselves are constrained, this can be completely safe. It is true that if there is one unconstrained execution in an address space, then the whole thing is tainted, but that this isn't the relevant scenario for things like file servers (which do not execute code on behalf of clients). I don't know where you got this idea, but it is wrong. I'm sorry. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 7:05 ` Florian Weimer @ 2019-10-01 11:42 ` Rich Felker 2019-10-01 14:07 ` posix_spawn Joshua Hudson 2019-10-16 12:40 ` Florian Weimer 0 siblings, 2 replies; 13+ messages in thread From: Rich Felker @ 2019-10-01 11:42 UTC (permalink / raw) To: Florian Weimer; +Cc: Joshua Hudson, musl On Tue, Oct 01, 2019 at 09:05:18AM +0200, Florian Weimer wrote: > * Rich Felker: > > > This is not safe and creates a false sense that something broken might > > work. Moreover it's a vulnerability to use it this way. You have a > > window where different tasks sharing VM space are executing with > > different privilege levels, and thereby one is able to seize execution > > of the other and achieve its privilege level. > > That's a non-sequitur. A shared address space does not necessarily mean > that execution under one set of credentials will have unrestricted > effects on executions under different credentials within the same > address space. It does, but not necessarily in all circumstances. The case in which is it dangerous is when one of the tasks is "dropping privileges" before executing code that either intentionally (e.g. a login session, script interpreter, etc. acting behalf of the new user) or unintentionally (because the code after dropping privileges is not as heavily scrutinized and has a vulnerability) lets the attacker execute code they control. In that case, the now-attacker-controlled task can perform operations on the VM space of the privileged task, e.g. using mmap to replace the code it's executing with whatever it wants. This issue is why it's so important that setuid, etc. not return before all threads have been confirmed to have completed the operation (just queuing or initiating it for them all is not enough). Rich ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: posix_spawn 2019-10-01 11:42 ` Rich Felker @ 2019-10-01 14:07 ` Joshua Hudson 2019-10-01 14:15 ` posix_spawn Florian Weimer 2019-10-16 12:40 ` Florian Weimer 1 sibling, 1 reply; 13+ messages in thread From: Joshua Hudson @ 2019-10-01 14:07 UTC (permalink / raw) To: Rich Felker; +Cc: Florian Weimer, musl [-- Attachment #1: Type: text/plain, Size: 244 bytes --] You guys open to adding more extensions to posix_spawn? The code that I'm actually trying to run is setgroups;setgid;setuid so I think the use case is common. More of the security-critical code being in the library is generally a good thing. [-- Attachment #2: Type: text/html, Size: 301 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: posix_spawn 2019-10-01 14:07 ` posix_spawn Joshua Hudson @ 2019-10-01 14:15 ` Florian Weimer 2019-10-01 14:44 ` posix_spawn Rich Felker 0 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2019-10-01 14:15 UTC (permalink / raw) To: Joshua Hudson; +Cc: Rich Felker, musl * Joshua Hudson: > You guys open to adding more extensions to posix_spawn? > > The code that I'm actually trying to run is setgroups;setgid;setuid so > I think the use case is common. > > More of the security-critical code being in the library is generally a > good thing. Rich is objecting to my glibc changes. We require unanimous consent at present. Unless I can convince him that his analysis is incorrect, that essentially kills further API additions in this area. Thanks, Florian ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 14:15 ` posix_spawn Florian Weimer @ 2019-10-01 14:44 ` Rich Felker 2019-10-01 15:06 ` Rich Felker 0 siblings, 1 reply; 13+ messages in thread From: Rich Felker @ 2019-10-01 14:44 UTC (permalink / raw) To: Florian Weimer; +Cc: Joshua Hudson, musl On Tue, Oct 01, 2019 at 04:15:54PM +0200, Florian Weimer wrote: > * Joshua Hudson: > > > You guys open to adding more extensions to posix_spawn? > > > > The code that I'm actually trying to run is setgroups;setgid;setuid so > > I think the use case is common. > > > > More of the security-critical code being in the library is generally a > > good thing. > > Rich is objecting to my glibc changes. We require unanimous consent at > present. Unless I can convince him that his analysis is incorrect, that > essentially kills further API additions in this area. Can you point me to which ones you're referring to? I'm not generally opposed to extensions to solve the deficiencies in posix_spawn, but there may be one I'm not remembering where there was something about it in particular that I found problematic. Rich ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 14:44 ` posix_spawn Rich Felker @ 2019-10-01 15:06 ` Rich Felker 0 siblings, 0 replies; 13+ messages in thread From: Rich Felker @ 2019-10-01 15:06 UTC (permalink / raw) To: Florian Weimer; +Cc: Joshua Hudson, musl On Tue, Oct 01, 2019 at 10:44:55AM -0400, Rich Felker wrote: > On Tue, Oct 01, 2019 at 04:15:54PM +0200, Florian Weimer wrote: > > * Joshua Hudson: > > > > > You guys open to adding more extensions to posix_spawn? > > > > > > The code that I'm actually trying to run is setgroups;setgid;setuid so > > > I think the use case is common. > > > > > > More of the security-critical code being in the library is generally a > > > good thing. > > > > Rich is objecting to my glibc changes. We require unanimous consent at > > present. Unless I can convince him that his analysis is incorrect, that > > essentially kills further API additions in this area. > > Can you point me to which ones you're referring to? I'm not generally > opposed to extensions to solve the deficiencies in posix_spawn, but > there may be one I'm not remembering where there was something about > it in particular that I found problematic. Ah, if it's this particular feature, arbitrary credential changes rather than just resetting effective ids back to real ones, then there are concerns like in this thread about tasks with different credentials sharing same VM space. However, I think since there's no opportunity to control execution in the child until after execve, it can probably be made safe; if not, CLONE_VM could be omitted when the relevant attribute is used. Rich ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: posix_spawn 2019-10-01 11:42 ` Rich Felker 2019-10-01 14:07 ` posix_spawn Joshua Hudson @ 2019-10-16 12:40 ` Florian Weimer 1 sibling, 0 replies; 13+ messages in thread From: Florian Weimer @ 2019-10-16 12:40 UTC (permalink / raw) To: Rich Felker; +Cc: Joshua Hudson, musl * Rich Felker: > On Tue, Oct 01, 2019 at 09:05:18AM +0200, Florian Weimer wrote: >> * Rich Felker: >> >> > This is not safe and creates a false sense that something broken might >> > work. Moreover it's a vulnerability to use it this way. You have a >> > window where different tasks sharing VM space are executing with >> > different privilege levels, and thereby one is able to seize execution >> > of the other and achieve its privilege level. >> >> That's a non-sequitur. A shared address space does not necessarily mean >> that execution under one set of credentials will have unrestricted >> effects on executions under different credentials within the same >> address space. > > It does, but not necessarily in all circumstances. The case in which > is it dangerous is when one of the tasks is "dropping privileges" > before executing code that either intentionally (e.g. a login session, > script interpreter, etc. acting behalf of the new user) or > unintentionally (because the code after dropping privileges is not as > heavily scrutinized and has a vulnerability) lets the attacker execute > code they control. In that case, the now-attacker-controlled task can > perform operations on the VM space of the privileged task, e.g. using > mmap to replace the code it's executing with whatever it wants. I'm still not convinced, sorry. setuid to a lower-privileged user does not give that user access to the process. Linux has an independent flag for that, and if you want to grant that level of access to an existing process, you have to set it explicitly. Sharing address space is just a tiny aspect here. If the process contains secrets (and some people would consider just the load address such a secret, especially with forking server processes), exposing it to other users could be problematic, even if there is no address space sharing involved. I don't understand the focus on setuid, to be honest. In practice, there are things that are much more dangerous for privileged processes (such as chroot or even chdir, or just plain old open). I'm not arguing for the sake of it, your skepticism (or should I say objection) probably blocks acceptance of my glibc patches in this area. (I think you mentioned that on libc-alpha at one point at least.) Just to reiterate, my motivation comes from analyzing actual system call usage in existing file servers (Samba and nfs-ganesha). They use per-thread credentials (via direct system calls to change the effective IDs) and a per-thread current directory (via unshare (CLONE_FS)). So in a sense, all we can do at this point is harm reduction by providing documented interfaces which spell out their limitations. Thanks, Florian ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-10-16 12:40 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-30 21:15 posix_spawn Joshua Hudson 2019-09-30 22:36 ` posix_spawn Rich Felker 2019-10-01 1:58 ` Joshua Hudson 2019-10-01 2:21 ` Rich Felker 2019-10-01 2:41 ` Joshua Hudson 2019-10-01 2:55 ` Rich Felker 2019-10-01 7:05 ` Florian Weimer 2019-10-01 11:42 ` Rich Felker 2019-10-01 14:07 ` posix_spawn Joshua Hudson 2019-10-01 14:15 ` posix_spawn Florian Weimer 2019-10-01 14:44 ` posix_spawn Rich Felker 2019-10-01 15:06 ` Rich Felker 2019-10-16 12:40 ` Florian Weimer
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).