Message-ID: <CALCETrWn2zB2reYjXbA5iUJpwwZ-0S2tr3Q1nS2+Z-Rcir2udQ@mail.gmail.com>
Date:	Tue, 19 May 2015 10:21:44 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
Cc:	Andy Lutomirski <luto@...nel.org>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	Andrew Morton <akpm@...uxfoundation.org>,
	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
	"Ted Ts'o" <tytso@....edu>, "Andrew G. Morgan" <morgan@...nel.org>,
	Linux API <linux-api@...r.kernel.org>,
	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	Austin S Hemmelgarn <ahferroin7@...il.com>,
	linux-security-module <linux-security-module@...r.kernel.org>,
	Aaron Jones <aaronmdjones@...il.com>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Markku Savela <msa@...h.iki.fi>,
	Kees Cook <keescook@...omium.org>,
	Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH man-pages v2] capabilities.7, prctl.2: Document ambient capabilities

On Tue, May 19, 2015 at 12:56 AM, Michael Kerrisk (man-pages)
<mtk.manpages@...il.com> wrote:
> Hi Andy,
>
> Thanks for this patch. There are some broken pieces though. Also,
> I have some minor questions about the API design. See below.
>
> On 05/15/2015 08:43 AM, Andy Lutomirski wrote:
>> Signed-off-by: Andy Lutomirski <luto@...nel.org>
>> ---
>>
>> There was no v1.  I'm calling this v2 to keep it in sync with the kernel
>> patch versioning.
>>
>>  man2/prctl.2        | 10 ++++++++++
>>  man7/capabilities.7 | 32 ++++++++++++++++++++++++++------
>>  2 files changed, 36 insertions(+), 6 deletions(-)
>>
>> diff --git a/man2/prctl.2 b/man2/prctl.2
>> index b352f6283624..5861e3aefe9a 100644
>> --- a/man2/prctl.2
>> +++ b/man2/prctl.2
>> @@ -949,6 +949,16 @@ had been called.
>>  For further information on Intel MPX, see the kernel source file
>>  .IR Documentation/x86/intel_mpx.txt .
>>  .\"
>> +.TP
>> +.BR PR_CAP_AMBIENT " (since Linux 4.2)"
>> +Reads or changes the ambient capability set.  If arg2 is PR_CAP_AMBIENT_RAISE,
>> +then the capability specified in arg3 is added to the ambient set.  This will
>> +fail, returning EPERM, if the capability is not already both permitted and
>> +inheritable or if the SECBIT_NO_CAP_AMBIENT_RAISE securebit is set.  If arg2
>> +is PR_CAP_AMBIENT_LOWER, then the capability specified in arg3 is removed
>> +from the ambient set.  If arg2 is PR_CAP_AMBIENT_GET, then
>> +.BR prctl (2)
>> +will return 1 if the capability in arg3 is in the ambient set and 0 if not.
>
> Some API design questions:
>
> 1. We already have prctl() operations that work on some capability sets:
>    PR_CAPBSET_READ and PR_CAPBSET_DROP. These don't use arg3; the operation
>    is directly encoded in the first argument of prctl(). Just to keep some
>    consistency, why not do things the same way for these new operations?

I'm torn.  On the one hand, consistency is nice.  On the other hand,
prctl is a mess and trying to organize new additions seems like a good
idea.

>
>    Also, you could opt for some consistency in the naming, so using "READ"
>    rather than "GET", for example. On the other hand, both "READ" and "GET"
>    are suboptimal names: this is really a test operation. So, maybe a
>    clean break to a good name, PR_CAP_AMBIENT_IS_SET, is best?

I like IS_SET.

>
>    Thus:
>
>      prctl(PR_CAP_AMBIENT_READ, cap, 0, 0, 0);  // or PR_CAP_AMBIENT_IS_SET?
>      prctl(PR_CAP_AMBIENT_RAISE, cap, 0, 0, 0);
>      prctl(PR_CAP_AMBIENT_LOWER, cap, 0, 0, 0);
>
> 2. In terms of the API design, would it be useful to have a prctl() operation
>    that clears the entire ambient set?
>
>      prctl(PR_CAP_AMBIENT_ZERO, 0, 0, 0, 0);    // or PR_CAP_AMBIENT_EMPTY?

Seems like a good idea.  How about _CLEAR?

>
>>  .SH RETURN VALUE
>>  On success,
>>  .BR PR_GET_DUMPABLE ,
>> diff --git a/man7/capabilities.7 b/man7/capabilities.7
>> index d75ec65de05b..dae62f0be3b7 100644
>> --- a/man7/capabilities.7
>> +++ b/man7/capabilities.7
>> @@ -697,13 +697,26 @@ a program whose associated file capabilities grant that capability).
>>  .IR Inheritable :
>>  This is a set of capabilities preserved across an
>>  .BR execve (2).
>> -It provides a mechanism for a process to assign capabilities
>> -to the permitted set of the new program during an
>> -.BR execve (2).
>> +Inheritable capabilities remain inheritable when executing any program,
>> +and inheritable capabilities are added to the permitted set when executing
>> +a program that has the corresponding bits set in the file inheritable set.
>> +When executing programs without file capabilities, ambient capabilities
>
> That last line is incomplete. Something needs adding/removing.

Whoops, will fix.

>
>>  .TP
>>  .IR Effective :
>>  This is the set of capabilities used by the kernel to
>>  perform permission checks for the thread.
>> +.TP
>> +.IR Ambient " (since Linux 4.2) :"
>
> Minor knit: s/ :/:/ for next version.

Will fix.

>
>> +This is a set of capabilities that are preserved across an
>> +.BR execve (2)
>> +of a program that does not have file capabilities.  The ambient capability
>> +set obeys the invariant that no capability can ever be ambient if it is
>> +not both permitted and inheritable.  Ambient capabilities are, with some
>> +exceptions, preserved in the permitted set and added to the effective
>> +set when
>> +.BR execve (2)
>> +is called.  The ambient capability set is modified using
>> +.BR prctl (2).
>
> I think it would be helpful to add a couple of sentences here on why the
> ambient set is useful (i.e., explain what deficiencies in the pre-existing
> API are addressed by the addition of this set--a brief piece from your
> 1/2 patch, for example).

Will do.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists