Message-ID: <CANA3KFX37=WEVXedSGAqMH_DxS=SxtjpeZ0PRizPQcBSyDqVpw@mail.gmail.com>
Date:   Wed, 17 May 2017 07:43:18 +1000
From:   Peter Dolding <oiaohm@...il.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Matt Brown <matt@...tt.com>, Alan Cox <gnomes@...rguk.ukuu.org.uk>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jann Horn <jannh@...gle.com>, James Morris <jmorris@...ei.org>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

On Wed, May 17, 2017 at 12:28 AM, Kees Cook <keescook@...omium.org> wrote:
> On Tue, May 16, 2017 at 5:22 AM, Matt Brown <matt@...tt.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.

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.

Powered by blists - more mailing lists