2019-09-26 09:53:44

by Jérémie Galarneau

[permalink] [raw]
Subject: Unmerged patches adding audit when protected_regular/fifos sysctl causes EACCES

Hi Kees,

I have noticed that the two top-most patches of your protected-creat
branch were never merged upstream [1]. Those patches add audit logs
whenever the protected_regular or protected_fifo sysctl prevent the
creation of a file/fifo.

They were mentioned in the v4 thread [2] of the "main" patch and
seemed acceptable, but they were no longer mentioned in v5 [3], which
was merged.

Now that systemd enables those sysctls by default (v241+), I got
bitten pretty hard by this check and it took me a while to figure out
what was happening [4]. I ended up catching it by adding a bunch of
printk(), including where you proposed to add an audit log statement.

I just found your two patches while implementing what you proposed almost 1:1.

Was there a reason why those were abandoned? Otherwise, would you mind
resubmitting them?

Thanks!
Jérémie

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/userspace/protected-creat
[2] https://lkml.org/lkml/2018/4/10/840
[3] https://lore.kernel.org/lkml/20180416175918.GA13494@beast/
[4] https://github.com/lttng/lttng-tools/commit/cf86ff2c4ababd01fea7ab2c9c289cb7c0a1bcd5

--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com


2019-09-26 09:57:17

by Kees Cook

[permalink] [raw]
Subject: Re: Unmerged patches adding audit when protected_regular/fifos sysctl causes EACCES

On Wed, Sep 25, 2019 at 02:58:28PM -0400, J?r?mie Galarneau wrote:
> Hi Kees,
>
> I have noticed that the two top-most patches of your protected-creat
> branch were never merged upstream [1]. Those patches add audit logs
> whenever the protected_regular or protected_fifo sysctl prevent the
> creation of a file/fifo.
>
> They were mentioned in the v4 thread [2] of the "main" patch and
> seemed acceptable, but they were no longer mentioned in v5 [3], which
> was merged.
>
> Now that systemd enables those sysctls by default (v241+), I got
> bitten pretty hard by this check and it took me a while to figure out
> what was happening [4]. I ended up catching it by adding a bunch of
> printk(), including where you proposed to add an audit log statement.
>
> I just found your two patches while implementing what you proposed almost 1:1.
>
> Was there a reason why those were abandoned? Otherwise, would you mind
> resubmitting them?

Hi!

There was concern about getting buy-in from the audit folks delaying
things even more. Instead of waiting for that, as it had already taken
a long time to get consensus even on the functionality, they were
dropped.

I'll rebase them and send them out again; thanks for the ping!

-Kees

>
> Thanks!
> J?r?mie
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/userspace/protected-creat
> [2] https://lkml.org/lkml/2018/4/10/840
> [3] https://lore.kernel.org/lkml/20180416175918.GA13494@beast/
> [4] https://github.com/lttng/lttng-tools/commit/cf86ff2c4ababd01fea7ab2c9c289cb7c0a1bcd5
>
> --
> J?r?mie Galarneau
> EfficiOS Inc.
> http://www.efficios.com

--
Kees Cook

2019-09-26 09:59:47

by Mathieu Desnoyers

[permalink] [raw]
Subject: Re: Unmerged patches adding audit when protected_regular/fifos sysctl causes EACCES

----- On Sep 25, 2019, at 4:12 PM, Kees Cook [email protected] wrote:

> On Wed, Sep 25, 2019 at 02:58:28PM -0400, Jérémie Galarneau wrote:
>> Hi Kees,
>>
>> I have noticed that the two top-most patches of your protected-creat
>> branch were never merged upstream [1]. Those patches add audit logs
>> whenever the protected_regular or protected_fifo sysctl prevent the
>> creation of a file/fifo.
>>
>> They were mentioned in the v4 thread [2] of the "main" patch and
>> seemed acceptable, but they were no longer mentioned in v5 [3], which
>> was merged.
>>
>> Now that systemd enables those sysctls by default (v241+), I got
>> bitten pretty hard by this check and it took me a while to figure out
>> what was happening [4]. I ended up catching it by adding a bunch of
>> printk(), including where you proposed to add an audit log statement.
>>
>> I just found your two patches while implementing what you proposed almost 1:1.
>>
>> Was there a reason why those were abandoned? Otherwise, would you mind
>> resubmitting them?
>
> Hi!
>
> There was concern about getting buy-in from the audit folks delaying
> things even more. Instead of waiting for that, as it had already taken
> a long time to get consensus even on the functionality, they were
> dropped.
>
> I'll rebase them and send them out again; thanks for the ping!

If you need additional justification for why those are needed, here are
a few problematic scenarios we're observing in the current situation.
Feel free to use those if you need to add extra justification for your
audit patches commit messages.

A first scenario is a host with containers, where a container runs
userspace processes which depend on the open() behavior changed
by those sysctl. If the host is updated to systemd 241+, which enables
those sysctl by default, those containers will start misbehaving, and
figuring out the culprit without any hint in the kernel dmesg is far
from obvious.

A similar situation happens for non-containerized deployments. If an
application depends on this open() ABI behavior tweaked by those sysctl,
the application will start failing if it happens to run on a system
with systemd 241+. Again, without any dmesg printout, it's rather hard
to diagnose.

Thanks!

Mathieu


>
> -Kees
>
>>
>> Thanks!
>> Jérémie
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/userspace/protected-creat
>> [2] https://lkml.org/lkml/2018/4/10/840
>> [3] https://lore.kernel.org/lkml/20180416175918.GA13494@beast/
>> [4]
>> https://github.com/lttng/lttng-tools/commit/cf86ff2c4ababd01fea7ab2c9c289cb7c0a1bcd5
>>
>> --
>> Jérémie Galarneau
>> EfficiOS Inc.
>> http://www.efficios.com
>
> --
> Kees Cook

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com