| Message ID | 20170529213800.29438-3-matt@nmatt.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 29 May 2017 17:38:00 -0400 Matt Brown <matt@nmatt.com> wrote: > This introduces the tiocsti_restrict sysctl, whose default is controlled > via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control > restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. Which is really quite pointless as I keep pointing out and you keep reposting this nonsense. > > This patch depends on patch 1/2 > > This patch was inspired from GRKERNSEC_HARDEN_TTY. > > This patch would have prevented > https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following > conditions: > * non-privileged container > * container run inside new user namespace And assuming no other ioctl could be used in an attack. Only there are rather a lot of ways an app with access to a tty can cause mischief if it's the same controlling tty as the higher privileged context that launched it. Properly written code allocates a new pty/tty pair for the lower privileged session. If the code doesn't do that then your change merely modifies the degree of mayhem it can cause. If it does it right then your patch is pointless. > Possible effects on userland: > > There could be a few user programs that would be effected by this > change. In other words, it's yet another weird config option that breaks stuff. NAK v7. Alan
With all due respect sir, i believe your review falls short of the purpose of this effort - to harden the kernel against flaws in userspace. Comments along the line of "if <userspace> does it right then your patch is pointless" are not relevant to the context of securing kernel functions/interfaces. What userspace should do has little bearing on defensive measures actually implemented in the kernel - if we took the approach of "someone else is responsible for that" in military operations, the world would be a much darker and different place today. Those who have the luxury of standoff from the critical impacts of security vulnerabilities may not take into account the fact that peoples lives literally depend on Linux getting a lot more secure, and quickly. If this work were not valuable, it wouldnt be an enabled kernel option on a massive number of kernels with attack surfaces reduced by the compound protections offered by the grsec patch set. I can't speak for the grsec people, but having read a small fraction of the commentary around the subject of mainline integration, it seems to me that NAKs like this are exactly why they had no interest in even trying - this review is based on the cultural views of the kernel community, not on the security benefits offered by the work in the current state of affairs (where userspace is broken). The purpose of each of these protections (being ported over from grsec) is not to offer carte blanche defense against all attackers and vectors, but to prevent specific classes of bugs from reducing the security posture of the system. By implementing these defenses in a layered manner we can significantly reduce our kernel attack surface. Once userspace catches up and does things the right way, and has no capacity for doing them the wrong way (aka, nothing attackers can use to bypass the proper userspace behavior), then the functionality really does become pointless, and can then be removed. From a practical perspective, can alternative solutions be offered along with NAKs? Killing things on the vine isnt great, and if a security measure is being denied, upstream should provide their solution to how they want to address the problem (or just an outline to guide the hardened folks). On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: > On Mon, 29 May 2017 17:38:00 -0400 > Matt Brown <matt@nmatt.com> wrote: > >> This introduces the tiocsti_restrict sysctl, whose default is controlled >> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. > > Which is really quite pointless as I keep pointing out and you keep > reposting this nonsense. > >> >> This patch depends on patch 1/2 >> >> This patch was inspired from GRKERNSEC_HARDEN_TTY. >> >> This patch would have prevented >> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >> conditions: >> * non-privileged container >> * container run inside new user namespace > > And assuming no other ioctl could be used in an attack. Only there are > rather a lot of ways an app with access to a tty can cause mischief if > it's the same controlling tty as the higher privileged context that > launched it. > > Properly written code allocates a new pty/tty pair for the lower > privileged session. If the code doesn't do that then your change merely > modifies the degree of mayhem it can cause. If it does it right then your > patch is pointless. > >> Possible effects on userland: >> >> There could be a few user programs that would be effected by this >> change. > > In other words, it's yet another weird config option that breaks stuff. > > > NAK v7. > > Alan
On 5/29/17 6:26 PM, Alan Cox wrote: > On Mon, 29 May 2017 17:38:00 -0400 > Matt Brown <matt@nmatt.com> wrote: > >> This introduces the tiocsti_restrict sysctl, whose default is controlled >> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. > > Which is really quite pointless as I keep pointing out and you keep > reposting this nonsense. > >> >> This patch depends on patch 1/2 >> >> This patch was inspired from GRKERNSEC_HARDEN_TTY. >> >> This patch would have prevented >> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >> conditions: >> * non-privileged container >> * container run inside new user namespace > > And assuming no other ioctl could be used in an attack. Only there are > rather a lot of ways an app with access to a tty can cause mischief if > it's the same controlling tty as the higher privileged context that > launched it. Can you give me an example of another ioctl that you can abuse to practically gain code execution in the privileged context? Saying that the child process could "cause mischief" is a bit vague. > > Properly written code allocates a new pty/tty pair for the lower > privileged session. If the code doesn't do that then your change merely > modifies the degree of mayhem it can cause. If it does it right then your > patch is pointless. > >> Possible effects on userland: >> >> There could be a few user programs that would be effected by this >> change. > > In other words, it's yet another weird config option that breaks stuff. > It doesn't break anything because it is default n. People that enable this option will understand they are disabling the tiocsti ioctl for non privileged processes. > > NAK v7. Rather than a NAK, could you explain how you would solve this problem in a way that we can protect userspace from shooting itself in the foot. See CVE lookup: https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti Matt
On 5/29/2017 4:51 PM, Boris Lukashev wrote: > With all due respect sir, i believe your review falls short of the > purpose of this effort - to harden the kernel against flaws in > userspace. Comments along the line of "if <userspace> does it right > then your patch is pointless" are not relevant to the context of > securing kernel functions/interfaces. What userspace should do has > little bearing on defensive measures actually implemented in the > kernel - if we took the approach of "someone else is responsible for > that" in military operations, the world would be a much darker and > different place today. Those who have the luxury of standoff from the > critical impacts of security vulnerabilities may not take into account > the fact that peoples lives literally depend on Linux getting a lot > more secure, and quickly. You are not going to help anyone with a kernel configuration that breaks agetty, csh, xemacs and tcsh. The opportunities for using such a configuration are limited. > If this work were not valuable, it wouldnt be an enabled kernel option > on a massive number of kernels with attack surfaces reduced by the > compound protections offered by the grsec patch set. I'll bet you a beverage that 99-44/100% of the people who have this enabled have no clue that it's even there. And if they did, most of them would turn it off. > I can't speak for > the grsec people, but having read a small fraction of the commentary > around the subject of mainline integration, it seems to me that NAKs > like this are exactly why they had no interest in even trying - this > review is based on the cultural views of the kernel community, not on > the security benefits offered by the work in the current state of > affairs (where userspace is broken). A security clamp-down that breaks important stuff is going to have a tough row to hoe going upstream. Same with a performance enhancement that breaks things. > The purpose of each of these > protections (being ported over from grsec) is not to offer carte > blanche defense against all attackers and vectors, but to prevent > specific classes of bugs from reducing the security posture of the > system. By implementing these defenses in a layered manner we can > significantly reduce our kernel attack surface. Sure, but they have to work right. That's an important reason to do small changes. A change that isn't acceptable can be rejected without slowing the general progress. > Once userspace catches > up and does things the right way, and has no capacity for doing them > the wrong way (aka, nothing attackers can use to bypass the proper > userspace behavior), then the functionality really does become > pointless, and can then be removed. Well, until someone comes along with yet another spiffy feature like containers and breaks it again. This is why a really good solution is required, and the one proposed isn't up to snuff. > >From a practical perspective, can alternative solutions be offered > along with NAKs? They often are, but let's face it, not everyone has the time, desire and/or expertise to solve every problem that comes up. > Killing things on the vine isnt great, and if a > security measure is being denied, upstream should provide their > solution to how they want to address the problem (or just an outline > to guide the hardened folks). The impact of a "security measure" can exceed the value provided. That is, I understand, the basis of the NAK. We need to be careful to keep in mind that, until such time as there is substantial interest in the sort of systemic changes that truly remove this class of issue, we're going to have to justify the risk/reward trade off when we try to introduce a change. > > On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >> On Mon, 29 May 2017 17:38:00 -0400 >> Matt Brown <matt@nmatt.com> wrote: >> >>> This introduces the tiocsti_restrict sysctl, whose default is controlled >>> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >>> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. >> Which is really quite pointless as I keep pointing out and you keep >> reposting this nonsense. >> >>> This patch depends on patch 1/2 >>> >>> This patch was inspired from GRKERNSEC_HARDEN_TTY. >>> >>> This patch would have prevented >>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >>> conditions: >>> * non-privileged container >>> * container run inside new user namespace >> And assuming no other ioctl could be used in an attack. Only there are >> rather a lot of ways an app with access to a tty can cause mischief if >> it's the same controlling tty as the higher privileged context that >> launched it. >> >> Properly written code allocates a new pty/tty pair for the lower >> privileged session. If the code doesn't do that then your change merely >> modifies the degree of mayhem it can cause. If it does it right then your >> patch is pointless. >> >>> Possible effects on userland: >>> >>> There could be a few user programs that would be effected by this >>> change. >> In other words, it's yet another weird config option that breaks stuff. >> >> >> NAK v7. >> >> Alan > >
Casey Schaufler, First I must start this off by saying I really appreciate your presentation on LSMs that is up on youtube. I've got a LSM in the works and your talk has helped me a bunch. On 5/29/17 8:27 PM, Casey Schaufler wrote: > On 5/29/2017 4:51 PM, Boris Lukashev wrote: >> With all due respect sir, i believe your review falls short of the >> purpose of this effort - to harden the kernel against flaws in >> userspace. Comments along the line of "if <userspace> does it right >> then your patch is pointless" are not relevant to the context of >> securing kernel functions/interfaces. What userspace should do has >> little bearing on defensive measures actually implemented in the >> kernel - if we took the approach of "someone else is responsible for >> that" in military operations, the world would be a much darker and >> different place today. Those who have the luxury of standoff from the >> critical impacts of security vulnerabilities may not take into account >> the fact that peoples lives literally depend on Linux getting a lot >> more secure, and quickly. > > You are not going to help anyone with a kernel configuration that > breaks agetty, csh, xemacs and tcsh. The opportunities for using > such a configuration are limited. This patch does not break these programs as you imply. 99% of users of these programs will not be effected. Its not like the TIOCSTI ioctl is a critical part of these programs. Also as I've stated elsewhere, this is not breaking userspace because this Kconfig/sysctl defaults to n. If someone is using the programs listed above in a way that does utilize an unprivileged call to the TIOCSTI ioctl, they can turn this feature off. > >> If this work were not valuable, it wouldnt be an enabled kernel option >> on a massive number of kernels with attack surfaces reduced by the >> compound protections offered by the grsec patch set. > > I'll bet you a beverage that 99-44/100% of the people who have > this enabled have no clue that it's even there. And if they did, > most of them would turn it off. > First, I don't know how to parse "99-44/100%" and therefore do not wish to wager a beverage on such confusing odds ;) Second, as stated above, this feature is off by default. However, I would expect this sysctl to show up in lists of procedures for hardening linux servers. >> I can't speak for >> the grsec people, but having read a small fraction of the commentary >> around the subject of mainline integration, it seems to me that NAKs >> like this are exactly why they had no interest in even trying - this >> review is based on the cultural views of the kernel community, not on >> the security benefits offered by the work in the current state of >> affairs (where userspace is broken). > > A security clamp-down that breaks important stuff is going > to have a tough row to hoe going upstream. Same with a performance > enhancement that breaks things. > >> The purpose of each of these >> protections (being ported over from grsec) is not to offer carte >> blanche defense against all attackers and vectors, but to prevent >> specific classes of bugs from reducing the security posture of the >> system. By implementing these defenses in a layered manner we can >> significantly reduce our kernel attack surface. > > Sure, but they have to work right. That's an important reason to do > small changes. A change that isn't acceptable can be rejected without > slowing the general progress. > >> Once userspace catches >> up and does things the right way, and has no capacity for doing them >> the wrong way (aka, nothing attackers can use to bypass the proper >> userspace behavior), then the functionality really does become >> pointless, and can then be removed. > > Well, until someone comes along with yet another spiffy feature > like containers and breaks it again. This is why a really good > solution is required, and the one proposed isn't up to snuff. > Can you please state your reasons for why you believe this solution is not "up to snuff?" So far myself and others have given what I believe to be sound responses to any objections to this patch. >> >From a practical perspective, can alternative solutions be offered >> along with NAKs? > > They often are, but let's face it, not everyone has the time, > desire and/or expertise to solve every problem that comes up. > >> Killing things on the vine isnt great, and if a >> security measure is being denied, upstream should provide their >> solution to how they want to address the problem (or just an outline >> to guide the hardened folks). > > The impact of a "security measure" can exceed the value provided. > That is, I understand, the basis of the NAK. We need to be careful > to keep in mind that, until such time as there is substantial interest > in the sort of systemic changes that truly remove this class of issue, > we're going to have to justify the risk/reward trade off when we try > to introduce a change. > >> >> On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >>> On Mon, 29 May 2017 17:38:00 -0400 >>> Matt Brown <matt@nmatt.com> wrote: >>> >>>> This introduces the tiocsti_restrict sysctl, whose default is controlled >>>> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >>>> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. >>> Which is really quite pointless as I keep pointing out and you keep >>> reposting this nonsense. >>> >>>> This patch depends on patch 1/2 >>>> >>>> This patch was inspired from GRKERNSEC_HARDEN_TTY. >>>> >>>> This patch would have prevented >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >>>> conditions: >>>> * non-privileged container >>>> * container run inside new user namespace >>> And assuming no other ioctl could be used in an attack. Only there are >>> rather a lot of ways an app with access to a tty can cause mischief if >>> it's the same controlling tty as the higher privileged context that >>> launched it. >>> >>> Properly written code allocates a new pty/tty pair for the lower >>> privileged session. If the code doesn't do that then your change merely >>> modifies the degree of mayhem it can cause. If it does it right then your >>> patch is pointless. >>> >>>> Possible effects on userland: >>>> >>>> There could be a few user programs that would be effected by this >>>> change. >>> In other words, it's yet another weird config option that breaks stuff. >>> >>> >>> NAK v7. >>> >>> Alan >> >> > Thanks, Matt Brown
On 5/29/2017 7:00 PM, Matt Brown wrote: > Casey Schaufler, > > First I must start this off by saying I really appreciate your presentation on > LSMs that is up on youtube. I've got a LSM in the works and your talk has > helped me a bunch. Thank you. Feedback (especially positive) is always appreciated. > > On 5/29/17 8:27 PM, Casey Schaufler wrote: >> On 5/29/2017 4:51 PM, Boris Lukashev wrote: >>> With all due respect sir, i believe your review falls short of the >>> purpose of this effort - to harden the kernel against flaws in >>> userspace. Comments along the line of "if <userspace> does it right >>> then your patch is pointless" are not relevant to the context of >>> securing kernel functions/interfaces. What userspace should do has >>> little bearing on defensive measures actually implemented in the >>> kernel - if we took the approach of "someone else is responsible for >>> that" in military operations, the world would be a much darker and >>> different place today. Those who have the luxury of standoff from the >>> critical impacts of security vulnerabilities may not take into account >>> the fact that peoples lives literally depend on Linux getting a lot >>> more secure, and quickly. >> You are not going to help anyone with a kernel configuration that >> breaks agetty, csh, xemacs and tcsh. The opportunities for using >> such a configuration are limited. > This patch does not break these programs as you imply. 99% of users of these > programs will not be effected. Its not like the TIOCSTI ioctl is a critical > part of these programs. Most likely not. > > Also as I've stated elsewhere, this is not breaking userspace because this > Kconfig/sysctl defaults to n. If someone is using the programs listed above in > a way that does utilize an unprivileged call to the TIOCSTI ioctl, they can > turn this feature off. Default "off" does not mean it doesn't break userspace. It means that it might not break userspace in your environment. Or it might, depending on the whim of the build tool of the day. > >>> If this work were not valuable, it wouldnt be an enabled kernel option >>> on a massive number of kernels with attack surfaces reduced by the >>> compound protections offered by the grsec patch set. >> I'll bet you a beverage that 99-44/100% of the people who have >> this enabled have no clue that it's even there. And if they did, >> most of them would turn it off. >> > First, I don't know how to parse "99-44/100%" and therefore do not wish to > wager a beverage on such confusing odds ;) 99.44%. And I loose a *lot* of beverage bets. > Second, as stated above, this feature is off by default. However, I would expect > this sysctl to show up in lists of procedures for hardening linux servers. It's esoteric enough that I expect that if anyone got bitten by it word would get out and no one would use it thereafter. > >>> I can't speak for >>> the grsec people, but having read a small fraction of the commentary >>> around the subject of mainline integration, it seems to me that NAKs >>> like this are exactly why they had no interest in even trying - this >>> review is based on the cultural views of the kernel community, not on >>> the security benefits offered by the work in the current state of >>> affairs (where userspace is broken). >> A security clamp-down that breaks important stuff is going >> to have a tough row to hoe going upstream. Same with a performance >> enhancement that breaks things. >> >>> The purpose of each of these >>> protections (being ported over from grsec) is not to offer carte >>> blanche defense against all attackers and vectors, but to prevent >>> specific classes of bugs from reducing the security posture of the >>> system. By implementing these defenses in a layered manner we can >>> significantly reduce our kernel attack surface. >> Sure, but they have to work right. That's an important reason to do >> small changes. A change that isn't acceptable can be rejected without >> slowing the general progress. >> >>> Once userspace catches >>> up and does things the right way, and has no capacity for doing them >>> the wrong way (aka, nothing attackers can use to bypass the proper >>> userspace behavior), then the functionality really does become >>> pointless, and can then be removed. >> Well, until someone comes along with yet another spiffy feature >> like containers and breaks it again. This is why a really good >> solution is required, and the one proposed isn't up to snuff. >> > Can you please state your reasons for why you believe this solution is not "up > to snuff?" So far myself and others have given what I believe to be sound > responses to any objections to this patch. If you can't convince Alan, who know ways more about ttys than anyone ought to, it's not up to snuff. > >>> >From a practical perspective, can alternative solutions be offered >>> along with NAKs? >> They often are, but let's face it, not everyone has the time, >> desire and/or expertise to solve every problem that comes up. >> >>> Killing things on the vine isnt great, and if a >>> security measure is being denied, upstream should provide their >>> solution to how they want to address the problem (or just an outline >>> to guide the hardened folks). >> The impact of a "security measure" can exceed the value provided. >> That is, I understand, the basis of the NAK. We need to be careful >> to keep in mind that, until such time as there is substantial interest >> in the sort of systemic changes that truly remove this class of issue, >> we're going to have to justify the risk/reward trade off when we try >> to introduce a change. >> >>> On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >>>> On Mon, 29 May 2017 17:38:00 -0400 >>>> Matt Brown <matt@nmatt.com> wrote: >>>> >>>>> This introduces the tiocsti_restrict sysctl, whose default is controlled >>>>> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >>>>> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. >>>> Which is really quite pointless as I keep pointing out and you keep >>>> reposting this nonsense. >>>> >>>>> This patch depends on patch 1/2 >>>>> >>>>> This patch was inspired from GRKERNSEC_HARDEN_TTY. >>>>> >>>>> This patch would have prevented >>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >>>>> conditions: >>>>> * non-privileged container >>>>> * container run inside new user namespace >>>> And assuming no other ioctl could be used in an attack. Only there are >>>> rather a lot of ways an app with access to a tty can cause mischief if >>>> it's the same controlling tty as the higher privileged context that >>>> launched it. >>>> >>>> Properly written code allocates a new pty/tty pair for the lower >>>> privileged session. If the code doesn't do that then your change merely >>>> modifies the degree of mayhem it can cause. If it does it right then your >>>> patch is pointless. >>>> >>>>> Possible effects on userland: >>>>> >>>>> There could be a few user programs that would be effected by this >>>>> change. >>>> In other words, it's yet another weird config option that breaks stuff. >>>> >>>> >>>> NAK v7. >>>> >>>> Alan >>> > Thanks, > Matt Brown >
On 5/29/17 10:46 PM, Casey Schaufler wrote: > On 5/29/2017 7:00 PM, Matt Brown wrote: >> Casey Schaufler, >> >> First I must start this off by saying I really appreciate your presentation on >> LSMs that is up on youtube. I've got a LSM in the works and your talk has >> helped me a bunch. > > Thank you. Feedback (especially positive) is always appreciated. > >> >> On 5/29/17 8:27 PM, Casey Schaufler wrote: >>> On 5/29/2017 4:51 PM, Boris Lukashev wrote: >>>> With all due respect sir, i believe your review falls short of the >>>> purpose of this effort - to harden the kernel against flaws in >>>> userspace. Comments along the line of "if <userspace> does it right >>>> then your patch is pointless" are not relevant to the context of >>>> securing kernel functions/interfaces. What userspace should do has >>>> little bearing on defensive measures actually implemented in the >>>> kernel - if we took the approach of "someone else is responsible for >>>> that" in military operations, the world would be a much darker and >>>> different place today. Those who have the luxury of standoff from the >>>> critical impacts of security vulnerabilities may not take into account >>>> the fact that peoples lives literally depend on Linux getting a lot >>>> more secure, and quickly. >>> You are not going to help anyone with a kernel configuration that >>> breaks agetty, csh, xemacs and tcsh. The opportunities for using >>> such a configuration are limited. >> This patch does not break these programs as you imply. 99% of users of these >> programs will not be effected. Its not like the TIOCSTI ioctl is a critical >> part of these programs. > > Most likely not. > >> >> Also as I've stated elsewhere, this is not breaking userspace because this >> Kconfig/sysctl defaults to n. If someone is using the programs listed above in >> a way that does utilize an unprivileged call to the TIOCSTI ioctl, they can >> turn this feature off. > > Default "off" does not mean it doesn't break userspace. It means that it might > not break userspace in your environment. Or it might, depending on the whim of > the build tool of the day. > By this logic, it seems like any introduced feature which toggles some security feature could be seen as "breaking userspace." For example: 1. Let there exist a LSM X that is set to "off" by default. (let's say its a simpler type of MAC ;) 2. There exists an inexperienced user Bob that toggles X to "on". 3. Bob complains that X has broken userspace because he now cannot access his SSH key from firefox. build tool's will always have important impacts on a system based on the config that is used. My understanding of the "don't break userspace" rule has always been to not change existing, userspace facing, APIs/ioctls/system calls/etc. I don't believe that my patch does this. >> >>>> If this work were not valuable, it wouldnt be an enabled kernel option >>>> on a massive number of kernels with attack surfaces reduced by the >>>> compound protections offered by the grsec patch set. >>> I'll bet you a beverage that 99-44/100% of the people who have >>> this enabled have no clue that it's even there. And if they did, >>> most of them would turn it off. >>> >> First, I don't know how to parse "99-44/100%" and therefore do not wish to >> wager a beverage on such confusing odds ;) > > 99.44%. And I loose a *lot* of beverage bets. > >> Second, as stated above, this feature is off by default. However, I would expect >> this sysctl to show up in lists of procedures for hardening linux servers. > > It's esoteric enough that I expect that if anyone got bitten by it > word would get out and no one would use it thereafter. > As we know in the security world, esoteric things can have major a impact. I have not looked thought all of these, but I imagine most of them could have been prevented by this patch. https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti >> >>>> I can't speak for >>>> the grsec people, but having read a small fraction of the commentary >>>> around the subject of mainline integration, it seems to me that NAKs >>>> like this are exactly why they had no interest in even trying - this >>>> review is based on the cultural views of the kernel community, not on >>>> the security benefits offered by the work in the current state of >>>> affairs (where userspace is broken). >>> A security clamp-down that breaks important stuff is going >>> to have a tough row to hoe going upstream. Same with a performance >>> enhancement that breaks things. >>> >>>> The purpose of each of these >>>> protections (being ported over from grsec) is not to offer carte >>>> blanche defense against all attackers and vectors, but to prevent >>>> specific classes of bugs from reducing the security posture of the >>>> system. By implementing these defenses in a layered manner we can >>>> significantly reduce our kernel attack surface. >>> Sure, but they have to work right. That's an important reason to do >>> small changes. A change that isn't acceptable can be rejected without >>> slowing the general progress. >>> >>>> Once userspace catches >>>> up and does things the right way, and has no capacity for doing them >>>> the wrong way (aka, nothing attackers can use to bypass the proper >>>> userspace behavior), then the functionality really does become >>>> pointless, and can then be removed. >>> Well, until someone comes along with yet another spiffy feature >>> like containers and breaks it again. This is why a really good >>> solution is required, and the one proposed isn't up to snuff. >>> >> Can you please state your reasons for why you believe this solution is not "up >> to snuff?" So far myself and others have given what I believe to be sound >> responses to any objections to this patch. > > If you can't convince Alan, who know ways more about ttys than anyone > ought to, it's not up to snuff. > >> >>>> >From a practical perspective, can alternative solutions be offered >>>> along with NAKs? >>> They often are, but let's face it, not everyone has the time, >>> desire and/or expertise to solve every problem that comes up. >>> >>>> Killing things on the vine isnt great, and if a >>>> security measure is being denied, upstream should provide their >>>> solution to how they want to address the problem (or just an outline >>>> to guide the hardened folks). >>> The impact of a "security measure" can exceed the value provided. >>> That is, I understand, the basis of the NAK. We need to be careful >>> to keep in mind that, until such time as there is substantial interest >>> in the sort of systemic changes that truly remove this class of issue, >>> we're going to have to justify the risk/reward trade off when we try >>> to introduce a change. >>> >>>> On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >>>>> On Mon, 29 May 2017 17:38:00 -0400 >>>>> Matt Brown <matt@nmatt.com> wrote: >>>>> >>>>>> This introduces the tiocsti_restrict sysctl, whose default is controlled >>>>>> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >>>>>> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. >>>>> Which is really quite pointless as I keep pointing out and you keep >>>>> reposting this nonsense. >>>>> >>>>>> This patch depends on patch 1/2 >>>>>> >>>>>> This patch was inspired from GRKERNSEC_HARDEN_TTY. >>>>>> >>>>>> This patch would have prevented >>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >>>>>> conditions: >>>>>> * non-privileged container >>>>>> * container run inside new user namespace >>>>> And assuming no other ioctl could be used in an attack. Only there are >>>>> rather a lot of ways an app with access to a tty can cause mischief if >>>>> it's the same controlling tty as the higher privileged context that >>>>> launched it. >>>>> >>>>> Properly written code allocates a new pty/tty pair for the lower >>>>> privileged session. If the code doesn't do that then your change merely >>>>> modifies the degree of mayhem it can cause. If it does it right then your >>>>> patch is pointless. >>>>> >>>>>> Possible effects on userland: >>>>>> >>>>>> There could be a few user programs that would be effected by this >>>>>> change. >>>>> In other words, it's yet another weird config option that breaks stuff. >>>>> >>>>> >>>>> NAK v7. >>>>> >>>>> Alan >>>> >> Thanks, >> Matt Brown >> >
Look there are two problems here 1. TIOCSTI has users 2. You don't actually fix anything The underlying problem is that if you give your tty handle to another process which you don't trust you are screwed. It's fundamental to the design of the Unix tty model and it's made worse in Linux by the fact that we use the tty descriptor to access all sorts of other console state (which makes a ton of sense). Many years ago a few people got this wrong. All those apps got fixes back then. They allocate a tty/pty pair and create a new session over that. The potentially hostile other app only gets to screw itself. If it was only about TIOCSTI then your patch would still not make sense because you could use on of the existing LSMs to actually write yourself some rules about who can and can't use TIOCSTI. For that matter you can even use the seccomp feature today to do this without touching your kernel because the ioctl number is a value so you can just block ioctl with argument 2 of TIOCSTI. So please explain why we need an obscure kernel config option that normal users will not understand which protects against nothing and can be done already ? Alan
On 5/29/2017 8:18 PM, Matt Brown wrote: > On 5/29/17 10:46 PM, Casey Schaufler wrote: >> On 5/29/2017 7:00 PM, Matt Brown wrote: >>> Casey Schaufler, >>> >>> First I must start this off by saying I really appreciate your presentation on >>> LSMs that is up on youtube. I've got a LSM in the works and your talk has >>> helped me a bunch. >> Thank you. Feedback (especially positive) is always appreciated. >> >>> On 5/29/17 8:27 PM, Casey Schaufler wrote: >>>> On 5/29/2017 4:51 PM, Boris Lukashev wrote: >>>>> With all due respect sir, i believe your review falls short of the >>>>> purpose of this effort - to harden the kernel against flaws in >>>>> userspace. Comments along the line of "if <userspace> does it right >>>>> then your patch is pointless" are not relevant to the context of >>>>> securing kernel functions/interfaces. What userspace should do has >>>>> little bearing on defensive measures actually implemented in the >>>>> kernel - if we took the approach of "someone else is responsible for >>>>> that" in military operations, the world would be a much darker and >>>>> different place today. Those who have the luxury of standoff from the >>>>> critical impacts of security vulnerabilities may not take into account >>>>> the fact that peoples lives literally depend on Linux getting a lot >>>>> more secure, and quickly. >>>> You are not going to help anyone with a kernel configuration that >>>> breaks agetty, csh, xemacs and tcsh. The opportunities for using >>>> such a configuration are limited. >>> This patch does not break these programs as you imply. 99% of users of these >>> programs will not be effected. Its not like the TIOCSTI ioctl is a critical >>> part of these programs. >> Most likely not. >> >>> Also as I've stated elsewhere, this is not breaking userspace because this >>> Kconfig/sysctl defaults to n. If someone is using the programs listed above in >>> a way that does utilize an unprivileged call to the TIOCSTI ioctl, they can >>> turn this feature off. >> Default "off" does not mean it doesn't break userspace. It means that it might >> not break userspace in your environment. Or it might, depending on the whim of >> the build tool of the day. >> > By this logic, it seems like any introduced feature which toggles some security > feature could be seen as "breaking userspace." For example: > > 1. Let there exist a LSM X that is set to "off" by default. > (let's say its a simpler type of MAC ;) You picked a bad example ... > 2. There exists an inexperienced user Bob that toggles X to "on". ... which is easy enough, however ... > 3. Bob complains that X has broken userspace because he now cannot access his > SSH key from firefox. ... that's not going to happen. Because the LSM you're referring to requires additional configuration to "break" userspace. That, by the way, was a conscious design choice. Now, I realize that's not the case for some other security modules, but they have things like "permissive mode" to make it easy on accident prone Bob. The analogy, while obvious, is invalid. > build tool's will always have important impacts on a system based on the config > that is used. > > My understanding of the "don't break userspace" rule has always been to not > change existing, userspace facing, APIs/ioctls/system calls/etc. I don't > believe that my patch does this. > >>>>> If this work were not valuable, it wouldnt be an enabled kernel option >>>>> on a massive number of kernels with attack surfaces reduced by the >>>>> compound protections offered by the grsec patch set. >>>> I'll bet you a beverage that 99-44/100% of the people who have >>>> this enabled have no clue that it's even there. And if they did, >>>> most of them would turn it off. >>>> >>> First, I don't know how to parse "99-44/100%" and therefore do not wish to >>> wager a beverage on such confusing odds ;) >> 99.44%. And I loose a *lot* of beverage bets. >> >>> Second, as stated above, this feature is off by default. However, I would expect >>> this sysctl to show up in lists of procedures for hardening linux servers. >> It's esoteric enough that I expect that if anyone got bitten by it >> word would get out and no one would use it thereafter. >> > As we know in the security world, esoteric things can have major a impact. I > have not looked thought all of these, but I imagine most of them could have > been prevented by this patch. > > https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti > >>>>> I can't speak for >>>>> the grsec people, but having read a small fraction of the commentary >>>>> around the subject of mainline integration, it seems to me that NAKs >>>>> like this are exactly why they had no interest in even trying - this >>>>> review is based on the cultural views of the kernel community, not on >>>>> the security benefits offered by the work in the current state of >>>>> affairs (where userspace is broken). >>>> A security clamp-down that breaks important stuff is going >>>> to have a tough row to hoe going upstream. Same with a performance >>>> enhancement that breaks things. >>>> >>>>> The purpose of each of these >>>>> protections (being ported over from grsec) is not to offer carte >>>>> blanche defense against all attackers and vectors, but to prevent >>>>> specific classes of bugs from reducing the security posture of the >>>>> system. By implementing these defenses in a layered manner we can >>>>> significantly reduce our kernel attack surface. >>>> Sure, but they have to work right. That's an important reason to do >>>> small changes. A change that isn't acceptable can be rejected without >>>> slowing the general progress. >>>> >>>>> Once userspace catches >>>>> up and does things the right way, and has no capacity for doing them >>>>> the wrong way (aka, nothing attackers can use to bypass the proper >>>>> userspace behavior), then the functionality really does become >>>>> pointless, and can then be removed. >>>> Well, until someone comes along with yet another spiffy feature >>>> like containers and breaks it again. This is why a really good >>>> solution is required, and the one proposed isn't up to snuff. >>>> >>> Can you please state your reasons for why you believe this solution is not "up >>> to snuff?" So far myself and others have given what I believe to be sound >>> responses to any objections to this patch. >> If you can't convince Alan, who know ways more about ttys than anyone >> ought to, it's not up to snuff. >> >>>>> >From a practical perspective, can alternative solutions be offered >>>>> along with NAKs? >>>> They often are, but let's face it, not everyone has the time, >>>> desire and/or expertise to solve every problem that comes up. >>>> >>>>> Killing things on the vine isnt great, and if a >>>>> security measure is being denied, upstream should provide their >>>>> solution to how they want to address the problem (or just an outline >>>>> to guide the hardened folks). >>>> The impact of a "security measure" can exceed the value provided. >>>> That is, I understand, the basis of the NAK. We need to be careful >>>> to keep in mind that, until such time as there is substantial interest >>>> in the sort of systemic changes that truly remove this class of issue, >>>> we're going to have to justify the risk/reward trade off when we try >>>> to introduce a change. >>>> >>>>> On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >>>>>> On Mon, 29 May 2017 17:38:00 -0400 >>>>>> Matt Brown <matt@nmatt.com> wrote: >>>>>> >>>>>>> This introduces the tiocsti_restrict sysctl, whose default is controlled >>>>>>> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >>>>>>> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. >>>>>> Which is really quite pointless as I keep pointing out and you keep >>>>>> reposting this nonsense. >>>>>> >>>>>>> This patch depends on patch 1/2 >>>>>>> >>>>>>> This patch was inspired from GRKERNSEC_HARDEN_TTY. >>>>>>> >>>>>>> This patch would have prevented >>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >>>>>>> conditions: >>>>>>> * non-privileged container >>>>>>> * container run inside new user namespace >>>>>> And assuming no other ioctl could be used in an attack. Only there are >>>>>> rather a lot of ways an app with access to a tty can cause mischief if >>>>>> it's the same controlling tty as the higher privileged context that >>>>>> launched it. >>>>>> >>>>>> Properly written code allocates a new pty/tty pair for the lower >>>>>> privileged session. If the code doesn't do that then your change merely >>>>>> modifies the degree of mayhem it can cause. If it does it right then your >>>>>> patch is pointless. >>>>>> >>>>>>> Possible effects on userland: >>>>>>> >>>>>>> There could be a few user programs that would be effected by this >>>>>>> change. >>>>>> In other words, it's yet another weird config option that breaks stuff. >>>>>> >>>>>> >>>>>> NAK v7. >>>>>> >>>>>> Alan >>> Thanks, >>> Matt Brown >>>
On 5/30/17 11:20 AM, Casey Schaufler wrote: > On 5/29/2017 8:18 PM, Matt Brown wrote: >> On 5/29/17 10:46 PM, Casey Schaufler wrote: >>> On 5/29/2017 7:00 PM, Matt Brown wrote: >>>> Casey Schaufler, >>>> >>>> First I must start this off by saying I really appreciate your presentation on >>>> LSMs that is up on youtube. I've got a LSM in the works and your talk has >>>> helped me a bunch. >>> Thank you. Feedback (especially positive) is always appreciated. >>> >>>> On 5/29/17 8:27 PM, Casey Schaufler wrote: >>>>> On 5/29/2017 4:51 PM, Boris Lukashev wrote: >>>>>> With all due respect sir, i believe your review falls short of the >>>>>> purpose of this effort - to harden the kernel against flaws in >>>>>> userspace. Comments along the line of "if <userspace> does it right >>>>>> then your patch is pointless" are not relevant to the context of >>>>>> securing kernel functions/interfaces. What userspace should do has >>>>>> little bearing on defensive measures actually implemented in the >>>>>> kernel - if we took the approach of "someone else is responsible for >>>>>> that" in military operations, the world would be a much darker and >>>>>> different place today. Those who have the luxury of standoff from the >>>>>> critical impacts of security vulnerabilities may not take into account >>>>>> the fact that peoples lives literally depend on Linux getting a lot >>>>>> more secure, and quickly. >>>>> You are not going to help anyone with a kernel configuration that >>>>> breaks agetty, csh, xemacs and tcsh. The opportunities for using >>>>> such a configuration are limited. >>>> This patch does not break these programs as you imply. 99% of users of these >>>> programs will not be effected. Its not like the TIOCSTI ioctl is a critical >>>> part of these programs. >>> Most likely not. >>> >>>> Also as I've stated elsewhere, this is not breaking userspace because this >>>> Kconfig/sysctl defaults to n. If someone is using the programs listed above in >>>> a way that does utilize an unprivileged call to the TIOCSTI ioctl, they can >>>> turn this feature off. >>> Default "off" does not mean it doesn't break userspace. It means that it might >>> not break userspace in your environment. Or it might, depending on the whim of >>> the build tool of the day. >>> >> By this logic, it seems like any introduced feature which toggles some security >> feature could be seen as "breaking userspace." For example: >> >> 1. Let there exist a LSM X that is set to "off" by default. >> (let's say its a simpler type of MAC ;) > > You picked a bad example ... > >> 2. There exists an inexperienced user Bob that toggles X to "on". > > ... which is easy enough, however ... > >> 3. Bob complains that X has broken userspace because he now cannot access his >> SSH key from firefox. > > ... that's not going to happen. Because the LSM you're referring to requires > additional configuration to "break" userspace. That, by the way, was a conscious > design choice. Now, I realize that's not the case for some other security modules, > but they have things like "permissive mode" to make it easy on accident prone Bob. > > The analogy, while obvious, is invalid. So I may have been a bit sarcastic with my analogy, but my point is that it seems like your definition of "breaking userspace" is too broad and could lead to things getting labeled as breaking userspace that clearly are not. > >> build tool's will always have important impacts on a system based on the config >> that is used. >> >> My understanding of the "don't break userspace" rule has always been to not >> change existing, userspace facing, APIs/ioctls/system calls/etc. I don't >> believe that my patch does this. >> >>>>>> If this work were not valuable, it wouldnt be an enabled kernel option >>>>>> on a massive number of kernels with attack surfaces reduced by the >>>>>> compound protections offered by the grsec patch set. >>>>> I'll bet you a beverage that 99-44/100% of the people who have >>>>> this enabled have no clue that it's even there. And if they did, >>>>> most of them would turn it off. >>>>> >>>> First, I don't know how to parse "99-44/100%" and therefore do not wish to >>>> wager a beverage on such confusing odds ;) >>> 99.44%. And I loose a *lot* of beverage bets. >>> >>>> Second, as stated above, this feature is off by default. However, I would expect >>>> this sysctl to show up in lists of procedures for hardening linux servers. >>> It's esoteric enough that I expect that if anyone got bitten by it >>> word would get out and no one would use it thereafter. >>> >> As we know in the security world, esoteric things can have major a impact. I >> have not looked thought all of these, but I imagine most of them could have >> been prevented by this patch. >> >> https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti >> >>>>>> I can't speak for >>>>>> the grsec people, but having read a small fraction of the commentary >>>>>> around the subject of mainline integration, it seems to me that NAKs >>>>>> like this are exactly why they had no interest in even trying - this >>>>>> review is based on the cultural views of the kernel community, not on >>>>>> the security benefits offered by the work in the current state of >>>>>> affairs (where userspace is broken). >>>>> A security clamp-down that breaks important stuff is going >>>>> to have a tough row to hoe going upstream. Same with a performance >>>>> enhancement that breaks things. >>>>> >>>>>> The purpose of each of these >>>>>> protections (being ported over from grsec) is not to offer carte >>>>>> blanche defense against all attackers and vectors, but to prevent >>>>>> specific classes of bugs from reducing the security posture of the >>>>>> system. By implementing these defenses in a layered manner we can >>>>>> significantly reduce our kernel attack surface. >>>>> Sure, but they have to work right. That's an important reason to do >>>>> small changes. A change that isn't acceptable can be rejected without >>>>> slowing the general progress. >>>>> >>>>>> Once userspace catches >>>>>> up and does things the right way, and has no capacity for doing them >>>>>> the wrong way (aka, nothing attackers can use to bypass the proper >>>>>> userspace behavior), then the functionality really does become >>>>>> pointless, and can then be removed. >>>>> Well, until someone comes along with yet another spiffy feature >>>>> like containers and breaks it again. This is why a really good >>>>> solution is required, and the one proposed isn't up to snuff. >>>>> >>>> Can you please state your reasons for why you believe this solution is not "up >>>> to snuff?" So far myself and others have given what I believe to be sound >>>> responses to any objections to this patch. >>> If you can't convince Alan, who know ways more about ttys than anyone >>> ought to, it's not up to snuff. >>> >>>>>> >From a practical perspective, can alternative solutions be offered >>>>>> along with NAKs? >>>>> They often are, but let's face it, not everyone has the time, >>>>> desire and/or expertise to solve every problem that comes up. >>>>> >>>>>> Killing things on the vine isnt great, and if a >>>>>> security measure is being denied, upstream should provide their >>>>>> solution to how they want to address the problem (or just an outline >>>>>> to guide the hardened folks). >>>>> The impact of a "security measure" can exceed the value provided. >>>>> That is, I understand, the basis of the NAK. We need to be careful >>>>> to keep in mind that, until such time as there is substantial interest >>>>> in the sort of systemic changes that truly remove this class of issue, >>>>> we're going to have to justify the risk/reward trade off when we try >>>>> to introduce a change. >>>>> >>>>>> On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >>>>>>> On Mon, 29 May 2017 17:38:00 -0400 >>>>>>> Matt Brown <matt@nmatt.com> wrote: >>>>>>> >>>>>>>> This introduces the tiocsti_restrict sysctl, whose default is controlled >>>>>>>> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >>>>>>>> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. >>>>>>> Which is really quite pointless as I keep pointing out and you keep >>>>>>> reposting this nonsense. >>>>>>> >>>>>>>> This patch depends on patch 1/2 >>>>>>>> >>>>>>>> This patch was inspired from GRKERNSEC_HARDEN_TTY. >>>>>>>> >>>>>>>> This patch would have prevented >>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >>>>>>>> conditions: >>>>>>>> * non-privileged container >>>>>>>> * container run inside new user namespace >>>>>>> And assuming no other ioctl could be used in an attack. Only there are >>>>>>> rather a lot of ways an app with access to a tty can cause mischief if >>>>>>> it's the same controlling tty as the higher privileged context that >>>>>>> launched it. >>>>>>> >>>>>>> Properly written code allocates a new pty/tty pair for the lower >>>>>>> privileged session. If the code doesn't do that then your change merely >>>>>>> modifies the degree of mayhem it can cause. If it does it right then your >>>>>>> patch is pointless. >>>>>>> >>>>>>>> Possible effects on userland: >>>>>>>> >>>>>>>> There could be a few user programs that would be effected by this >>>>>>>> change. >>>>>>> In other words, it's yet another weird config option that breaks stuff. >>>>>>> >>>>>>> >>>>>>> NAK v7. >>>>>>> >>>>>>> Alan >>>> Thanks, >>>> Matt Brown >>>> >
On 5/30/17 8:24 AM, Alan Cox wrote: > Look there are two problems here > > 1. TIOCSTI has users I don't see how this is a problem. > > 2. You don't actually fix anything > > The underlying problem is that if you give your tty handle to another > process which you don't trust you are screwed. It's fundamental to the > design of the Unix tty model and it's made worse in Linux by the fact > that we use the tty descriptor to access all sorts of other console state > (which makes a ton of sense). > > Many years ago a few people got this wrong. All those apps got fixes back > then. They allocate a tty/pty pair and create a new session over that. > The potentially hostile other app only gets to screw itself. > Many years ago? We already got one in 2017, as well as a bunch last year. See: https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti > If it was only about TIOCSTI then your patch would still not make sense > because you could use on of the existing LSMs to actually write yourself > some rules about who can and can't use TIOCSTI. For that matter you can > even use the seccomp feature today to do this without touching your > kernel because the ioctl number is a value so you can just block ioctl > with argument 2 of TIOCSTI. > Seccomp requires the program in question to "opt-in" so to speak and set certain restrictions on itself. However as you state above, any TIOCSTI protection doesn't matter if the program correctly allocates a tty/pty pair. This protections seeks to protect users from programs that don't do things correctly. Rather than killing bugs, this feature attempts to kill an entire bug class that shows little sign of slowing down in the world of containers and sandboxes. > So please explain why we need an obscure kernel config option that normal > users will not understand which protects against nothing and can be > done already ? > > Alan >
> Seccomp requires the program in question to "opt-in" so to speak and set > certain restrictions on itself. However as you state above, any TIOCSTI > protection doesn't matter if the program correctly allocates a tty/pty pair. > This protections seeks to protect users from programs that don't do things > correctly. Rather than killing bugs, this feature attempts to kill an entire > bug class that shows little sign of slowing down in the world of containers and > sandboxes. It's possible to do it in PID1 as root without NO_NEW_PRIVS set, but there isn't an existing implementation of that. It's not included in init systems like systemd. There's no way to toggle that off at runtime one that's done like this sysctl though. If a system administrator wants to enable it, they'll need to modify a configuration file and reboot if it was even supported by the init system. It's the same argument that was used against perf_event_paranoid=3. Meanwhile, perf_event_paranoid=3 is a mandatory requirement for every Android device and toggling it at runtime is *necessary* since that's exposed as a system property writable by the Android Debug Bridge shell user (i.e. physical access via USB + ADB enabled within the OS + ADB key of the ADB client accepted). There's less use case for TIOCSTI so toggling it on at runtime isn't as important, but a toggle like this is a LOT friendlier than a seccomp blacklist even if that was supported by common init systems, and it's not.
On Tue, 2017-05-30 at 12:28 -0400, Matt Brown wrote: > On 5/30/17 8:24 AM, Alan Cox wrote: > > Look there are two problems here > > > > 1. TIOCSTI has users > > I don't see how this is a problem. > > > > > 2. You don't actually fix anything > > > > The underlying problem is that if you give your tty handle to > > another > > process which you don't trust you are screwed. It's fundamental to > > the > > design of the Unix tty model and it's made worse in Linux by the > > fact > > that we use the tty descriptor to access all sorts of other console > > state > > (which makes a ton of sense). > > > > Many years ago a few people got this wrong. All those apps got > > fixes back > > then. They allocate a tty/pty pair and create a new session over > > that. > > The potentially hostile other app only gets to screw itself. > > > > Many years ago? We already got one in 2017, as well as a bunch last > year. > See: https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti > > > If it was only about TIOCSTI then your patch would still not make > > sense > > because you could use on of the existing LSMs to actually write > > yourself > > some rules about who can and can't use TIOCSTI. For that matter you > > can > > even use the seccomp feature today to do this without touching your > > kernel because the ioctl number is a value so you can just block > > ioctl > > with argument 2 of TIOCSTI. > > > > Seccomp requires the program in question to "opt-in" so to speak and > set > certain restrictions on itself. However as you state above, any > TIOCSTI > protection doesn't matter if the program correctly allocates a > tty/pty pair. > This protections seeks to protect users from programs that don't do > things > correctly. Rather than killing bugs, this feature attempts to kill an > entire > bug class that shows little sign of slowing down in the world of > containers and > sandboxes. Just FYI, you can also restrict TIOCSTI (or any other ioctl command) via SELinux ioctl whitelisting, and Android is using that feature to restrict TIOCSTI usage in Android O (at least based on the developer previews to date, also in AOSP master). > > > So please explain why we need an obscure kernel config option that > > normal > > users will not understand which protects against nothing and can be > > done already ? > > > > Alan > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux- > security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at  http://vger.kernel.org/majordomo-info.html
On Tue, May 30, 2017 at 11:32 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> Seccomp requires the program in question to "opt-in" so to speak and >> set >> certain restrictions on itself. However as you state above, any >> TIOCSTI >> protection doesn't matter if the program correctly allocates a >> tty/pty pair. >> This protections seeks to protect users from programs that don't do >> things >> correctly. Rather than killing bugs, this feature attempts to kill an >> entire >> bug class that shows little sign of slowing down in the world of >> containers and >> sandboxes. > > Just FYI, you can also restrict TIOCSTI (or any other ioctl command) > via SELinux ioctl whitelisting, and Android is using that feature to > restrict TIOCSTI usage in Android O (at least based on the developer > previews to date, also in AOSP master). For reference, this is https://android-review.googlesource.com/306278 , where we moved to a whitelist for handling ioctls for ptys. -- Nick
On 5/30/17 2:44 PM, Nick Kralevich wrote: > On Tue, May 30, 2017 at 11:32 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>> Seccomp requires the program in question to "opt-in" so to speak and >>> set >>> certain restrictions on itself. However as you state above, any >>> TIOCSTI >>> protection doesn't matter if the program correctly allocates a >>> tty/pty pair. >>> This protections seeks to protect users from programs that don't do >>> things >>> correctly. Rather than killing bugs, this feature attempts to kill an >>> entire >>> bug class that shows little sign of slowing down in the world of >>> containers and >>> sandboxes. >> >> Just FYI, you can also restrict TIOCSTI (or any other ioctl command) >> via SELinux ioctl whitelisting, and Android is using that feature to >> restrict TIOCSTI usage in Android O (at least based on the developer >> previews to date, also in AOSP master). > > For reference, this is https://android-review.googlesource.com/306278 > , where we moved to a whitelist for handling ioctls for ptys. > > -- Nick > Thanks, I didn't know that android was doing this. I still think this feature is worthwhile for people to be able to harden their systems against this attack vector without having to implement a MAC. Matt
> Thanks, I didn't know that android was doing this. I still think this > feature > is worthwhile for people to be able to harden their systems against > this attack > vector without having to implement a MAC. Since there's a capable LSM hook for ioctl already, it means it could go in Yama with ptrace_scope but core kernel code would still need to be changed to track the owning tty. I think Yama vs. core kernel shouldn't matter much anymore due to stackable LSMs. Not the case for perf_event_paranoid=3 where a) there's already a sysctl exposed which would be unfortunate to duplicate, b) there isn't an LSM hook yet (AFAIK). The toggles for ptrace and perf events are more useful though since they're very commonly used debugging features vs. this obscure, rarely used ioctl that in practice no one will notice is missing. It's still friendlier to have a toggle than a seccomp policy requiring a reboot to get rid of it, or worse compiling it out of the kernel.
On Tue, 30 May 2017 12:28:59 -0400 Matt Brown <matt@nmatt.com> wrote: > On 5/30/17 8:24 AM, Alan Cox wrote: > > Look there are two problems here > > > > 1. TIOCSTI has users > > I don't see how this is a problem. Which is unfortunate. To start with if it didn't have users we could just delete it. > > > > 2. You don't actually fix anything > > > > The underlying problem is that if you give your tty handle to another > > process which you don't trust you are screwed. It's fundamental to the > > design of the Unix tty model and it's made worse in Linux by the fact > > that we use the tty descriptor to access all sorts of other console state > > (which makes a ton of sense). > > > > Many years ago a few people got this wrong. All those apps got fixes back > > then. They allocate a tty/pty pair and create a new session over that. > > The potentially hostile other app only gets to screw itself. > > > > Many years ago? We already got one in 2017, as well as a bunch last year. > See: https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti All the apps got fixed at the time. The fact the next generation of forgot to learn from it is unfortunate but hardly new. Also every single one of those that exposes a tty in that way allows other annoying behaviours via other ioctl interfaces so none of them would have been properly mitigated. If you really want to do that particular bit of snake oiling then you can use the existing SELinux, seccomp and related interfaces. They can even do the job properly by whitelisting or blocking long lists of ioctls. > This protections seeks to protect users from programs that don't do things > correctly. Rather than killing bugs, this feature attempts to kill an entire > bug class that shows little sign of slowing down in the world of containers and > sandboxes. Well maybe the people writing them need to learn what they are doing and stop passing random file descriptors into their container (I've even seen people handing X file handles into their 'container'). The kernel can do some things to help programmers but it can't stop people writing crap. Anyone writing code that crosses security boundaries should have at least a vague idea of what they are doing. The only way you'd actually really prevent this would be to magically open a new pty/tty pair and substitute the file handlers plus a data copying thread when someone created a namespace. Now you can actually do that with the ptrace functionality in seccomp but it would still be fairly insane to expect the kernel to handle. Alan [Actually even more sensible would be to revert the entire sorry container mess and use VMs but it's a bit late for that ;-)]
On 5/30/17 4:22 PM, Daniel Micay wrote: >> Thanks, I didn't know that android was doing this. I still think this >> feature >> is worthwhile for people to be able to harden their systems against >> this attack >> vector without having to implement a MAC. > > Since there's a capable LSM hook for ioctl already, it means it could go > in Yama with ptrace_scope but core kernel code would still need to be > changed to track the owning tty. I think Yama vs. core kernel shouldn't > matter much anymore due to stackable LSMs. > What does everyone think about a v8 that moves this feature under Yama and uses the file_ioctl LSM hook? > Not the case for perf_event_paranoid=3 where a) there's already a sysctl > exposed which would be unfortunate to duplicate, b) there isn't an LSM > hook yet (AFAIK). > > The toggles for ptrace and perf events are more useful though since > they're very commonly used debugging features vs. this obscure, rarely > used ioctl that in practice no one will notice is missing. It's still > friendlier to have a toggle than a seccomp policy requiring a reboot to > get rid of it, or worse compiling it out of the kernel. >
On 5/30/17 6:51 PM, Alan Cox wrote: > On Tue, 30 May 2017 12:28:59 -0400 > Matt Brown <matt@nmatt.com> wrote: > >> On 5/30/17 8:24 AM, Alan Cox wrote: >>> Look there are two problems here >>> >>> 1. TIOCSTI has users >> >> I don't see how this is a problem. > > Which is unfortunate. To start with if it didn't have users we could just > delete it. > >>> >>> 2. You don't actually fix anything >>> >>> The underlying problem is that if you give your tty handle to another >>> process which you don't trust you are screwed. It's fundamental to the >>> design of the Unix tty model and it's made worse in Linux by the fact >>> that we use the tty descriptor to access all sorts of other console state >>> (which makes a ton of sense). >>> >>> Many years ago a few people got this wrong. All those apps got fixes back >>> then. They allocate a tty/pty pair and create a new session over that. >>> The potentially hostile other app only gets to screw itself. >>> >> >> Many years ago? We already got one in 2017, as well as a bunch last year. >> See: https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=tiocsti > > All the apps got fixed at the time. The fact the next generation of > forgot to learn from it is unfortunate but hardly new. Also every single > one of those that exposes a tty in that way allows other annoying > behaviours via other ioctl interfaces so none of them would have been > properly mitigated. > This is my point. Apps will continue to shoot themselves in the foot. Of course the correct response to one of these vulns is to not pass ttys across a security boundary. We have an opportunity here to reduce the impact of this bug class at the kernel level. Rejecting this mitigation because the real solution is to use a tty/pty pair is like saying we should reject ASLR because the real solution to buffer overflows is proper bounds checking. > If you really want to do that particular bit of snake oiling then you can > use the existing SELinux, seccomp and related interfaces. They can even > do the job properly by whitelisting or blocking long lists of ioctls. > >> This protections seeks to protect users from programs that don't do things >> correctly. Rather than killing bugs, this feature attempts to kill an entire >> bug class that shows little sign of slowing down in the world of containers and >> sandboxes. > > Well maybe the people writing them need to learn what they are doing and > stop passing random file descriptors into their container (I've even seen > people handing X file handles into their 'container'). > > The kernel can do some things to help programmers but it can't stop > people writing crap. Anyone writing code that crosses security boundaries > should have at least a vague idea of what they are doing. > > The only way you'd actually really prevent this would be to magically > open a new pty/tty pair and substitute the file handlers plus a data > copying thread when someone created a namespace. > > Now you can actually do that with the ptrace functionality in seccomp but > it would still be fairly insane to expect the kernel to handle. > > Alan > [Actually even more sensible would be to revert the entire sorry > container mess and use VMs but it's a bit late for that ;-)] > Totally agree. VMs >> Containers but the cat is out of the bag and we can't put it back.
On Tue, 2017-05-30 at 19:00 -0400, Matt Brown wrote: > On 5/30/17 4:22 PM, Daniel Micay wrote: > > > Thanks, I didn't know that android was doing this. I still think > > > this > > > feature > > > is worthwhile for people to be able to harden their systems > > > against > > > this attack > > > vector without having to implement a MAC. > > > > Since there's a capable LSM hook for ioctl already, it means it > > could go > > in Yama with ptrace_scope but core kernel code would still need to > > be > > changed to track the owning tty. I think Yama vs. core kernel > > shouldn't > > matter much anymore due to stackable LSMs. > > > > What does everyone think about a v8 that moves this feature under Yama > and uses > the file_ioctl LSM hook? It would only make a difference if it could be fully contained there, as in not depending on tracking the tty owner.
> This is my point. Apps will continue to shoot themselves in the foot. Of course > the correct response to one of these vulns is to not pass ttys across a > security boundary. We have an opportunity here to reduce the impact of this bug > class at the kernel level. Not really. If you pass me your console for example I can mmap your framebuffer and spy on you all day. Or I could reprogram your fonts, your keyboard, your video mode, or use set and paste selection to write stuff. If you are using X and you can't get tty handles right you'll no doubt pass me a copy of your X file descriptor in which case I own your display, your keyboard and your mouse and I don't need to use TIOCSTI there either. There are so many different attacks based upon that screwup that the kernel cannot defend against them. You aren't exactly reducing the impact. Alan
On 5/30/17 7:40 PM, Daniel Micay wrote: > On Tue, 2017-05-30 at 19:00 -0400, Matt Brown wrote: >> On 5/30/17 4:22 PM, Daniel Micay wrote: >>>> Thanks, I didn't know that android was doing this. I still think >>>> this >>>> feature >>>> is worthwhile for people to be able to harden their systems >>>> against >>>> this attack >>>> vector without having to implement a MAC. >>> >>> Since there's a capable LSM hook for ioctl already, it means it >>> could go >>> in Yama with ptrace_scope but core kernel code would still need to >>> be >>> changed to track the owning tty. I think Yama vs. core kernel >>> shouldn't >>> matter much anymore due to stackable LSMs. >>> >> >> What does everyone think about a v8 that moves this feature under Yama >> and uses >> the file_ioctl LSM hook? > > It would only make a difference if it could be fully contained there, as > in not depending on tracking the tty owner. > For the reasons discussed earlier (to allow for nested containers where one of the containers is privileged) we want to track the user namespace that owns the tty.
On Mon, 29 May 2017, Boris Lukashev wrote: > With all due respect sir, i believe your review falls short of the > purpose of this effort - to harden the kernel against flaws in > userspace. Which effort? Kernel self protection is about protecting against flaws in the kernel. See: https://kernsec.org/wiki/index.php/Kernel_Self_Protection_Project "This project starts with the premise that kernel bugs have a very long lifetime, and that the kernel must be designed in ways to protect against these flaws." We need to avoid conflating: - hardening the kernel against attack; and - modifying the kernel to try and harden userspace. These patches are the latter, and the case for them is not as straightforward. - James
On 05/30/2017 10:48 PM, James Morris wrote: > On Mon, 29 May 2017, Boris Lukashev wrote: > >> With all due respect sir, i believe your review falls short of the >> purpose of this effort - to harden the kernel against flaws in >> userspace. > > Which effort? Kernel self protection is about protecting against flaws in > the kernel. > > See: > https://kernsec.org/wiki/index.php/Kernel_Self_Protection_Project > > "This project starts with the premise that kernel bugs have a very long > lifetime, and that the kernel must be designed in ways to protect against > these flaws." > > We need to avoid conflating: > > - hardening the kernel against attack; and > - modifying the kernel to try and harden userspace. > > These patches are the latter, and the case for them is not as > straightforward. > > > - James > I agree that these patches aren't kernel self protection and I don't believe I have claimed they are such a thing. These patches I'm presenting are more akin to ptrace protections that are found in Yama.
On Tue, May 30, 2017 at 4:56 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >> This is my point. Apps will continue to shoot themselves in the foot. Of course >> the correct response to one of these vulns is to not pass ttys across a >> security boundary. We have an opportunity here to reduce the impact of this bug >> class at the kernel level. > > Not really. > > If you pass me your console for example I can mmap your framebuffer and > spy on you all day. Or I could reprogram your fonts, your keyboard, your > video mode, or use set and paste selection to write stuff. If you are > using X and you can't get tty handles right you'll no doubt pass me a > copy of your X file descriptor in which case I own your display, your > keyboard and your mouse and I don't need to use TIOCSTI there either. > > There are so many different attacks based upon that screwup that the > kernel cannot defend against them. You aren't exactly reducing the impact. I still cannot wrap my head around why providing users with a protection is a bad thing. Yes, the other tty games are bad, but this fixes a specific and especially bad case that is easy to kill. It's got a Kconfig and a sysctl. It's not on by default. This protects the common case of privileged ttys that aren't attached to consoles, etc, so while the framebuffer thing is an issue, it's not always an issue, etc. I'd like to hear Greg's thoughts on this series... -Kees
On 6/1/17 4:35 AM, Kees Cook wrote: > I still cannot wrap my head around why providing users with a > protection is a bad thing. Yes, the other tty games are bad, but this > fixes a specific and especially bad case that is easy to kill. It's > got a Kconfig and a sysctl. It's not on by default. This protects the > common case of privileged ttys that aren't attached to consoles, etc, > so while the framebuffer thing is an issue, it's not always an issue, > etc. There are a couple of reasons for that: First of all, a protection is extra cost, in terms of maintenance, knowledge (a new knob) and compatibility. That extra cost may sound minimal, but adds up pretty quickly. If the protection is "easily" bypassable (that is, today we use TIOCSTI, tomorrow we use something else in the same path), then that extra cost/complexity stays for no good reason. Feature creep is a real issue, in security, IMHO - it's not a 'number of features' game. Second, stuff that is delivered off by default tends to rot. I don't work on Linux, but generally try really hard to not add something that is not ON by default at least for a small number of things. Stuff inevitably breaks, and it's extra cost. To me, a protection that needs to be off by default, raises a red flag. I know Linux has a somewhat different philosophy (centered around the kernel config that each distribution pieces together and ships), so mileage probably varies there. I don't have enough skills to comment about all the possible TTY attacks and quirks, but I think I understand where Alan comes from. Good luck. - Enrico
> I still cannot wrap my head around why providing users with a > protection is a bad thing. Yes, the other tty games are bad, but this > fixes a specific and especially bad case that is easy to kill. It's > got a Kconfig and a sysctl. It's not on by default. This protects the > common case of privileged ttys that aren't attached to consoles, etc, Which just leads to stuff not getting fixed. Like all the code out there today which is still vulnerable to selection based attacks because people didn't do the job right when "fixing" stuff because they are not thinking about security at a systems level but just tickboxing CVEs. I'm not against doing something to protect the container folks, but that something as with Android is a whitelist of ioctls. And if we need to do this with a kernel hook lets do it properly. Remember the namespace of the tty on creation If the magic security flag is set then Apply a whitelist to *any* tty ioctl call where the ns doesn't match and we might as well just take the Android whitelist since they've kindly built it for us all! In the tty layer it ends up being something around 10 lines of code and some other file somewhere in security/ that's just a switch or similar with the whitelisted ioctl codes in it. That (or a similar SELinux ruleset) would actually fix the problem. SELinux would be better because it can also apply the rules when doing things like su/sudo/... Alan
Quoting Alan Cox (gnomes@lxorguk.ukuu.org.uk): > > I still cannot wrap my head around why providing users with a > > protection is a bad thing. Yes, the other tty games are bad, but this > > fixes a specific and especially bad case that is easy to kill. It's > > got a Kconfig and a sysctl. It's not on by default. This protects the > > common case of privileged ttys that aren't attached to consoles, etc, > > Which just leads to stuff not getting fixed. Like all the code out there > today which is still vulnerable to selection based attacks because people > didn't do the job right when "fixing" stuff because they are not > thinking about security at a systems level but just tickboxing CVEs. > > I'm not against doing something to protect the container folks, but that > something as with Android is a whitelist of ioctls. And if we need to do Whitelist of ioctls (at least using seccomp) is not sufficient because then we have to turn the ioctl always-off. But like you say we may want to enable it for ptys which are created inside the container's user ns. > this with a kernel hook lets do it properly. > > Remember the namespace of the tty on creation Matt's patch does this, > If the magic security flag is set then > Apply a whitelist to *any* tty ioctl call where the ns doesn't > match Seems sensible. > and we might as well just take the Android whitelist since they've kindly > built it for us all! > > In the tty layer it ends up being something around 10 lines of code and > some other file somewhere in security/ that's just a switch or similar > with the whitelisted ioctl codes in it. > > That (or a similar SELinux ruleset) would actually fix the problem. > SELinux would be better because it can also apply the rules when doing > things like su/sudo/... > > Alan
On Thu, Jun 1, 2017 at 12:12 AM, lazytyped <lazytyped@gmail.com> wrote: > > > On 6/1/17 4:35 AM, Kees Cook wrote: >> >> I still cannot wrap my head around why providing users with a >> protection is a bad thing. Yes, the other tty games are bad, but this >> fixes a specific and especially bad case that is easy to kill. It's >> got a Kconfig and a sysctl. It's not on by default. This protects the >> common case of privileged ttys that aren't attached to consoles, etc, >> so while the framebuffer thing is an issue, it's not always an issue, >> etc. > > There are a couple of reasons for that: > > First of all, a protection is extra cost, in terms of maintenance, knowledge > (a new knob) and compatibility. That extra cost may sound minimal, but adds > up pretty quickly. If the protection is "easily" bypassable (that is, today > we use TIOCSTI, tomorrow we use something else in the same path), then that > extra cost/complexity stays for no good reason. Feature creep is a real > issue, in security, IMHO - it's not a 'number of features' game. Yes, absolutely. But the trouble tends to be around "poor initial design" userspace APIs that need work-arounds to make them safe. These cases are relatively rare, luckily, so there's not much actual "slippery slope" for adding those kinds of protections. As for bypassing, besides Alan's physical-console-specific issues, I haven't seen anyone point out things with privileged TTYs that are still an escalation path when TIOCSTI is unavailable. So, the way I see it, this is a fix for a bad API that keeps being coded poorly in userspace that, while it doesn't fix all physical console attacks, does block the specific issue introduced by TIOCSTI. And since there are legit users of TIOCSTI we can't just remove it, since some people want the functionality over the risk. Hence, a sysctl. All of the reasoning here seems to match the link restrictions from 5 years ago: a crappy API (sticky bit) is not handled by userspace (open /tmp/$$!) and people get attacked. The solution was a sysctl to enable the link restrictions that killed the entire class of the common attack (though it didn't solve especially egregious bad uses, much like the TIOCSTI fix). Every distro enabled the sysctl, and, while the data is noisy, looking a CVEs matching "/tmp symlink", the numbers drop from 2013 and later (with none yet for 2017). > Second, stuff that is delivered off by default tends to rot. I don't work on > Linux, but generally try really hard to not add something that is not ON by > default at least for a small number of things. Stuff inevitably breaks, and > it's extra cost. > To me, a protection that needs to be off by default, raises a red flag. I > know Linux has a somewhat different philosophy (centered around the kernel > config that each distribution pieces together and ships), so mileage > probably varies there. Totally agreed in general about the default-off, but the Linux ecosystem is a weird one: Linus has mandated that the kernel can never break userspace by default, effectively freeing upstream developers from the responsibility of dealing with such breakage. However, distros can have their own set of defaults. In many of the security-sensitive areas, distros turn things on by default. As mentioned above, link restrictions were enabled by default on distros (some even before it was upstream), as well as things like CONFIG_DEBUG_RODATA (before it was finally made mandatory, a decade later), CONFIG_HARDENED_USERCOPY, etc. This is the evolution of Linux security defenses: first it has to exist (but default-off), then everyone gets comfortable with it being enabled because distros turn it on, then everyone gets frustrated that it's not default-on in the kernel and changes it there too, either by identifying improvements to allow it on-by-default or just having a flag-day. An example of the former might be the ancient progression of /proc/self/maps protections (on-show sysctl in 2.6.22, on-open mm check in 2.6.24, drop sysctl in 2.6.27). For the latter, noted above, CONFIG_DEBUG_RODATA (now CONFIG_STRICT_KERNEL_RWX) is a good example where it was just decided, finally, to make it mandatory (on x86 and arm64 so far). > I don't have enough skills to comment about all the possible TTY attacks and > quirks, but I think I understand where Alan comes from. Yeah, agreed. I certainly don't claim it solves all the TTY risks, but it closes the door on a big one. That kind of change has real-world benefits, and we have decades of evidence to show that just saying "fix it in userspace" is not going to protect anyone. -Kees
On Thu, Jun 1, 2017 at 6:08 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >> I still cannot wrap my head around why providing users with a >> protection is a bad thing. Yes, the other tty games are bad, but this >> fixes a specific and especially bad case that is easy to kill. It's >> got a Kconfig and a sysctl. It's not on by default. This protects the >> common case of privileged ttys that aren't attached to consoles, etc, > > Which just leads to stuff not getting fixed. Like all the code out there > today which is still vulnerable to selection based attacks because people > didn't do the job right when "fixing" stuff because they are not > thinking about security at a systems level but just tickboxing CVEs. There's a difference between "bugs" and "security bugs". Letting security bugs continue to get exploited because we want to flush out bugs seems insensitive to the people getting attacked. I'd rather protect against a class of bug than have to endless fix each bug. > I'm not against doing something to protect the container folks, but that > something as with Android is a whitelist of ioctls. And if we need to do > this with a kernel hook lets do it properly. > > Remember the namespace of the tty on creation > If the magic security flag is set then > Apply a whitelist to *any* tty ioctl call where the ns doesn't > match > > and we might as well just take the Android whitelist since they've kindly > built it for us all! > > In the tty layer it ends up being something around 10 lines of code and > some other file somewhere in security/ that's just a switch or similar > with the whitelisted ioctl codes in it. > > That (or a similar SELinux ruleset) would actually fix the problem. > SELinux would be better because it can also apply the rules when doing > things like su/sudo/... Just to play devil's advocate, wouldn't such a system continue to not address your physical-console concerns? I wouldn't want to limit the protection to only containers (but it's a good start), since it wouldn't protect people not using containers that still have a privileged TTY attached badly somewhere. If you're talking about wholistic SELinux policy, sure, I could imagine a wholistic fix. But for the tons of people without a comprehensive SELinux policy, the proposed protection continues to make sense. -Kees
> There's a difference between "bugs" and "security bugs". Letting Not really, it's merely a matter of severity of result. A non security bug that hoses your hard disk is to anyone but security nutcases at least as bad as a security hole. > security bugs continue to get exploited because we want to flush out > bugs seems insensitive to the people getting attacked. I'd rather > protect against a class of bug than have to endless fix each bug. The others are security bugs too to varying degree > > I'm not against doing something to protect the container folks, but that > > something as with Android is a whitelist of ioctls. And if we need to do > > this with a kernel hook lets do it properly. > > > > Remember the namespace of the tty on creation > > If the magic security flag is set then > > Apply a whitelist to *any* tty ioctl call where the ns doesn't > > match > > > > and we might as well just take the Android whitelist since they've kindly > > built it for us all! > > > > In the tty layer it ends up being something around 10 lines of code and > > some other file somewhere in security/ that's just a switch or similar > > with the whitelisted ioctl codes in it. > > > > That (or a similar SELinux ruleset) would actually fix the problem. > > SELinux would be better because it can also apply the rules when doing > > things like su/sudo/... > > Just to play devil's advocate, wouldn't such a system continue to not > address your physical-console concerns? I wouldn't want to limit the It would for the cases that a whitelist and container check covers - because the whitelist wouldn't allow you to do anything but boring stuff on the tty. TIOCSTI is just one of a whole range of differently stupid and annoying opportunities. Containers do not and should not be able to set the keymap, change the video mode, use console selection, make funny beepy noises, access video I/O registers and all the other stuff like that. Nothing is going to break if we have a fairly conservative whitelist. > protection to only containers (but it's a good start), since it > wouldn't protect people not using containers that still have a > privileged TTY attached badly somewhere. How are you going to magically fix the problem. I'm not opposed to fixing the real problem but right now it appears to be a product of wishful thinking not programming. What's the piece of security code that magically discerns the fact you are running something untrusted at the other end of your tty. SELinux can do it via labelling but I don't see any generic automatic way for the kernel to magically work out when to whitelist and when not to. If there is a better magic rule than differing-namespace then provide the code. You can't just disable TIOCSTI, it has users deal with it. You can get away with disabling it for namespace crossing I think but if you do that you need to disable a pile of others. (If it breaks containers blocking TIOCSTI then we need to have a good look at algorithms for deciding when to flush the input queue on exiting a container or somesuch) > If you're talking about wholistic SELinux policy, sure, I could > imagine a wholistic fix. But for the tons of people without a > comprehensive SELinux policy, the proposed protection continues to > make sense. No it doesn't. It's completely useless unless you actually bother to address the other exploit opportunities. Right now the proposal is a hack to do if (TIOCSTI && different_namespace && magic_flag) the proper solution is if (!whitelisted(ioctl) && different_namespace && magic_flag) The former is snake oil, the latter actually deals with the problem space for namespaced stuff comprehensively and is only a tiny amount more code. For non namespaced stuff it makes it no worse, if you don't allocate a pty/tty pair properly then the gods are not going to magically save you from on high sorry. And if you want to completely kill TIOCSTI even though it's kind of pointless you can use seccomp. We can make things a bit better for the non-namespaced cases by providing a new ioctl that turns on/off whitelisting for that tty so that the process can do ioctl(tty_fd, TIOCTRUST, &zero); execl("/home/foo/stupid-idea", "ooops", NULL); that's a simple per tty flag and a trivial one condition extra check to the test above. We do need some way to change it back however and that's a bit trickier given we don't want the stupid-idea tool to be able to but we do want the invoking shell to - maybe you have to be session leader ? Alan
On Thu, 1 Jun 2017 12:18:58 -0500 "Serge E. Hallyn" <serge@hallyn.com> wrote: > Quoting Alan Cox (gnomes@lxorguk.ukuu.org.uk): > > > I still cannot wrap my head around why providing users with a > > > protection is a bad thing. Yes, the other tty games are bad, but this > > > fixes a specific and especially bad case that is easy to kill. It's > > > got a Kconfig and a sysctl. It's not on by default. This protects the > > > common case of privileged ttys that aren't attached to consoles, etc, > > > > Which just leads to stuff not getting fixed. Like all the code out there > > today which is still vulnerable to selection based attacks because people > > didn't do the job right when "fixing" stuff because they are not > > thinking about security at a systems level but just tickboxing CVEs. > > > > I'm not against doing something to protect the container folks, but that > > something as with Android is a whitelist of ioctls. And if we need to do > > Whitelist of ioctls (at least using seccomp) is not sufficient because > then we have to turn the ioctl always-off. But like you say we may want > to enable it for ptys which are created inside the container's user ns. > > > this with a kernel hook lets do it properly. > > > > Remember the namespace of the tty on creation > > Matt's patch does this, > > > If the magic security flag is set then > > Apply a whitelist to *any* tty ioctl call where the ns doesn't > > match > > Seems sensible. I'm arguing that we need to swap the TIOCSTI test for a !whitelisted() test to go with the namespace difference check. Then it makes sense because we actually address the real problem. Alan
On Thu, 1 Jun 2017, Kees Cook wrote: > All of the reasoning here seems to match the link restrictions from 5 > years ago: a crappy API (sticky bit) is not handled by userspace (open > /tmp/$$!) and people get attacked. The solution was a sysctl to enable > the link restrictions that killed the entire class of the common > attack (though it didn't solve especially egregious bad uses, much This is the problem -- it doesn't really eliminate the underlying issue. A better solution (in this case) was to implement a new API which addresses the issue at an architectural level, i.e. namespace-based private /tmp views, and encourage its adoption. > like the TIOCSTI fix). Every distro enabled the sysctl, and, while the > data is noisy, looking a CVEs matching "/tmp symlink", the numbers > drop from 2013 and later (with none yet for 2017). I wonder how much of this is due to the sysctl vs. adoption of private /tmp, and what may be lurking in the "egregious bad uses" category for future CVEs. And obviously we don't know what various folk may have up their sleeves, if anything.
On 6/1/17 5:24 PM, Alan Cox wrote: >> There's a difference between "bugs" and "security bugs". Letting > > Not really, it's merely a matter of severity of result. A non security > bug that hoses your hard disk is to anyone but security nutcases at > least as bad as a security hole. > >> security bugs continue to get exploited because we want to flush out >> bugs seems insensitive to the people getting attacked. I'd rather >> protect against a class of bug than have to endless fix each bug. > > The others are security bugs too to varying degree > >>> I'm not against doing something to protect the container folks, but that >>> something as with Android is a whitelist of ioctls. And if we need to do >>> this with a kernel hook lets do it properly. >>> >>> Remember the namespace of the tty on creation >>> If the magic security flag is set then >>> Apply a whitelist to *any* tty ioctl call where the ns doesn't >>> match >>> >>> and we might as well just take the Android whitelist since they've kindly >>> built it for us all! >>> >>> In the tty layer it ends up being something around 10 lines of code and >>> some other file somewhere in security/ that's just a switch or similar >>> with the whitelisted ioctl codes in it. >>> >>> That (or a similar SELinux ruleset) would actually fix the problem. >>> SELinux would be better because it can also apply the rules when doing >>> things like su/sudo/... >> >> Just to play devil's advocate, wouldn't such a system continue to not >> address your physical-console concerns? I wouldn't want to limit the > > It would for the cases that a whitelist and container check covers - > because the whitelist wouldn't allow you to do anything but boring stuff > on the tty. TIOCSTI is just one of a whole range of differently stupid > and annoying opportunities. Containers do not and should not be able to > set the keymap, change the video mode, use console selection, make funny > beepy noises, access video I/O registers and all the other stuff like > that. Nothing is going to break if we have a fairly conservative > whitelist. > >> protection to only containers (but it's a good start), since it >> wouldn't protect people not using containers that still have a >> privileged TTY attached badly somewhere. > > How are you going to magically fix the problem. I'm not opposed to fixing > the real problem but right now it appears to be a product of wishful > thinking not programming. What's the piece of security code that > magically discerns the fact you are running something untrusted at the > other end of your tty. SELinux can do it via labelling but I don't see > any generic automatic way for the kernel to magically work out when to > whitelist and when not to. If there is a better magic rule than > differing-namespace then provide the code. > > You can't just disable TIOCSTI, it has users deal with it. You can > get away with disabling it for namespace crossing I think but if you do > that you need to disable a pile of others. > > (If it breaks containers blocking TIOCSTI then we need to have a good > look at algorithms for deciding when to flush the input queue on exiting > a container or somesuch) > >> If you're talking about wholistic SELinux policy, sure, I could >> imagine a wholistic fix. But for the tons of people without a >> comprehensive SELinux policy, the proposed protection continues to >> make sense. > > No it doesn't. It's completely useless unless you actually bother to > address the other exploit opportunities. > > Right now the proposal is a hack to do > > if (TIOCSTI && different_namespace && magic_flag) > This is not what my patch does. Mine is like: if (TIOCSTI && !ns_capable(tty->owner_user_ns, CAP_SYS_ADMIN) && magic_flag) in other words: if (TIOCSTI && (different_owner_user_ns || !CAP_SYS_ADMIN) && magic_flag) can you specify what you mean by different_namespace? which namespace? The reason I brought in the user namespace was to solve an edge case with nested containers. capable() will never be true in a container, so nested containers would break if we didn't do the ns_capable check. This is also the reason for the addition of owner_user_ns to tty_struct. The point of my patch is to prevent any type of tiocsti activity where the tty is given to an unprivileged process/container. The emphasis of my check is on the CAP_SYS_ADMIN part, not the namespace part. > the proper solution is > > if (!whitelisted(ioctl) && different_namespace && magic_flag) > > The former is snake oil, the latter actually deals with the problem space > for namespaced stuff comprehensively and is only a tiny amount more code. > > For non namespaced stuff it makes it no worse, if you don't allocate a > pty/tty pair properly then the gods are not going to magically save you > from on high sorry. And if you want to completely kill TIOCSTI even > though it's kind of pointless you can use seccomp. > > We can make things a bit better for the non-namespaced cases by providing > a new ioctl that turns on/off whitelisting for that tty so that the > process can do > > ioctl(tty_fd, TIOCTRUST, &zero); > execl("/home/foo/stupid-idea", "ooops", NULL); > > that's a simple per tty flag and a trivial one condition extra check to > the test above. We do need some way to change it back however and that's > a bit trickier given we don't want the stupid-idea tool to be able to but > we do want the invoking shell to - maybe you have to be session leader ? > > Alan >
Quoting Matt Brown (matt@nmatt.com): > On 6/1/17 5:24 PM, Alan Cox wrote: > >> There's a difference between "bugs" and "security bugs". Letting > > > > Not really, it's merely a matter of severity of result. A non security > > bug that hoses your hard disk is to anyone but security nutcases at > > least as bad as a security hole. > > > >> security bugs continue to get exploited because we want to flush out > >> bugs seems insensitive to the people getting attacked. I'd rather > >> protect against a class of bug than have to endless fix each bug. > > > > The others are security bugs too to varying degree > > > >>> I'm not against doing something to protect the container folks, but that > >>> something as with Android is a whitelist of ioctls. And if we need to do > >>> this with a kernel hook lets do it properly. > >>> > >>> Remember the namespace of the tty on creation > >>> If the magic security flag is set then > >>> Apply a whitelist to *any* tty ioctl call where the ns doesn't > >>> match > >>> > >>> and we might as well just take the Android whitelist since they've kindly > >>> built it for us all! > >>> > >>> In the tty layer it ends up being something around 10 lines of code and > >>> some other file somewhere in security/ that's just a switch or similar > >>> with the whitelisted ioctl codes in it. > >>> > >>> That (or a similar SELinux ruleset) would actually fix the problem. > >>> SELinux would be better because it can also apply the rules when doing > >>> things like su/sudo/... > >> > >> Just to play devil's advocate, wouldn't such a system continue to not > >> address your physical-console concerns? I wouldn't want to limit the > > > > It would for the cases that a whitelist and container check covers - > > because the whitelist wouldn't allow you to do anything but boring stuff > > on the tty. TIOCSTI is just one of a whole range of differently stupid > > and annoying opportunities. Containers do not and should not be able to > > set the keymap, change the video mode, use console selection, make funny > > beepy noises, access video I/O registers and all the other stuff like > > that. Nothing is going to break if we have a fairly conservative > > whitelist. > > > >> protection to only containers (but it's a good start), since it > >> wouldn't protect people not using containers that still have a > >> privileged TTY attached badly somewhere. > > > > How are you going to magically fix the problem. I'm not opposed to fixing > > the real problem but right now it appears to be a product of wishful > > thinking not programming. What's the piece of security code that > > magically discerns the fact you are running something untrusted at the > > other end of your tty. SELinux can do it via labelling but I don't see > > any generic automatic way for the kernel to magically work out when to > > whitelist and when not to. If there is a better magic rule than > > differing-namespace then provide the code. > > > > You can't just disable TIOCSTI, it has users deal with it. You can > > get away with disabling it for namespace crossing I think but if you do > > that you need to disable a pile of others. > > > > (If it breaks containers blocking TIOCSTI then we need to have a good > > look at algorithms for deciding when to flush the input queue on exiting > > a container or somesuch) > > > >> If you're talking about wholistic SELinux policy, sure, I could > >> imagine a wholistic fix. But for the tons of people without a > >> comprehensive SELinux policy, the proposed protection continues to > >> make sense. > > > > No it doesn't. It's completely useless unless you actually bother to > > address the other exploit opportunities. > > > > Right now the proposal is a hack to do > > > > if (TIOCSTI && different_namespace && magic_flag) > > > > > This is not what my patch does. Mine is like: > > if (TIOCSTI && !ns_capable(tty->owner_user_ns, CAP_SYS_ADMIN) && > magic_flag) > > in other words: > if (TIOCSTI && (different_owner_user_ns || !CAP_SYS_ADMIN) && > magic_flag) > > can you specify what you mean by different_namespace? which namespace? I think you're focusing on the wrong thing. Your capable check (apart from the fact that I think I've been convinced CAP_SYS_ADMIN is wrong) is fine. The key point is to not only check for TIOCSTI, but instead check for a whitelisted ioctl. What would the whitelist look like? Should configuing that be the way that you enable/disable, instead of the sysctl in this patchset? So by default the whitelist includes all ioctls (no change), but things like sandboxes/sudo/container-starts can clear out the whitelist?
On 6/2/17 11:36 AM, Serge E. Hallyn wrote: > Quoting Matt Brown (matt@nmatt.com): >> On 6/1/17 5:24 PM, Alan Cox wrote: >>>> There's a difference between "bugs" and "security bugs". Letting >>> >>> Not really, it's merely a matter of severity of result. A non security >>> bug that hoses your hard disk is to anyone but security nutcases at >>> least as bad as a security hole. >>> >>>> security bugs continue to get exploited because we want to flush out >>>> bugs seems insensitive to the people getting attacked. I'd rather >>>> protect against a class of bug than have to endless fix each bug. >>> >>> The others are security bugs too to varying degree >>> >>>>> I'm not against doing something to protect the container folks, but that >>>>> something as with Android is a whitelist of ioctls. And if we need to do >>>>> this with a kernel hook lets do it properly. >>>>> >>>>> Remember the namespace of the tty on creation >>>>> If the magic security flag is set then >>>>> Apply a whitelist to *any* tty ioctl call where the ns doesn't >>>>> match >>>>> >>>>> and we might as well just take the Android whitelist since they've kindly >>>>> built it for us all! >>>>> >>>>> In the tty layer it ends up being something around 10 lines of code and >>>>> some other file somewhere in security/ that's just a switch or similar >>>>> with the whitelisted ioctl codes in it. >>>>> >>>>> That (or a similar SELinux ruleset) would actually fix the problem. >>>>> SELinux would be better because it can also apply the rules when doing >>>>> things like su/sudo/... >>>> >>>> Just to play devil's advocate, wouldn't such a system continue to not >>>> address your physical-console concerns? I wouldn't want to limit the >>> >>> It would for the cases that a whitelist and container check covers - >>> because the whitelist wouldn't allow you to do anything but boring stuff >>> on the tty. TIOCSTI is just one of a whole range of differently stupid >>> and annoying opportunities. Containers do not and should not be able to >>> set the keymap, change the video mode, use console selection, make funny >>> beepy noises, access video I/O registers and all the other stuff like >>> that. Nothing is going to break if we have a fairly conservative >>> whitelist. >>> >>>> protection to only containers (but it's a good start), since it >>>> wouldn't protect people not using containers that still have a >>>> privileged TTY attached badly somewhere. >>> >>> How are you going to magically fix the problem. I'm not opposed to fixing >>> the real problem but right now it appears to be a product of wishful >>> thinking not programming. What's the piece of security code that >>> magically discerns the fact you are running something untrusted at the >>> other end of your tty. SELinux can do it via labelling but I don't see >>> any generic automatic way for the kernel to magically work out when to >>> whitelist and when not to. If there is a better magic rule than >>> differing-namespace then provide the code. >>> >>> You can't just disable TIOCSTI, it has users deal with it. You can >>> get away with disabling it for namespace crossing I think but if you do >>> that you need to disable a pile of others. >>> >>> (If it breaks containers blocking TIOCSTI then we need to have a good >>> look at algorithms for deciding when to flush the input queue on exiting >>> a container or somesuch) >>> >>>> If you're talking about wholistic SELinux policy, sure, I could >>>> imagine a wholistic fix. But for the tons of people without a >>>> comprehensive SELinux policy, the proposed protection continues to >>>> make sense. >>> >>> No it doesn't. It's completely useless unless you actually bother to >>> address the other exploit opportunities. >>> >>> Right now the proposal is a hack to do >>> >>> if (TIOCSTI && different_namespace && magic_flag) >>> >> >> >> This is not what my patch does. Mine is like: >> >> if (TIOCSTI && !ns_capable(tty->owner_user_ns, CAP_SYS_ADMIN) && >> magic_flag) >> >> in other words: >> if (TIOCSTI && (different_owner_user_ns || !CAP_SYS_ADMIN) && >> magic_flag) >> >> can you specify what you mean by different_namespace? which namespace? > > I think you're focusing on the wrong thing. Your capable check (apart > from the fact that I think I've been convinced CAP_SYS_ADMIN is wrong) > is fine. Can't we also have a sysctl that toggles if CAP_SYS_ADMIN is involved in this whitelist check? Otherwise someone might leave things out of the whitelist just because they want to use those ioctls as a privileged process. Also restricting a privileged user from ioctls with this whitelist approach is going to be pointless because, if the whitelist is configurable from userspace, they will just be able to modify the whitelist. > > The key point is to not only check for TIOCSTI, but instead check for > a whitelisted ioctl. > > What would the whitelist look like? Should configuing that be the way > that you enable/disable, instead of the sysctl in this patchset? So > by default the whitelist includes all ioctls (no change), but things > like sandboxes/sudo/container-starts can clear out the whitelist? > I'm fine with moving this to an LSM that whitelists ioctls. I also want to understand what a whitelist would like look and how you would configure it? Does a sysctl that is a list of allowed ioctls work? I don't want to just have a static whitelist that you can't change without recompiling your kernel. just running a sysctl -a on a linux box shows me one thing that looks like a list: net.core.flow_limit_cpu_bitmap
Quoting Matt Brown (matt@nmatt.com): > On 6/2/17 11:36 AM, Serge E. Hallyn wrote: > > Quoting Matt Brown (matt@nmatt.com): > >> On 6/1/17 5:24 PM, Alan Cox wrote: > >>>> There's a difference between "bugs" and "security bugs". Letting > >>> > >>> Not really, it's merely a matter of severity of result. A non security > >>> bug that hoses your hard disk is to anyone but security nutcases at > >>> least as bad as a security hole. > >>> > >>>> security bugs continue to get exploited because we want to flush out > >>>> bugs seems insensitive to the people getting attacked. I'd rather > >>>> protect against a class of bug than have to endless fix each bug. > >>> > >>> The others are security bugs too to varying degree > >>> > >>>>> I'm not against doing something to protect the container folks, but that > >>>>> something as with Android is a whitelist of ioctls. And if we need to do > >>>>> this with a kernel hook lets do it properly. > >>>>> > >>>>> Remember the namespace of the tty on creation > >>>>> If the magic security flag is set then > >>>>> Apply a whitelist to *any* tty ioctl call where the ns doesn't > >>>>> match > >>>>> > >>>>> and we might as well just take the Android whitelist since they've kindly > >>>>> built it for us all! > >>>>> > >>>>> In the tty layer it ends up being something around 10 lines of code and > >>>>> some other file somewhere in security/ that's just a switch or similar > >>>>> with the whitelisted ioctl codes in it. > >>>>> > >>>>> That (or a similar SELinux ruleset) would actually fix the problem. > >>>>> SELinux would be better because it can also apply the rules when doing > >>>>> things like su/sudo/... > >>>> > >>>> Just to play devil's advocate, wouldn't such a system continue to not > >>>> address your physical-console concerns? I wouldn't want to limit the > >>> > >>> It would for the cases that a whitelist and container check covers - > >>> because the whitelist wouldn't allow you to do anything but boring stuff > >>> on the tty. TIOCSTI is just one of a whole range of differently stupid > >>> and annoying opportunities. Containers do not and should not be able to > >>> set the keymap, change the video mode, use console selection, make funny > >>> beepy noises, access video I/O registers and all the other stuff like > >>> that. Nothing is going to break if we have a fairly conservative > >>> whitelist. > >>> > >>>> protection to only containers (but it's a good start), since it > >>>> wouldn't protect people not using containers that still have a > >>>> privileged TTY attached badly somewhere. > >>> > >>> How are you going to magically fix the problem. I'm not opposed to fixing > >>> the real problem but right now it appears to be a product of wishful > >>> thinking not programming. What's the piece of security code that > >>> magically discerns the fact you are running something untrusted at the > >>> other end of your tty. SELinux can do it via labelling but I don't see > >>> any generic automatic way for the kernel to magically work out when to > >>> whitelist and when not to. If there is a better magic rule than > >>> differing-namespace then provide the code. > >>> > >>> You can't just disable TIOCSTI, it has users deal with it. You can > >>> get away with disabling it for namespace crossing I think but if you do > >>> that you need to disable a pile of others. > >>> > >>> (If it breaks containers blocking TIOCSTI then we need to have a good > >>> look at algorithms for deciding when to flush the input queue on exiting > >>> a container or somesuch) > >>> > >>>> If you're talking about wholistic SELinux policy, sure, I could > >>>> imagine a wholistic fix. But for the tons of people without a > >>>> comprehensive SELinux policy, the proposed protection continues to > >>>> make sense. > >>> > >>> No it doesn't. It's completely useless unless you actually bother to > >>> address the other exploit opportunities. > >>> > >>> Right now the proposal is a hack to do > >>> > >>> if (TIOCSTI && different_namespace && magic_flag) > >>> > >> > >> > >> This is not what my patch does. Mine is like: > >> > >> if (TIOCSTI && !ns_capable(tty->owner_user_ns, CAP_SYS_ADMIN) && > >> magic_flag) > >> > >> in other words: > >> if (TIOCSTI && (different_owner_user_ns || !CAP_SYS_ADMIN) && > >> magic_flag) > >> > >> can you specify what you mean by different_namespace? which namespace? > > > > I think you're focusing on the wrong thing. Your capable check (apart > > from the fact that I think I've been convinced CAP_SYS_ADMIN is wrong) > > is fine. > I'm cc:ing linux-api here because really we're designing an interesting API. > Can't we also have a sysctl that toggles if CAP_SYS_ADMIN is involved in > this whitelist check? Otherwise someone might leave things out of the > whitelist just because they want to use those ioctls as a privileged > process. I'm not quite sure what you're asking for here. Let me offer a precise strawman design. I'm sure there are problems with it, it's just a starting point. system-wide whitelist (for now 'may_push_chars') is full by default. By default, nothing changes - you can use those on your own tty, need CAP_SYS_ADMIN against init_user_ns otherwise. Introduce a new CAP_TTY_PRIVILEGED. When may_push_chars is removed from the whitelist, you lose the ability to use TIOCSTI on a tty - even your own - if you do not have CAP_TTY_PRIVILEGED against the tty's user_ns. > Also restricting a privileged user from ioctls with this > whitelist approach is going to be pointless because, if the whitelist is > configurable from userspace, they will just be able to modify the > whitelist. > > > > > The key point is to not only check for TIOCSTI, but instead check for > > a whitelisted ioctl. > > > > What would the whitelist look like? Should configuing that be the way > > that you enable/disable, instead of the sysctl in this patchset? So > > by default the whitelist includes all ioctls (no change), but things > > like sandboxes/sudo/container-starts can clear out the whitelist? > > > > I'm fine with moving this to an LSM that whitelists ioctls. I also want Right - what else would go into the whitelist? may_mmap? > to understand what a whitelist would like look and how you would > configure it? Does a sysctl that is a list of allowed ioctls work? I > don't want to just have a static whitelist that you can't change without > recompiling your kernel. > > just running a sysctl -a on a linux box shows me one thing that looks > like a list: net.core.flow_limit_cpu_bitmap
On 6/2/17 12:57 PM, Serge E. Hallyn wrote: > Quoting Matt Brown (matt@nmatt.com): >> On 6/2/17 11:36 AM, Serge E. Hallyn wrote: >>> Quoting Matt Brown (matt@nmatt.com): >>>> On 6/1/17 5:24 PM, Alan Cox wrote: >>>>>> There's a difference between "bugs" and "security bugs". Letting >>>>> >>>>> Not really, it's merely a matter of severity of result. A non security >>>>> bug that hoses your hard disk is to anyone but security nutcases at >>>>> least as bad as a security hole. >>>>> >>>>>> security bugs continue to get exploited because we want to flush out >>>>>> bugs seems insensitive to the people getting attacked. I'd rather >>>>>> protect against a class of bug than have to endless fix each bug. >>>>> >>>>> The others are security bugs too to varying degree >>>>> >>>>>>> I'm not against doing something to protect the container folks, but that >>>>>>> something as with Android is a whitelist of ioctls. And if we need to do >>>>>>> this with a kernel hook lets do it properly. >>>>>>> >>>>>>> Remember the namespace of the tty on creation >>>>>>> If the magic security flag is set then >>>>>>> Apply a whitelist to *any* tty ioctl call where the ns doesn't >>>>>>> match >>>>>>> >>>>>>> and we might as well just take the Android whitelist since they've kindly >>>>>>> built it for us all! >>>>>>> >>>>>>> In the tty layer it ends up being something around 10 lines of code and >>>>>>> some other file somewhere in security/ that's just a switch or similar >>>>>>> with the whitelisted ioctl codes in it. >>>>>>> >>>>>>> That (or a similar SELinux ruleset) would actually fix the problem. >>>>>>> SELinux would be better because it can also apply the rules when doing >>>>>>> things like su/sudo/... >>>>>> >>>>>> Just to play devil's advocate, wouldn't such a system continue to not >>>>>> address your physical-console concerns? I wouldn't want to limit the >>>>> >>>>> It would for the cases that a whitelist and container check covers - >>>>> because the whitelist wouldn't allow you to do anything but boring stuff >>>>> on the tty. TIOCSTI is just one of a whole range of differently stupid >>>>> and annoying opportunities. Containers do not and should not be able to >>>>> set the keymap, change the video mode, use console selection, make funny >>>>> beepy noises, access video I/O registers and all the other stuff like >>>>> that. Nothing is going to break if we have a fairly conservative >>>>> whitelist. >>>>> >>>>>> protection to only containers (but it's a good start), since it >>>>>> wouldn't protect people not using containers that still have a >>>>>> privileged TTY attached badly somewhere. >>>>> >>>>> How are you going to magically fix the problem. I'm not opposed to fixing >>>>> the real problem but right now it appears to be a product of wishful >>>>> thinking not programming. What's the piece of security code that >>>>> magically discerns the fact you are running something untrusted at the >>>>> other end of your tty. SELinux can do it via labelling but I don't see >>>>> any generic automatic way for the kernel to magically work out when to >>>>> whitelist and when not to. If there is a better magic rule than >>>>> differing-namespace then provide the code. >>>>> >>>>> You can't just disable TIOCSTI, it has users deal with it. You can >>>>> get away with disabling it for namespace crossing I think but if you do >>>>> that you need to disable a pile of others. >>>>> >>>>> (If it breaks containers blocking TIOCSTI then we need to have a good >>>>> look at algorithms for deciding when to flush the input queue on exiting >>>>> a container or somesuch) >>>>> >>>>>> If you're talking about wholistic SELinux policy, sure, I could >>>>>> imagine a wholistic fix. But for the tons of people without a >>>>>> comprehensive SELinux policy, the proposed protection continues to >>>>>> make sense. >>>>> >>>>> No it doesn't. It's completely useless unless you actually bother to >>>>> address the other exploit opportunities. >>>>> >>>>> Right now the proposal is a hack to do >>>>> >>>>> if (TIOCSTI && different_namespace && magic_flag) >>>>> >>>> >>>> >>>> This is not what my patch does. Mine is like: >>>> >>>> if (TIOCSTI && !ns_capable(tty->owner_user_ns, CAP_SYS_ADMIN) && >>>> magic_flag) >>>> >>>> in other words: >>>> if (TIOCSTI && (different_owner_user_ns || !CAP_SYS_ADMIN) && >>>> magic_flag) >>>> >>>> can you specify what you mean by different_namespace? which namespace? >>> >>> I think you're focusing on the wrong thing. Your capable check (apart >>> from the fact that I think I've been convinced CAP_SYS_ADMIN is wrong) >>> is fine. >> > > I'm cc:ing linux-api here because really we're designing an interesting > API. > >> Can't we also have a sysctl that toggles if CAP_SYS_ADMIN is involved in >> this whitelist check? Otherwise someone might leave things out of the >> whitelist just because they want to use those ioctls as a privileged >> process. > > I'm not quite sure what you're asking for here. Let me offer a precise > strawman design. I'm sure there are problems with it, it's just a starting > point. > > system-wide whitelist (for now 'may_push_chars') is full by default. > So is may_push_chars just an alias for TIOCSTI? Or are there some potential whitelist members that would map to multiple ioctls? > By default, nothing changes - you can use those on your own tty, need > CAP_SYS_ADMIN against init_user_ns otherwise. > > Introduce a new CAP_TTY_PRIVILEGED. > I'm fine with this. > When may_push_chars is removed from the whitelist, you lose the ability > to use TIOCSTI on a tty - even your own - if you do not have CAP_TTY_PRIVILEGED > against the tty's user_ns. > How do you propose storing/updating the whitelist? sysctl? If it is a sysctl, would each whitelist member have a sysctl? e.g.: kernel.ioctlwhitelist.may_push_chars = 1 Overall, I'm fine with this idea. >> Also restricting a privileged user from ioctls with this >> whitelist approach is going to be pointless because, if the whitelist is >> configurable from userspace, they will just be able to modify the >> whitelist. >> >>> >>> The key point is to not only check for TIOCSTI, but instead check for >>> a whitelisted ioctl. >>> >>> What would the whitelist look like? Should configuing that be the way >>> that you enable/disable, instead of the sysctl in this patchset? So >>> by default the whitelist includes all ioctls (no change), but things >>> like sandboxes/sudo/container-starts can clear out the whitelist? >>> >> >> I'm fine with moving this to an LSM that whitelists ioctls. I also want > > Right - what else would go into the whitelist? may_mmap? > >> to understand what a whitelist would like look and how you would >> configure it? Does a sysctl that is a list of allowed ioctls work? I >> don't want to just have a static whitelist that you can't change without >> recompiling your kernel. >> >> just running a sysctl -a on a linux box shows me one thing that looks >> like a list: net.core.flow_limit_cpu_bitmap
Quoting Matt Brown (matt@nmatt.com): > On 6/2/17 12:57 PM, Serge E. Hallyn wrote: > > I'm not quite sure what you're asking for here. Let me offer a precise > > strawman design. I'm sure there are problems with it, it's just a starting > > point. > > > > system-wide whitelist (for now 'may_push_chars') is full by default. > > > > So is may_push_chars just an alias for TIOCSTI? Or are there some > potential whitelist members that would map to multiple ioctls? <shrug> I'm seeing it as only TIOCSTI right now. > > By default, nothing changes - you can use those on your own tty, need > > CAP_SYS_ADMIN against init_user_ns otherwise. > > > > Introduce a new CAP_TTY_PRIVILEGED. > > > > I'm fine with this. > > > When may_push_chars is removed from the whitelist, you lose the ability > > to use TIOCSTI on a tty - even your own - if you do not have CAP_TTY_PRIVILEGED > > against the tty's user_ns. > > > > How do you propose storing/updating the whitelist? sysctl? > > If it is a sysctl, would each whitelist member have a sysctl? > e.g.: kernel.ioctlwhitelist.may_push_chars = 1 > > Overall, I'm fine with this idea. That sounds reasonable. Or a securityfs file - I guess not everyone has securityfs, but if it were to become part of YAMA then that would work. -serge
On Thu, Jun 1, 2017 at 3:56 PM, James Morris <jmorris@namei.org> wrote: > On Thu, 1 Jun 2017, Kees Cook wrote: > >> All of the reasoning here seems to match the link restrictions from 5 >> years ago: a crappy API (sticky bit) is not handled by userspace (open >> /tmp/$$!) and people get attacked. The solution was a sysctl to enable >> the link restrictions that killed the entire class of the common >> attack (though it didn't solve especially egregious bad uses, much > > This is the problem -- it doesn't really eliminate the underlying issue. > > A better solution (in this case) was to implement a new API which > addresses the issue at an architectural level, i.e. namespace-based > private /tmp views, and encourage its adoption. I think this is a frequent mistake in evaluating how to kill bug classes: there doesn't have to be only one solution, especially when there are downsides to be dealt with. In the link restriction case, the VFS solution immediately solved the vast majority of issues without breaking any applications. The private /tmp takes time to roll out, and breaks the shared /tmp way of doing things that some tools use to share files, etc. So the private /tmp solves more of the problem (eliminates link attacks in subdirectories of /tmp), but breaks real use-cases. And ultimately, there is nothing incompatible about the solutions, so both could (and were) pursued. >> like the TIOCSTI fix). Every distro enabled the sysctl, and, while the >> data is noisy, looking a CVEs matching "/tmp symlink", the numbers >> drop from 2013 and later (with none yet for 2017). > > I wonder how much of this is due to the sysctl vs. adoption of private > /tmp, and what may be lurking in the "egregious bad uses" category for > future CVEs. And obviously we don't know what various folk may have up > their sleeves, if anything. Private /tmp is still somewhat less common, the VFS changes were rolled out almost universally, so, if the numbers can be trusted at all, I would assume it's the VFS changes. That said, CVE numbers tend to fluctuate based on researcher interest, rather than being a true measure of real-world problems. The anecdata I have while at Ubuntu was having people complain about getting owned by /tmp symlink ToCToU from time to time on the Ubuntu security IRC channel, and after Ubuntu rolled out the VFS changes, that dropped to zero in all the years since. So, I guess, my point is that there are always multiple solutions that come with various benefits and downsides, and when there isn't a perfect solution, imperfect solutions that make real-world changes on attack surface are worth pursuing, even in parallel. Perfection shouldn't be the enemy of the good. (Perfection is preferred, but it's not usuallyt pragmatically possible.) -Kees
On 6/2/17 2:18 PM, Serge E. Hallyn wrote: > Quoting Matt Brown (matt@nmatt.com): >> On 6/2/17 12:57 PM, Serge E. Hallyn wrote: >>> I'm not quite sure what you're asking for here. Let me offer a precise >>> strawman design. I'm sure there are problems with it, it's just a starting >>> point. >>> >>> system-wide whitelist (for now 'may_push_chars') is full by default. >>> >> >> So is may_push_chars just an alias for TIOCSTI? Or are there some >> potential whitelist members that would map to multiple ioctls? > > <shrug> I'm seeing it as only TIOCSTI right now. > >>> By default, nothing changes - you can use those on your own tty, need >>> CAP_SYS_ADMIN against init_user_ns otherwise. >>> >>> Introduce a new CAP_TTY_PRIVILEGED. >>> >> >> I'm fine with this. >> >>> When may_push_chars is removed from the whitelist, you lose the ability >>> to use TIOCSTI on a tty - even your own - if you do not have CAP_TTY_PRIVILEGED >>> against the tty's user_ns. >>> >> >> How do you propose storing/updating the whitelist? sysctl? >> >> If it is a sysctl, would each whitelist member have a sysctl? >> e.g.: kernel.ioctlwhitelist.may_push_chars = 1 >> >> Overall, I'm fine with this idea. > > That sounds reasonable. Or a securityfs file - I guess not everyone > has securityfs, but if it were to become part of YAMA then that would > work. > Yama doesn't depend on securityfs does it? What do other people think? Should this be an addition to YAMA or its own thing? Alan Cox: what do you think of the above ioctl whitelisting scheme?
On Fri, Jun 2, 2017 at 12:22 PM, Matt Brown <matt@nmatt.com> wrote: > On 6/2/17 2:18 PM, Serge E. Hallyn wrote: >> Quoting Matt Brown (matt@nmatt.com): >>> On 6/2/17 12:57 PM, Serge E. Hallyn wrote: >>>> I'm not quite sure what you're asking for here. Let me offer a precise >>>> strawman design. I'm sure there are problems with it, it's just a starting >>>> point. >>>> >>>> system-wide whitelist (for now 'may_push_chars') is full by default. >>>> >>> >>> So is may_push_chars just an alias for TIOCSTI? Or are there some >>> potential whitelist members that would map to multiple ioctls? >> >> <shrug> I'm seeing it as only TIOCSTI right now. >> >>>> By default, nothing changes - you can use those on your own tty, need >>>> CAP_SYS_ADMIN against init_user_ns otherwise. >>>> >>>> Introduce a new CAP_TTY_PRIVILEGED. >>>> >>> >>> I'm fine with this. >>> >>>> When may_push_chars is removed from the whitelist, you lose the ability >>>> to use TIOCSTI on a tty - even your own - if you do not have CAP_TTY_PRIVILEGED >>>> against the tty's user_ns. >>>> >>> >>> How do you propose storing/updating the whitelist? sysctl? >>> >>> If it is a sysctl, would each whitelist member have a sysctl? >>> e.g.: kernel.ioctlwhitelist.may_push_chars = 1 >>> >>> Overall, I'm fine with this idea. >> >> That sounds reasonable. Or a securityfs file - I guess not everyone >> has securityfs, but if it were to become part of YAMA then that would >> work. >> > > Yama doesn't depend on securityfs does it? > > What do other people think? Should this be an addition to YAMA or its > own thing? > > Alan Cox: what do you think of the above ioctl whitelisting scheme? It's easy to stack LSMs, so since Yama is ptrace-focused, perhaps make a separate one for TTY hardening? -Kees
On 6/2/17 3:25 PM, Kees Cook wrote: > On Fri, Jun 2, 2017 at 12:22 PM, Matt Brown <matt@nmatt.com> wrote: >> On 6/2/17 2:18 PM, Serge E. Hallyn wrote: >>> Quoting Matt Brown (matt@nmatt.com): >>>> On 6/2/17 12:57 PM, Serge E. Hallyn wrote: >>>>> I'm not quite sure what you're asking for here. Let me offer a precise >>>>> strawman design. I'm sure there are problems with it, it's just a starting >>>>> point. >>>>> >>>>> system-wide whitelist (for now 'may_push_chars') is full by default. >>>>> >>>> >>>> So is may_push_chars just an alias for TIOCSTI? Or are there some >>>> potential whitelist members that would map to multiple ioctls? >>> >>> <shrug> I'm seeing it as only TIOCSTI right now. >>> >>>>> By default, nothing changes - you can use those on your own tty, need >>>>> CAP_SYS_ADMIN against init_user_ns otherwise. >>>>> >>>>> Introduce a new CAP_TTY_PRIVILEGED. >>>>> >>>> >>>> I'm fine with this. >>>> >>>>> When may_push_chars is removed from the whitelist, you lose the ability >>>>> to use TIOCSTI on a tty - even your own - if you do not have CAP_TTY_PRIVILEGED >>>>> against the tty's user_ns. >>>>> >>>> >>>> How do you propose storing/updating the whitelist? sysctl? >>>> >>>> If it is a sysctl, would each whitelist member have a sysctl? >>>> e.g.: kernel.ioctlwhitelist.may_push_chars = 1 >>>> >>>> Overall, I'm fine with this idea. >>> >>> That sounds reasonable. Or a securityfs file - I guess not everyone >>> has securityfs, but if it were to become part of YAMA then that would >>> work. >>> >> >> Yama doesn't depend on securityfs does it? >> >> What do other people think? Should this be an addition to YAMA or its >> own thing? >> >> Alan Cox: what do you think of the above ioctl whitelisting scheme? > > It's easy to stack LSMs, so since Yama is ptrace-focused, perhaps make > a separate one for TTY hardening? > Sounds good. I also like the idea of them being separate. Matt Brown
> Can't we also have a sysctl that toggles if CAP_SYS_ADMIN is involved in > this whitelist check? Otherwise someone might leave things out of the > whitelist just because they want to use those ioctls as a privileged > process. Also restricting a privileged user from ioctls with this > whitelist approach is going to be pointless because, if the whitelist is > configurable from userspace, they will just be able to modify the > whitelist. Something like CAP_SYS_ADMIN is fine if you must have it configurable on the grounds that CAP_SYS_ADMIN will let you override the list anyway. > I'm fine with moving this to an LSM that whitelists ioctls. I also want > to understand what a whitelist would like look and how you would > configure it? Does a sysctl that is a list of allowed ioctls work? I > don't want to just have a static whitelist that you can't change without > recompiling your kernel. That's odd because your previous argument was for a fixed one entry whitelist containing TIOCSTI 8) The list can probably be fixed. IMHO those wanting to do cleverer stuff sre inevitably going to end up using a "grown up" LSM for the job because a) they'll want to shape things like su not just containers b) they will have cases where you want to lock down cleverly - eg you can disable TIOCSTI use except by certain apps, and make those apps non ptraceable so only existing real users of it can continue to use it. For the whitelist you want most of the standard tty ioctls, but none of the device specific ones, none of the hardware configuration ones, no TIOCSTI, no selection, no line discipline change (or you can set a network ldisc or various other evil and fascinating things). I really think that for container type uses the whitelist can be fairly clearly defined because we know a container isn't supposed to be screwing with the hardware of the parent, configuring network interfaces or pushing data to trip people up. If we are prepared to accept nuisance attacks (messing up baud rate, hangup, etc) then it's fairly easy to fix. So I'd say it's all the generic tty ioctls except TIOCSTI and TIOCSETD but it would be good to see what Android is going with and why. You can still do some typeback stuff even then because the consoles and terminals have ranges of query and requests you can trigger. As you can use termios to absorb some symbols in the reply and map which one to use for newline you can even type stuff. However it's very limited - hex digits, [, c and some other oddments. People have looked hard at that and I've yet to see a successful attack. Yes I can make you run test (aka '[') but I've yet to see a way to use it for anything. Alan
On Fri, Jun 2, 2017 at 1:05 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: > So I'd say it's all the generic tty ioctls except TIOCSTI and TIOCSETD > but it would be good to see what Android is going with and why. Android limits tty ioctls to the following whitelist: TIOCOUTQ FIOCLEX TCGETS TCSETS TIOCGWINSZ TIOCSWINSZ TIOCSCTTY TCSETSW TCFLSH TIOCSPGRP TIOCGPGRP See unpriv_tty_ioctls at https://android.googlesource.com/platform/system/sepolicy/+/34b4b73729b288b4109b2225c1445eb58393b8cb/public/ioctl_macros#51
On 6/2/17 4:05 PM, Alan Cox wrote: >> Can't we also have a sysctl that toggles if CAP_SYS_ADMIN is involved in >> this whitelist check? Otherwise someone might leave things out of the >> whitelist just because they want to use those ioctls as a privileged >> process. Also restricting a privileged user from ioctls with this >> whitelist approach is going to be pointless because, if the whitelist is >> configurable from userspace, they will just be able to modify the >> whitelist. > > Something like CAP_SYS_ADMIN is fine if you must have it configurable on > the grounds that CAP_SYS_ADMIN will let you override the list anyway. > >> I'm fine with moving this to an LSM that whitelists ioctls. I also want >> to understand what a whitelist would like look and how you would >> configure it? Does a sysctl that is a list of allowed ioctls work? I >> don't want to just have a static whitelist that you can't change without >> recompiling your kernel. > > That's odd because your previous argument was for a fixed one entry > whitelist containing TIOCSTI 8) > I just take some convincing that's all ;) > The list can probably be fixed. IMHO those wanting to do cleverer stuff > sre inevitably going to end up using a "grown up" LSM for the job because > I really want it to be more flexible if we are making this into a full blown LSM. From drivers/tty/tty_ioctl.c I gather the following tty ioctls: TCGETA TCGETS TCGETS2 TCGETX TCSETA TCSETAF TCSETAW TCSETS TCSETS2 TCSETSF TCSETSF2 TCSETSW TCSETSW2 TCSETX TCSETXF TCSETXW TIOCGETC TIOCGETP TIOCGLCKTRMIOS TIOCGLTC TIOCGSOFTCAR TIOCSETC TIOCSETN TIOCSETP TIOCSLCKTRMIOS TIOCSLTC TIOCSSOFTCAR would it be overkill to have a sysctl kernel.ttyioctlwhitelist.X where X is one of the ioctls above? (definitely will work on a better name than ttyioctlwhitelist) > a) they'll want to shape things like su not just containers > b) they will have cases where you want to lock down cleverly - eg you can > disable TIOCSTI use except by certain apps, and make those apps non > ptraceable so only existing real users of it can continue to use it. I agree. When you have those kinds of requirements, a MAC is required. > > For the whitelist you want most of the standard tty ioctls, but none of > the device specific ones, none of the hardware configuration ones, no > TIOCSTI, no selection, no line discipline change (or you can set a > network ldisc or various other evil and fascinating things). > > I really think that for container type uses the whitelist can be fairly > clearly defined because we know a container isn't supposed to be screwing > with the hardware of the parent, configuring network interfaces or > pushing data to trip people up. > > If we are prepared to accept nuisance attacks (messing up baud rate, > hangup, etc) then it's fairly easy to fix. > > So I'd say it's all the generic tty ioctls except TIOCSTI and TIOCSETD > but it would be good to see what Android is going with and why. > > You can still do some typeback stuff even then because the consoles and > terminals have ranges of query and requests you can trigger. As you can > use termios to absorb some symbols in the reply and map which one to use > for newline you can even type stuff. However it's very limited - hex > digits, [, c and some other oddments. People have looked hard at that and > I've yet to see a successful attack. Yes I can make you run test (aka > '[') but I've yet to see a way to use it for anything. > > Alan >
> TIOCSLCKTRMIOS That one I'm more dubious about > TIOCSLTC > TIOCSSOFTCAR tty_io.c also has a few and n_tty has a couple we'd want. > > would it be overkill to have a sysctl kernel.ttyioctlwhitelist.X where X > is one of the ioctls above? Why would anyone want to change the entries on that list Alan
On 06/03/2017 06:00 PM, Alan Cox wrote: >> TIOCSLCKTRMIOS > > That one I'm more dubious about > >> TIOCSLTC >> TIOCSSOFTCAR > > tty_io.c also has a few and n_tty has a couple we'd want. > >> >> would it be overkill to have a sysctl kernel.ttyioctlwhitelist.X where X >> is one of the ioctls above? > > Why would anyone want to change the entries on that list > Did you see Serge's proposed solution? I want us to not be talking past each other. Serge proposed the following: | By default, nothing changes - you can use those on your own tty, need | CAP_SYS_ADMIN against init_user_ns otherwise. | | Introduce a new CAP_TTY_PRIVILEGED. | | When may_push_chars is removed from the whitelist, you lose the | ability to use TIOCSTI on a tty - even your own - if you do not have | CAP_TTY_PRIVILEGED against the tty's user_ns. The question is how do you add/remove something from this whitelist? I assume by add/remove we don't mean that you have to recompile your kernel to change the whitelist! you earlier said you wanted the check to look like this: | if (!whitelisted(ioctl) && different_namespace && magic_flag) I want to know which namespace you are talking about here. Did you mean user_namespace? (the namespace I added tracking for in the tty_struct)
On Sun, Jun 4, 2017 at 8:22 AM, Matt Brown <matt@nmatt.com> wrote: > On 06/03/2017 06:00 PM, Alan Cox wrote: >>> >>> TIOCSLCKTRMIOS >> >> >> That one I'm more dubious about >> >>> TIOCSLTC >>> TIOCSSOFTCAR >> >> >> tty_io.c also has a few and n_tty has a couple we'd want. >> >>> >>> would it be overkill to have a sysctl kernel.ttyioctlwhitelist.X where X >>> is one of the ioctls above? >> >> >> Why would anyone want to change the entries on that list >> > > Did you see Serge's proposed solution? I want us to not be talking past > each other. Serge proposed the following: > > | By default, nothing changes - you can use those on your own tty, need > | CAP_SYS_ADMIN against init_user_ns otherwise. > | > | Introduce a new CAP_TTY_PRIVILEGED. > | > | When may_push_chars is removed from the whitelist, you lose the > | ability to use TIOCSTI on a tty - even your own - if you do not have > | CAP_TTY_PRIVILEGED against the tty's user_ns. > > The question is how do you add/remove something from this whitelist? I > assume by add/remove we don't mean that you have to recompile your > kernel to change the whitelist! > > you earlier said you wanted the check to look like this: > > | if (!whitelisted(ioctl) && different_namespace && magic_flag) > > I want to know which namespace you are talking about here. Did you mean > user_namespace? (the namespace I added tracking for in the tty_struct) There are many ways to attempt to cure this problem. They some that are just wrong. Pushing stuff up to CAP_SYS_ADMIN is fairly much always wrong. Using a whitelisted solution does have a downside but to use some application that use TIOCSTI safely I have not had to push application to CAP_SYS_ADMIN. Another question due to the way the exploit work a broken TIOCSTI where push back could be something someone as CAP_SYS_ADMIN run. What I don't know if yet set when ever an application used TIOCSTI to push back chars back into input that this would set input to be flushed on tty disconnect or application termination would this break any applications. So it may be possible to allow applications to freely use TIOCSTI just make sure that anything an application has pushed back into input buffer cannot get to anything else. The thing to remember is most times when applications are controlling other applications they are not pushing data backwards on input.. Question I have is what is valid usage cases of TIOCSTI. Thinking grscecurity got away with pushing this up to CAP_SYS_ADMIN there may not be many. If there is no valid usage of TIOCSTI across applications there is no reason why TIOCSTI cannot be setup to automatically trigger input flushs to prevent TIOCSTI inserted data getting anywhere. . This could be like X11 and it huge number of features where large number were found that no one ever used just was created that way because it was though like it would be useful. My problem here is TIOCSTI might not need a flag at all. TIOCSTI functionality maybe in need of limitations particularly if TIOCSTI push back into input cross from one application to the next has no genuine application usage. So far no one has started that exploited TIOCSTI functionality exists in any genuine application as expected functionality. I cannot find example of where pushing back into input then going to background or dieing/exiting and having that pushed back input processed is done by any genuine application as expected functionality. That is something that could be limited if there is no genuine users and close the door without having to modify existing applications that don't expect to-do that. Its really simple to get focused in on quick fix to problems without asking is the behaviour even required. Peter Dolding
On Mon, May 29, 2017 at 8:27 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 5/29/2017 4:51 PM, Boris Lukashev wrote: >> With all due respect sir, i believe your review falls short of the >> purpose of this effort - to harden the kernel against flaws in >> userspace. Comments along the line of "if <userspace> does it right >> then your patch is pointless" are not relevant to the context of >> securing kernel functions/interfaces. What userspace should do has >> little bearing on defensive measures actually implemented in the >> kernel - if we took the approach of "someone else is responsible for >> that" in military operations, the world would be a much darker and >> different place today. Those who have the luxury of standoff from the >> critical impacts of security vulnerabilities may not take into account >> the fact that peoples lives literally depend on Linux getting a lot >> more secure, and quickly. > > You are not going to help anyone with a kernel configuration that > breaks agetty, csh, xemacs and tcsh. The opportunities for using > such a configuration are limited. > First off, thank you for the clear and rational responses - newbie status in these upstreaming/kernel matters has a bit of a steep learning curve. I would however, have to disagree with the above in that there is a very large number of purpose built Linux systems in play, home routers being a good example, which effectively retain the same security posture over their lifetime in an increasingly hostile operating environment. Mitigation at every tier of the attack cycle is desirable as a (configurable) default such as to at least reduce the impact of compromise (they may leak memory containing the admin cred for the web ui via some protocol error, but dont get shells, local privs, and raw device access). Then there are all the servers out there which would benefit from this, as they dont use those components but do expose TTYs to dangerous consumers, and even the workstations in a similar boat where they dont depend on faulty consumers but are operated by faulty users. Devil's in the details right? Ideally, if properly designed with input from greybeards, faults can be mostly nullified and edge cases addressed with adjacent maintainers. >> If this work were not valuable, it wouldnt be an enabled kernel option >> on a massive number of kernels with attack surfaces reduced by the >> compound protections offered by the grsec patch set. > > I'll bet you a beverage that 99-44/100% of the people who have > this enabled have no clue that it's even there. And if they did, > most of them would turn it off. > Wouldn't dream of taking that bet - the vast majority of Linux users dont even know they're Linux users :). Disagree with the latter part though, the majority of people rely on the appropriate organs of society (military, police, security-focused devs) to provide for their safety; they not only take for granted but largely abide by the constraints placed by those in the know such as to ensure that safety. A cynical example being that you may not know exactly what the stuff in that hazmat-labled container will do to you (may make you into Wolverine, or melt your bones), but you're not likely to open it and take a sip - you trust the people who sealed it to know enough of the details to have made that decision. Implementing changes like this (properly scoped and implemented, as the refinement process seems to be doing) makes a lot of sense top-down as it forces consumers to do things the right way instead of waiting for them before adopting a useful function. >> I can't speak for >> the grsec people, but having read a small fraction of the commentary >> around the subject of mainline integration, it seems to me that NAKs >> like this are exactly why they had no interest in even trying - this >> review is based on the cultural views of the kernel community, not on >> the security benefits offered by the work in the current state of >> affairs (where userspace is broken). > > A security clamp-down that breaks important stuff is going > to have a tough row to hoe going upstream. Same with a performance > enhancement that breaks things. > Back to the newbie status bit, i've read and heard (in varying degrees of satisfaction and frustration) that upstream makes it hard to adopt significant changes as a culture. Obviously there are practical concerns around compatibility, but there must be some avenue to have a clear and rational discussion with Olympus about inducing a well planned and short period of churn to affect changes which will greatly extend the iconic notion of systems running stable and safe for years at a time... In practical terms, distributions ship tons of patches for kernel bugs faster than you can reload a blunderbuss. While that is good practice, and should continue, it results in constant operating efforts on the parts of the consumer having to "manage their updates" since the bug isn't restricted in access vectors or efficacy by a global defensive measure, but requires direct patching to mitigate. Compliance beholden systems, which tend to be in the critical path, suffer obligatory burdens from the status quo - find me a hospital IT security manager who hasn't thought of running (himself or his boss) head first through his office window since the start of the quarter, and i'll betcha he started work Friday. A "security clamp-down" would be great if it were well coordinated with the ecosystem such as to have bugs found by compile-time approaches immediately addressed by their owning maintainers, quickly spread use of read-only and randomized structures, memory allocations, or whatever other defenses are implemented at the core/LSM infrastructure tiers. Once the large changes are in, things go back to business-as-usual, but without the ops engineers being restricted to eating with dull plastic spoons. >> The purpose of each of these >> protections (being ported over from grsec) is not to offer carte >> blanche defense against all attackers and vectors, but to prevent >> specific classes of bugs from reducing the security posture of the >> system. By implementing these defenses in a layered manner we can >> significantly reduce our kernel attack surface. > > Sure, but they have to work right. That's an important reason to do > small changes. A change that isn't acceptable can be rejected without > slowing the general progress. > Understood and agreed - getting the right scope and constraints for the job is imperative for proper design and implementation of the solution. As i'm reading the evolution of this thread i'm learning how submissions evolve to fit the needs defined by members. Sort of rare to see a process involving multiple participants these days which hasn't devolved into a sandy goat-roping procedure. >> Once userspace catches >> up and does things the right way, and has no capacity for doing them >> the wrong way (aka, nothing attackers can use to bypass the proper >> userspace behavior), then the functionality really does become >> pointless, and can then be removed. > > Well, until someone comes along with yet another spiffy feature > like containers and breaks it again. This is why a really good > solution is required, and the one proposed isn't up to snuff. > >> >From a practical perspective, can alternative solutions be offered >> along with NAKs? > > They often are, but let's face it, not everyone has the time, > desire and/or expertise to solve every problem that comes up. > >> Killing things on the vine isnt great, and if a >> security measure is being denied, upstream should provide their >> solution to how they want to address the problem (or just an outline >> to guide the hardened folks). > > The impact of a "security measure" can exceed the value provided. > That is, I understand, the basis of the NAK. We need to be careful > to keep in mind that, until such time as there is substantial interest > in the sort of systemic changes that truly remove this class of issue, > we're going to have to justify the risk/reward trade off when we try > to introduce a change. > Here i again, have to respectfully disagree. Waiting on "significant interest" to patch their workstations and servers is what got a chunk of the world looking for backups and coughing up bitcoins recently. If there is a viable threat model, it should be addressed proactively, as opposed to potential operating bugs which are shaken out in the course of ops (which is how i understand current triage to work for bugs - once its found, not potentially because conditions could create it). Security as an afterthought can be seen as a form of complacency, and as we've all read - "complacency kills." >> >> On Mon, May 29, 2017 at 6:26 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote: >>> On Mon, 29 May 2017 17:38:00 -0400 >>> Matt Brown <matt@nmatt.com> wrote: >>> >>>> This introduces the tiocsti_restrict sysctl, whose default is controlled >>>> via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this control >>>> restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. >>> Which is really quite pointless as I keep pointing out and you keep >>> reposting this nonsense. >>> >>>> This patch depends on patch 1/2 >>>> >>>> This patch was inspired from GRKERNSEC_HARDEN_TTY. >>>> >>>> This patch would have prevented >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following >>>> conditions: >>>> * non-privileged container >>>> * container run inside new user namespace >>> And assuming no other ioctl could be used in an attack. Only there are >>> rather a lot of ways an app with access to a tty can cause mischief if >>> it's the same controlling tty as the higher privileged context that >>> launched it. >>> >>> Properly written code allocates a new pty/tty pair for the lower >>> privileged session. If the code doesn't do that then your change merely >>> modifies the degree of mayhem it can cause. If it does it right then your >>> patch is pointless. >>> >>>> Possible effects on userland: >>>> >>>> There could be a few user programs that would be effected by this >>>> change. >>> In other words, it's yet another weird config option that breaks stuff. >>> >>> >>> NAK v7. >>> >>> Alan >> >> >
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt index bac23c1..f7985cf 100644 --- a/Documentation/sysctl/kernel.txt +++ b/Documentation/sysctl/kernel.txt @@ -89,6 +89,7 @@ show up in /proc/sys/kernel: - sysctl_writes_strict - tainted - threads-max +- tiocsti_restrict - unknown_nmi_panic - watchdog - watchdog_thresh @@ -987,6 +988,26 @@ available RAM pages threads-max is reduced accordingly. ============================================================== +tiocsti_restrict: + +This toggle indicates whether unprivileged users are prevented +from using the TIOCSTI ioctl to inject commands into other processes +which share a tty session. + +When tiocsti_restrict is set to (0) there are no restrictions(accept +the default restriction of only being able to injection commands into +one's own tty). When tiocsti_restrict is set to (1), users must +have CAP_SYS_ADMIN to use the TIOCSTI ioctl. + +When user namespaces are in use, the check for the capability +CAP_SYS_ADMIN is done against the user namespace that originally +opened the tty. + +The kernel config option CONFIG_SECURITY_TIOCSTI_RESTRICT sets the +default value of tiocsti_restrict. + +============================================================== + unknown_nmi_panic: The value in this file affects behavior of handling NMI. When the diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index c276814..0f2733d 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -2297,11 +2297,19 @@ static int tty_fasync(int fd, struct file *filp, int on) * FIXME: may race normal receive processing */ +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); + static int tiocsti(struct tty_struct *tty, char __user *p) { char ch, mbz = 0; struct tty_ldisc *ld; + if (tiocsti_restrict && + !ns_capable(tty->owner_user_ns, CAP_SYS_ADMIN)) { + dev_warn_ratelimited(tty->dev, + "Denied TIOCSTI ioctl for non-privileged process\n"); + return -EPERM; + } if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) return -EPERM; if (get_user(ch, p)) diff --git a/include/linux/tty.h b/include/linux/tty.h index d902d42..2fd7f49 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -344,6 +344,8 @@ struct tty_file_private { struct list_head list; }; +extern int tiocsti_restrict; + /* tty magic number */ #define TTY_MAGIC 0x5401 diff --git a/kernel/sysctl.c b/kernel/sysctl.c index acf0a5a..68d1363 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -67,6 +67,7 @@ #include <linux/kexec.h> #include <linux/bpf.h> #include <linux/mount.h> +#include <linux/tty.h> #include <linux/uaccess.h> #include <asm/processor.h> @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { .extra2 = &two, }, #endif +#if defined CONFIG_TTY + { + .procname = "tiocsti_restrict", + .data = &tiocsti_restrict, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = proc_dointvec_minmax_sysadmin, + .extra1 = &zero, + .extra2 = &one, + }, +#endif { .procname = "ngroups_max", .data = &ngroups_max, diff --git a/security/Kconfig b/security/Kconfig index 823ca1a..665b610 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT If you are unsure how to answer this question, answer N. +config SECURITY_TIOCSTI_RESTRICT + bool "Restrict unprivileged use of tiocsti command injection" + default n + help + This enforces restrictions on unprivileged users injecting commands + into other processes which share a tty session using the TIOCSTI + ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. + + If this option is not selected, no restrictions will be enforced + unless the tiocsti_restrict sysctl is explicitly set to (1). + + If you are unsure how to answer this question, answer N. + config SECURITY bool "Enable different security models" depends on SYSFS