| Message ID | 20231003201151.1406279-3-siddhesh@sourceware.org |
|---|---|
| State | New |
| Headers | show |
| Series | make all tunables SXID_ERASE | expand |
The 10/03/2023 16:11, Siddhesh Poyarekar wrote: > Limit effect of memory tagging to the same process and don't let it > bleed across privilege boundaries into non-setuid children of setuid > processes. > > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> the description does not match the documented behaviour of SXID_IGNORE. (the setuid binary passes on the setting from its parent, i don't see the privilege boundary crossing) and it does not explain why would you want to turn a security hardening feature off. i'm not against the patch as the heap tagging feature is very experimental at this point, but it needs a better explanation. > --- > elf/dl-tunables.list | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list > index 42d8ffd06d..44baf10eaa 100644 > --- a/elf/dl-tunables.list > +++ b/elf/dl-tunables.list > @@ -152,7 +152,6 @@ glibc { > type: INT_32 > minval: 0 > maxval: 255 > - security_level: SXID_IGNORE > } > } > > -- > 2.41.0 >
On 2023-10-04 03:29, Szabolcs Nagy wrote: > The 10/03/2023 16:11, Siddhesh Poyarekar wrote: >> Limit effect of memory tagging to the same process and don't let it >> bleed across privilege boundaries into non-setuid children of setuid >> processes. >> >> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > > the description does not match the documented behaviour of > SXID_IGNORE. (the setuid binary passes on the setting from > its parent, i don't see the privilege boundary crossing) Maybe "privilege boundary crossing" is too strong a phrase, how about "bleed across different users or groups"? > and it does not explain why would you want to turn a security > hardening feature off. > > i'm not against the patch as the heap tagging feature is > very experimental at this point, but it needs a better > explanation. How about: """ Memory tagging is still an experimental feature, so limit propagation of tunables across setxid binaries. """ In future though, would you want SXID_IGNORE for memory tagging? I would expect that once memory tagging becomes a stable feature you'd want it to be enabled by default and disabled by, e.g. a systemwide tunable. I can't see why you'd want it to go across the setxid boundary. Thanks, Sid
The 10/04/2023 07:59, Siddhesh Poyarekar wrote: > On 2023-10-04 03:29, Szabolcs Nagy wrote: > > The 10/03/2023 16:11, Siddhesh Poyarekar wrote: > > > Limit effect of memory tagging to the same process and don't let it > > > bleed across privilege boundaries into non-setuid children of setuid > > > processes. > > > > > > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > > > > the description does not match the documented behaviour of > > SXID_IGNORE. (the setuid binary passes on the setting from > > its parent, i don't see the privilege boundary crossing) > > Maybe "privilege boundary crossing" is too strong a phrase, how about "bleed > across different users or groups"? > > > and it does not explain why would you want to turn a security > > hardening feature off. > > > > i'm not against the patch as the heap tagging feature is > > very experimental at this point, but it needs a better > > explanation. > > How about: > > """ > Memory tagging is still an experimental feature, so limit propagation of > tunables across setxid binaries. > """ well i don't mind the wording, but i wanted to see an actual justification. "this is experiemental" is not useful. "limit propagation across setxid binaries" answers what the patch does, but not why. is there an actual problem you are trying to solve? do you think SXID_IGNORE is not suitable for security hardening features? what is the intended usage of it? i don't see anything immediately wrong with inheriting env from a grandparent process if in between there was a setuid process that ignored the env. (i also don't see it as very useful/necessary) > In future though, would you want SXID_IGNORE for memory tagging? I would > expect that once memory tagging becomes a stable feature you'd want it to be > enabled by default and disabled by, e.g. a systemwide tunable. I can't see > why you'd want it to go across the setxid boundary. it may not be enabled by default because of its overhead (we need hw to decide). i think it is unexpected that setxid drops env vars (ignoring is ok, but dropping is weird). i can live with the 'drop' semantics but then why do we have SXID_IGNORE?
On 2023-10-04 10:04, Szabolcs Nagy wrote: > well i don't mind the wording, but i wanted to see > an actual justification. > > "this is experiemental" is not useful. > > "limit propagation across setxid binaries" answers > what the patch does, but not why. > > is there an actual problem you are trying to solve? > do you think SXID_IGNORE is not suitable for security > hardening features? what is the intended usage of it? > > i don't see anything immediately wrong with inheriting > env from a grandparent process if in between there was > a setuid process that ignored the env. (i also don't > see it as very useful/necessary) The actual problem I'm trying to solve is to get rid of SXID_IGNORE and SXID_NONE and the first step is to drop users of those levels. Supporting those levels requires additional processing in __libc_enable_secure, which is a source of bugs like CVE-2023-4911. I'm not convinced that we really *need* to have tunables go across sxid boundaries, so I want to flip the question around: do you think it is *necessary* for memory tagging tunables to percolate sxid boundaries? If not, then they should stay at SXID_ERASE. >> In future though, would you want SXID_IGNORE for memory tagging? I would >> expect that once memory tagging becomes a stable feature you'd want it to be >> enabled by default and disabled by, e.g. a systemwide tunable. I can't see >> why you'd want it to go across the setxid boundary. > > it may not be enabled by default because of its overhead > (we need hw to decide). > > i think it is unexpected that setxid drops env vars > (ignoring is ok, but dropping is weird). > > i can live with the 'drop' semantics but then why do > we have SXID_IGNORE? They were introduced for compatibility with the original semantics of MALLOC_MMAP_THRESHOLD_ and friends, that would go across sxid boundaries. In those cases too I'm not convinced that the use case really needs to be supported and that users should find a way to achieve that effect in other ways, such as having a wrapper around the non-setuid child that needs to see these tunables and export the tunables in that wrapper. The end goal here is to drop everything but SXID_ERASE so that we don't have to do string twiddling under __libc_enable_secure. The other alternative to achieve the same effect (i.e. not twiddle strings in __libc_enable_secure) could be to make *everything* SXID_IGNORE, which would then leave GLIBC_TUNABLES untouched for non-setuid children to do what they please with it. Sid
The 10/04/2023 10:23, Siddhesh Poyarekar wrote: > The actual problem I'm trying to solve is to get rid of SXID_IGNORE and > SXID_NONE and the first step is to drop users of those levels. Supporting > those levels requires additional processing in __libc_enable_secure, which > is a source of bugs like CVE-2023-4911. I'm not convinced that we really > *need* to have tunables go across sxid boundaries, so I want to flip the > question around: do you think it is *necessary* for memory tagging tunables > to percolate sxid boundaries? If not, then they should stay at SXID_ERASE. ... > The end goal here is to drop everything but SXID_ERASE so that we don't have > to do string twiddling under __libc_enable_secure. > > The other alternative to achieve the same effect (i.e. not twiddle strings > in __libc_enable_secure) could be to make *everything* SXID_IGNORE, which > would then leave GLIBC_TUNABLES untouched for non-setuid children to do what > they please with it. personally i think setuid binaries should not touch the env, just ignore it, so everything SXID_IGNORE. it's not just the string twiddling but tunables_strdup is a problem too (it can fail, needs early alloc,..). ideally tunables_init would be a nop with AT_SECURE and otherwise parsed the env var without malloc and touching the env[] array passed by the kernel.
On 2023-10-04 10:51, Szabolcs Nagy wrote: > personally i think setuid binaries should not touch the > env, just ignore it, so everything SXID_IGNORE. > > it's not just the string twiddling but tunables_strdup > is a problem too (it can fail, needs early alloc,..). > ideally tunables_init would be a nop with AT_SECURE and > otherwise parsed the env var without malloc and touching > the env[] array passed by the kernel. > Sure, that could be a less contentious change. I'll send v2 with it. Thanks, Sid
On 2023-10-04 10:53, Siddhesh Poyarekar wrote: > On 2023-10-04 10:51, Szabolcs Nagy wrote: >> personally i think setuid binaries should not touch the >> env, just ignore it, so everything SXID_IGNORE. >> >> it's not just the string twiddling but tunables_strdup >> is a problem too (it can fail, needs early alloc,..). >> ideally tunables_init would be a nop with AT_SECURE and >> otherwise parsed the env var without malloc and touching >> the env[] array passed by the kernel. >> > > Sure, that could be a less contentious change. I'll send v2 with it. I should clarify though, that I'm not removing tunables_strdup because it's still needed for regular tunables processing. The v2 I'm testing will simply make all tunables SXID_IGNORE and tunables_init a nop with AT_SECURE. Sid
On 04/10/23 11:51, Szabolcs Nagy wrote: > The 10/04/2023 10:23, Siddhesh Poyarekar wrote: >> The actual problem I'm trying to solve is to get rid of SXID_IGNORE and >> SXID_NONE and the first step is to drop users of those levels. Supporting >> those levels requires additional processing in __libc_enable_secure, which >> is a source of bugs like CVE-2023-4911. I'm not convinced that we really >> *need* to have tunables go across sxid boundaries, so I want to flip the >> question around: do you think it is *necessary* for memory tagging tunables >> to percolate sxid boundaries? If not, then they should stay at SXID_ERASE. > > ... > >> The end goal here is to drop everything but SXID_ERASE so that we don't have >> to do string twiddling under __libc_enable_secure. >> >> The other alternative to achieve the same effect (i.e. not twiddle strings >> in __libc_enable_secure) could be to make *everything* SXID_IGNORE, which >> would then leave GLIBC_TUNABLES untouched for non-setuid children to do what >> they please with it. > > personally i think setuid binaries should not touch the > env, just ignore it, so everything SXID_IGNORE. Do you mean just drop unsecvars and not filter out glibc environment variables for AT_SECURE? > > it's not just the string twiddling but tunables_strdup > is a problem too (it can fail, needs early alloc,..). > ideally tunables_init would be a nop with AT_SECURE and > otherwise parsed the env var without malloc and touching > the env[] array passed by the kernel. I complete agree, tunables_strdup was a source of headache due its allocation requirement and, now, for parsing tunables.
The 10/04/2023 14:01, Adhemerval Zanella Netto wrote: > On 04/10/23 11:51, Szabolcs Nagy wrote: > > > > personally i think setuid binaries should not touch the > > env, just ignore it, so everything SXID_IGNORE. > > Do you mean just drop unsecvars and not filter out glibc environment variables > for AT_SECURE? i'd fix this in one place that makes the behaviour easy to reason about: _dl_next_ld_env_entry in rtld should just return empty in secure mode and same for getenv, internally it should return empty. then we know that nothing in libc can depend on the env. (if something parses env directly that should be fixed) if anything, there should be a whitelist, not blacklist of env vars. of course this changes behaviour, so if we want to be bw compat then we have to live with the current unsetenv logic.
On 2023-10-05 04:19, Szabolcs Nagy wrote: > i'd fix this in one place that makes the behaviour easy > to reason about: _dl_next_ld_env_entry in rtld should > just return empty in secure mode and same for getenv, > internally it should return empty. > > then we know that nothing in libc can depend on the env. > (if something parses env directly that should be fixed) > > if anything, there should be a whitelist, not blacklist > of env vars. That won't work because it would require knowledge of (or a mechanism to specify) safety of environment variables used by the application and its children. The current unsecvars approach is probably the best option. > of course this changes behaviour, so if we want to be > bw compat then we have to live with the current unsetenv > logic. The current unsetenv logic is well reasoned IMO; the tunables layer made it complicated and it ought to be sufficient to just remove that. But that would require dropping the memory tagging tunable from SXID_IGNORE and erasing GLIBC_TUNABLES by putting it in unsecvars.h. Thanks, Sid
The 10/05/2023 08:55, Siddhesh Poyarekar wrote: > On 2023-10-05 04:19, Szabolcs Nagy wrote: > > i'd fix this in one place that makes the behaviour easy > > to reason about: _dl_next_ld_env_entry in rtld should > > just return empty in secure mode and same for getenv, > > internally it should return empty. > > > > then we know that nothing in libc can depend on the env. > > (if something parses env directly that should be fixed) > > > > if anything, there should be a whitelist, not blacklist > > of env vars. > > That won't work because it would require knowledge of (or a mechanism to > specify) safety of environment variables used by the application and its > children. The current unsecvars approach is probably the best option. why would you need a whitelist of application envvars? if there is any env var usage *in libc* that is valid to affect setuid binaries then those should be whitelisted. (black list works too, but more error prone in imo) > > of course this changes behaviour, so if we want to be > > bw compat then we have to live with the current unsetenv > > logic. > > The current unsetenv logic is well reasoned IMO; the tunables layer made it > complicated and it ought to be sufficient to just remove that. But that > would require dropping the memory tagging tunable from SXID_IGNORE and > erasing GLIBC_TUNABLES by putting it in unsecvars.h. i think it is broken to rewrite env[] that is passed by the kernel. but since glibc always did this i guess it's fine.
On 2023-10-05 09:59, Szabolcs Nagy wrote: > The 10/05/2023 08:55, Siddhesh Poyarekar wrote: >> On 2023-10-05 04:19, Szabolcs Nagy wrote: >>> i'd fix this in one place that makes the behaviour easy >>> to reason about: _dl_next_ld_env_entry in rtld should >>> just return empty in secure mode and same for getenv, >>> internally it should return empty. >>> >>> then we know that nothing in libc can depend on the env. >>> (if something parses env directly that should be fixed) >>> >>> if anything, there should be a whitelist, not blacklist >>> of env vars. >> >> That won't work because it would require knowledge of (or a mechanism to >> specify) safety of environment variables used by the application and its >> children. The current unsecvars approach is probably the best option. > > why would you need a whitelist of application envvars? > > if there is any env var usage *in libc* that is valid to > affect setuid binaries then those should be whitelisted. > (black list works too, but more error prone in imo) So do you mean (1) maintain a full list of envvars recognized by glibc and then (2) designate specific ones as safe? That would be a good idea. Sid
On Thu, Oct 5, 2023, at 9:59 AM, Szabolcs Nagy wrote: > The 10/05/2023 08:55, Siddhesh Poyarekar wrote: >> The current unsetenv logic is well reasoned IMO; the tunables layer made it >> complicated and it ought to be sufficient to just remove that. But that >> would require dropping the memory tagging tunable from SXID_IGNORE and >> erasing GLIBC_TUNABLES by putting it in unsecvars.h. > > i think it is broken to rewrite env[] that is passed by > the kernel. but since glibc always did this i guess it's > fine. I think the CVE that prompted this discussion demonstrates that it's *insecure* to allow children of setxid processes to inherit any environment variable that is considered insecure to consult in the setxid process itself. I also think we ought to be talking about a very short *whitelist* of environment variables that are allowed to survive execve() of a setxid binary -- off the top of my head, TERM, LANG, LANGUAGE, LC_*, and maybe *nothing else* -- and putting that list into the kernel itself. zw
On 2023-10-05 14:31, Zack Weinberg wrote: > On Thu, Oct 5, 2023, at 9:59 AM, Szabolcs Nagy wrote: >> The 10/05/2023 08:55, Siddhesh Poyarekar wrote: >>> The current unsetenv logic is well reasoned IMO; the tunables layer made it >>> complicated and it ought to be sufficient to just remove that. But that >>> would require dropping the memory tagging tunable from SXID_IGNORE and >>> erasing GLIBC_TUNABLES by putting it in unsecvars.h. >> >> i think it is broken to rewrite env[] that is passed by >> the kernel. but since glibc always did this i guess it's >> fine. > > I think the CVE that prompted this discussion demonstrates that it's *insecure* > to allow children of setxid processes to inherit any environment variable that is > considered insecure to consult in the setxid process itself. I don't completely disagree with the conclusion below, but the CVE that prompted this discussion doesn't say anything about environment inheritance because the vulnerability had nothing to do with environment processing and inheritance. The issue there is limited to complex parsing of a particular environment variable in a setxid context and the main lesson there IMO is to keep any kind of processing to a bare minimum in a setxid context. Processing for environment inheritance (specifically, cleaning out unsecvars) is fairly stable code that has stood the test of time. It makes sense like you suggest below, to make it an inclusion list rather than an exclusion list, but IMO that's a separate hardening exercise from ripping tunables out of the setxid context. > I also think we ought to be talking about a very short *whitelist* of environment > variables that are allowed to survive execve() of a setxid binary -- off the top > of my head, TERM, LANG, LANGUAGE, LC_*, and maybe *nothing else* -- and putting > that list into the kernel itself. > > zw > Thanks, Sid
On Thu, Oct 5, 2023, at 3:11 PM, Siddhesh Poyarekar wrote: > On 2023-10-05 14:31, Zack Weinberg wrote: >> On Thu, Oct 5, 2023, at 9:59 AM, Szabolcs Nagy wrote: >>> i think it is broken to rewrite env[] that is passed by the >>> kernel. but since glibc always did this i guess it's fine. >> >> I think the CVE that prompted this discussion demonstrates that >> it's *insecure* to allow children of setxid processes to inherit >> any environment variable that is considered insecure to consult in >> the setxid process itself. > > I don't completely disagree with the conclusion below, but the CVE > that prompted this discussion doesn't say anything about environment > inheritance because the vulnerability had nothing to do with > environment processing and inheritance. I may have misunderstood the CVE or mixed it up with another one. I thought there was a recent CVE in which a SXID_IGNORE environment variable was inherited by a child process, and that child process was rendered vulnerable to further exploitation because it honored that variable. > The issue there is limited to complex parsing of a particular > environment variable in a setxid context and the main lesson there > IMO is to keep any kind of processing to a bare minimum in a setxid > context. Agreed. > Processing for environment inheritance (specifically, cleaning out > unsecvars) is fairly stable code that has stood the test of time. > It makes sense like you suggest below, to make it an inclusion list > rather than an exclusion list, but IMO that's a separate hardening > exercise from ripping tunables out of the setxid context. Also agreed. zw
On 2023-10-06 08:25, Zack Weinberg wrote: >> I don't completely disagree with the conclusion below, but the CVE >> that prompted this discussion doesn't say anything about environment >> inheritance because the vulnerability had nothing to do with >> environment processing and inheritance. > > I may have misunderstood the CVE or mixed it up with another one. > I thought there was a recent CVE in which a SXID_IGNORE environment > variable was inherited by a child process, and that child process was > rendered vulnerable to further exploitation because it honored that > variable. Hmm, I don't remember anything like this recently (but my memory is worse than my airplane flying skills), but something like that would be an interesting data point and further confirmation that we need to get rid of SXID_IGNORE and SXID_NONE. In any case, I can't see a good reason anymore to keep these levels, especially if we drop memory tagging, malloc tuning and malloc debugging tunables from SXID_IGNORE. If memory tagging needs to persist across setxid programs then there needs to be a different way, maybe through systemwide tunables[1] that DJ is working on, or maybe even ELF markup. Adhemerval has taken this patchset and is going to build on it to rip it all out, so we'll hopefully resolve all of this together. Thanks, Sid [1] https://inbox.sourceware.org/libc-alpha/xn1qeayuo9.fsf@greed.delorie.com/T/#u
On 06/10/23 09:41, Siddhesh Poyarekar wrote: > On 2023-10-06 08:25, Zack Weinberg wrote: >>> I don't completely disagree with the conclusion below, but the CVE >>> that prompted this discussion doesn't say anything about environment >>> inheritance because the vulnerability had nothing to do with >>> environment processing and inheritance. >> >> I may have misunderstood the CVE or mixed it up with another one. >> I thought there was a recent CVE in which a SXID_IGNORE environment >> variable was inherited by a child process, and that child process was >> rendered vulnerable to further exploitation because it honored that >> variable. > > Hmm, I don't remember anything like this recently (but my memory is worse than my airplane flying skills), but something like that would be an interesting data point and further confirmation that we need to get rid of SXID_IGNORE and SXID_NONE. In any case, I can't see a good reason anymore to keep these levels, especially if we drop memory tagging, malloc tuning and malloc debugging tunables from SXID_IGNORE. If memory tagging needs to persist across setxid programs then there needs to be a different way, maybe through systemwide tunables[1] that DJ is working on, or maybe even ELF markup. > > Adhemerval has taken this patchset and is going to build on it to rip it all out, so we'll hopefully resolve all of this together. I think the less contiguous change would to keep with the blacklist of environment variables, since changing to whitelist will change the current semantic (and add possible user-visible breakage), ignore any tunable on AT_SECURE binaries (as some Linux distributions are already doing [1]), add GLIBC_TUNABLES to unsecvars (so if AT_SECURE binaries want to set any tunable, they should it explicit), and remove the requirement of duplice the GLIBC_TUNABLES tunable parsing (which some memory allocation failure). Yes, security hardening tunables won't be applied for AT_SECURE binaries, but I think opt-in security features that have performance implications should done by system administrator instead of relying on users. [1] https://git.altlinux.org/gears/g/glibc.git?p=glibc.git;a=commitdiff;h=5d1686416ab766f3dd0780ab730650c4c0f76ca9
On 2023-10-06 13:10, Adhemerval Zanella Netto wrote: >> Adhemerval has taken this patchset and is going to build on it to rip it all out, so we'll hopefully resolve all of this together. > > I think the less contiguous change would to keep with the blacklist of > environment variables, ... Yes of course, sorry I didn't mean to sneak up additional requirements on the work you're doing :) Thanks, Sid
On Fri, 6 Oct 2023 at 07:32, Zack Weinberg <zack@owlfolio.org> wrote: > I also think we ought to be talking about a very short *whitelist* of > environment > variables that are allowed to survive execve() of a setxid binary -- off > the top > of my head, TERM, LANG, LANGUAGE, LC_*, and maybe *nothing else* -- and > putting > that list into the kernel itself. > That would break at least one application I know about (snapd): https://bugs.launchpad.net/snapd/+bug/1682308 Cheers, mwh
On Sun, Oct 8, 2023, at 3:51 PM, Michael Hudson-Doyle wrote: > On Fri, 6 Oct 2023 at 07:32, Zack Weinberg <zack@owlfolio.org> wrote: >> I also think we ought to be talking about a very short *whitelist* of >> environment variables that are allowed to survive execve() of a >> setxid binary -- off the top of my head, TERM, LANG, LANGUAGE, LC_*, >> and maybe *nothing else* -- and putting that list into the kernel >> itself. > > That would break at least one application I know about (snapd): > https://bugs.launchpad.net/snapd/+bug/1682308 Flip answer: I don't think snapd ought to exist in the first place (because it violates the Highlander Principle of Package Management), so I don't care if it gets broken. More serious answer: The specific thing snapd is doing here could be handled via some sort of client-server protocol, with a *non*-setxid client. This would allow the code running at elevated privilege to treat the environment variables as an opaque blob of data to be copied into the nascent sandboxed process, which would be safer. Complementary serious answer: Are you sure there's no way to leverage the ability to set arbitrary environment variables inside the sandbox to carry out a sandbox escape? Are you *certain*? zw
diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list index 42d8ffd06d..44baf10eaa 100644 --- a/elf/dl-tunables.list +++ b/elf/dl-tunables.list @@ -152,7 +152,6 @@ glibc { type: INT_32 minval: 0 maxval: 255 - security_level: SXID_IGNORE } }
Limit effect of memory tagging to the same process and don't let it bleed across privilege boundaries into non-setuid children of setuid processes. Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> --- elf/dl-tunables.list | 1 - 1 file changed, 1 deletion(-)