Re: [PATCH] keyrings: Allow searching the user session keyring

[Gwendal Grignou]

On Tue, Jun 14, 2016 at 2:46 AM, David Howells  wrote:
> Gwendal Grignou  wrote:
>
>> Currently, if a session keyring exists, we are not searching in the
>> user session or user keyrings.
>
> That is correct.  New session keyrings are given a link to the user session if
> created by pam_keyinit.  If you don't want to search the user keyring, you can
> just unlink it from your session keyring.
The problem I am facing is that ecrytpfs library (see
https://github.com/dustinkirkland/ecryptfs-utils/blob/master/src/libecryptfs/key_management.c,
function ecryptfs_add_auth_tok_to_keyring) specifically adds the
needed keys to user keyring.
Without the patch above, this code stops working when a session
keyring exists, because the kernel will not search within the user
keyring.
>
> The uid 0 user-session keyring is a potential
> security hole because it allows implicit sharing of authentication data
> between daemon processes.
For ecryptfs, multiple root processes needs to access the key. For
mitigating security risk, we run root daemons that don't need the key
in thin container (called minijail).
>
> David

Re: [PATCH] keyrings: Allow searching the user session keyring

[Gwendal Grignou]

On Tue, Jun 14, 2016 at 2:46 AM, David Howells  wrote:
> Gwendal Grignou  wrote:
>
>> Currently, if a session keyring exists, we are not searching in the
>> user session or user keyrings.
>
> That is correct.  New session keyrings are given a link to the user session if
> created by pam_keyinit.  If you don't want to search the user keyring, you can
> just unlink it from your session keyring.
The problem I am facing is that ecrytpfs library (see
https://github.com/dustinkirkland/ecryptfs-utils/blob/master/src/libecryptfs/key_management.c,
function ecryptfs_add_auth_tok_to_keyring) specifically adds the
needed keys to user keyring.
Without the patch above, this code stops working when a session
keyring exists, because the kernel will not search within the user
keyring.
>
> The uid 0 user-session keyring is a potential
> security hole because it allows implicit sharing of authentication data
> between daemon processes.
For ecryptfs, multiple root processes needs to access the key. For
mitigating security risk, we run root daemons that don't need the key
in thin container (called minijail).
>
> David

Re: [PATCH] keyrings: Allow searching the user session keyring

[David Howells]

Gwendal Grignou  wrote:

> Currently, if a session keyring exists, we are not searching in the
> user session or user keyrings.

That is correct.  New session keyrings are given a link to the user session if
created by pam_keyinit.  If you don't want to search the user keyring, you can
just unlink it from your session keyring.

The user-session keyring is a fallback keyring in case there's no session
keyring.  It seemed to make things easier at the time, but it shouldn't really
exist and I would deprecate it and remove it if I could - especially now that
persistent keyrings exist.  The uid 0 user-session keyring is a potential
security hole because it allows implicit sharing of authentication data
between daemon processes.

David

Re: [PATCH] keyrings: Allow searching the user session keyring

[David Howells]

Gwendal Grignou  wrote:

> Currently, if a session keyring exists, we are not searching in the
> user session or user keyrings.

That is correct.  New session keyrings are given a link to the user session if
created by pam_keyinit.  If you don't want to search the user keyring, you can
just unlink it from your session keyring.

The user-session keyring is a fallback keyring in case there's no session
keyring.  It seemed to make things easier at the time, but it shouldn't really
exist and I would deprecate it and remove it if I could - especially now that
persistent keyrings exist.  The uid 0 user-session keyring is a potential
security hole because it allows implicit sharing of authentication data
between daemon processes.

David

Re: [PATCH] keyrings: Allow searching the user session keyring

[Serge E. Hallyn]

Sorry, I just don't know al lthe subtleties there, I think we just need
David's ack/nack here.

Quoting Gwendal Grignou (gwen...@google.com):
> Any feedback on this? It is mandatory if we want to mount a ecryptfs
> directory while the session keyring is used.
> 
> Thanks,
> Gwendal.
> 
> On Thu, Mar 17, 2016 at 10:04 AM, Gwendal Grignou  
> wrote:
> > Resent to a larger audience.
> >
> > On Thu, Mar 10, 2016 at 2:20 PM, Gwendal Grignou  
> > wrote:
> >> Currently, if a session keyring exists, we are not searching in the
> >> user session or user keyrings.
> >>
> >> This is a problem when a session keyring exists and we want to use
> >> ecryptfs, who adds the needed key only in the user keyring.
> >>
> >> TEST=Without this change, mounting an ecryptfs "partition" fails when a
> >> session keyring exists:
> >> ...
> >> [ 2686.047522] Could not find key with description: [dd6f92bd8660b36c]
> >> ...
> >> Although the key exits:
> >> keyctl show @us
> >> Keyring
> >>  549666721 --alswrv  0 65534  keyring: _uid_ses.0
> >>  346719914 --alswrv  0 65534   \_ keyring: _uid.0
> >>  235623693 --alswrv  0 0   \_ user: dd6f92bd8660b36c
> >>  747773852 --alswrv  0 0   \_ user: 7025717e50fd74a2
> >> With this change, ecryptfs can see the keys it needs.
> >>
> >> Note that 'keyctl show' still only shows the session keyring by default.
> >> We need to specify 'keyctl show @us' to see the user session keyring
> >> when the session keyring exits.
> >>
> >> Signed-off-by: Gwendal Grignou 
> >> ---
> >>  security/keys/process_keys.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> >> index e6d50172..a77d66e 100644
> >> --- a/security/keys/process_keys.c
> >> +++ b/security/keys/process_keys.c
> >> @@ -395,8 +395,8 @@ key_ref_t search_my_process_keyrings(struct 
> >> keyring_search_context *ctx)
> >> break;
> >> }
> >> }
> >> -   /* or search the user-session keyring */
> >> -   else if (ctx->cred->user->session_keyring) {
> >> +   /* finally search the user-session keyring */
> >> +   if (ctx->cred->user->session_keyring) {
> >> key_ref = keyring_search_aux(
> >> make_key_ref(ctx->cred->user->session_keyring, 1),
> >> ctx);
> >> --
> >> 2.7.0.rc3.207.g0ac5344
> >>

Re: [PATCH] keyrings: Allow searching the user session keyring

[Serge E. Hallyn]

Sorry, I just don't know al lthe subtleties there, I think we just need
David's ack/nack here.

Quoting Gwendal Grignou (gwen...@google.com):
> Any feedback on this? It is mandatory if we want to mount a ecryptfs
> directory while the session keyring is used.
> 
> Thanks,
> Gwendal.
> 
> On Thu, Mar 17, 2016 at 10:04 AM, Gwendal Grignou  
> wrote:
> > Resent to a larger audience.
> >
> > On Thu, Mar 10, 2016 at 2:20 PM, Gwendal Grignou  
> > wrote:
> >> Currently, if a session keyring exists, we are not searching in the
> >> user session or user keyrings.
> >>
> >> This is a problem when a session keyring exists and we want to use
> >> ecryptfs, who adds the needed key only in the user keyring.
> >>
> >> TEST=Without this change, mounting an ecryptfs "partition" fails when a
> >> session keyring exists:
> >> ...
> >> [ 2686.047522] Could not find key with description: [dd6f92bd8660b36c]
> >> ...
> >> Although the key exits:
> >> keyctl show @us
> >> Keyring
> >>  549666721 --alswrv  0 65534  keyring: _uid_ses.0
> >>  346719914 --alswrv  0 65534   \_ keyring: _uid.0
> >>  235623693 --alswrv  0 0   \_ user: dd6f92bd8660b36c
> >>  747773852 --alswrv  0 0   \_ user: 7025717e50fd74a2
> >> With this change, ecryptfs can see the keys it needs.
> >>
> >> Note that 'keyctl show' still only shows the session keyring by default.
> >> We need to specify 'keyctl show @us' to see the user session keyring
> >> when the session keyring exits.
> >>
> >> Signed-off-by: Gwendal Grignou 
> >> ---
> >>  security/keys/process_keys.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> >> index e6d50172..a77d66e 100644
> >> --- a/security/keys/process_keys.c
> >> +++ b/security/keys/process_keys.c
> >> @@ -395,8 +395,8 @@ key_ref_t search_my_process_keyrings(struct 
> >> keyring_search_context *ctx)
> >> break;
> >> }
> >> }
> >> -   /* or search the user-session keyring */
> >> -   else if (ctx->cred->user->session_keyring) {
> >> +   /* finally search the user-session keyring */
> >> +   if (ctx->cred->user->session_keyring) {
> >> key_ref = keyring_search_aux(
> >> make_key_ref(ctx->cred->user->session_keyring, 1),
> >> ctx);
> >> --
> >> 2.7.0.rc3.207.g0ac5344
> >>

Re: [PATCH] keyrings: Allow searching the user session keyring

[Gwendal Grignou]

Any feedback on this? It is mandatory if we want to mount a ecryptfs
directory while the session keyring is used.

Thanks,
Gwendal.

On Thu, Mar 17, 2016 at 10:04 AM, Gwendal Grignou  wrote:
> Resent to a larger audience.
>
> On Thu, Mar 10, 2016 at 2:20 PM, Gwendal Grignou  wrote:
>> Currently, if a session keyring exists, we are not searching in the
>> user session or user keyrings.
>>
>> This is a problem when a session keyring exists and we want to use
>> ecryptfs, who adds the needed key only in the user keyring.
>>
>> TEST=Without this change, mounting an ecryptfs "partition" fails when a
>> session keyring exists:
>> ...
>> [ 2686.047522] Could not find key with description: [dd6f92bd8660b36c]
>> ...
>> Although the key exits:
>> keyctl show @us
>> Keyring
>>  549666721 --alswrv  0 65534  keyring: _uid_ses.0
>>  346719914 --alswrv  0 65534   \_ keyring: _uid.0
>>  235623693 --alswrv  0 0   \_ user: dd6f92bd8660b36c
>>  747773852 --alswrv  0 0   \_ user: 7025717e50fd74a2
>> With this change, ecryptfs can see the keys it needs.
>>
>> Note that 'keyctl show' still only shows the session keyring by default.
>> We need to specify 'keyctl show @us' to see the user session keyring
>> when the session keyring exits.
>>
>> Signed-off-by: Gwendal Grignou 
>> ---
>>  security/keys/process_keys.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
>> index e6d50172..a77d66e 100644
>> --- a/security/keys/process_keys.c
>> +++ b/security/keys/process_keys.c
>> @@ -395,8 +395,8 @@ key_ref_t search_my_process_keyrings(struct 
>> keyring_search_context *ctx)
>> break;
>> }
>> }
>> -   /* or search the user-session keyring */
>> -   else if (ctx->cred->user->session_keyring) {
>> +   /* finally search the user-session keyring */
>> +   if (ctx->cred->user->session_keyring) {
>> key_ref = keyring_search_aux(
>> make_key_ref(ctx->cred->user->session_keyring, 1),
>> ctx);
>> --
>> 2.7.0.rc3.207.g0ac5344
>>

Re: [PATCH] keyrings: Allow searching the user session keyring

[Gwendal Grignou]

Any feedback on this? It is mandatory if we want to mount a ecryptfs
directory while the session keyring is used.

Thanks,
Gwendal.

On Thu, Mar 17, 2016 at 10:04 AM, Gwendal Grignou  wrote:
> Resent to a larger audience.
>
> On Thu, Mar 10, 2016 at 2:20 PM, Gwendal Grignou  wrote:
>> Currently, if a session keyring exists, we are not searching in the
>> user session or user keyrings.
>>
>> This is a problem when a session keyring exists and we want to use
>> ecryptfs, who adds the needed key only in the user keyring.
>>
>> TEST=Without this change, mounting an ecryptfs "partition" fails when a
>> session keyring exists:
>> ...
>> [ 2686.047522] Could not find key with description: [dd6f92bd8660b36c]
>> ...
>> Although the key exits:
>> keyctl show @us
>> Keyring
>>  549666721 --alswrv  0 65534  keyring: _uid_ses.0
>>  346719914 --alswrv  0 65534   \_ keyring: _uid.0
>>  235623693 --alswrv  0 0   \_ user: dd6f92bd8660b36c
>>  747773852 --alswrv  0 0   \_ user: 7025717e50fd74a2
>> With this change, ecryptfs can see the keys it needs.
>>
>> Note that 'keyctl show' still only shows the session keyring by default.
>> We need to specify 'keyctl show @us' to see the user session keyring
>> when the session keyring exits.
>>
>> Signed-off-by: Gwendal Grignou 
>> ---
>>  security/keys/process_keys.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
>> index e6d50172..a77d66e 100644
>> --- a/security/keys/process_keys.c
>> +++ b/security/keys/process_keys.c
>> @@ -395,8 +395,8 @@ key_ref_t search_my_process_keyrings(struct 
>> keyring_search_context *ctx)
>> break;
>> }
>> }
>> -   /* or search the user-session keyring */
>> -   else if (ctx->cred->user->session_keyring) {
>> +   /* finally search the user-session keyring */
>> +   if (ctx->cred->user->session_keyring) {
>> key_ref = keyring_search_aux(
>> make_key_ref(ctx->cred->user->session_keyring, 1),
>> ctx);
>> --
>> 2.7.0.rc3.207.g0ac5344
>>

Re: [PATCH] keyrings: Allow searching the user session keyring

[Gwendal Grignou]

Resent to a larger audience.

On Thu, Mar 10, 2016 at 2:20 PM, Gwendal Grignou  wrote:
> Currently, if a session keyring exists, we are not searching in the
> user session or user keyrings.
>
> This is a problem when a session keyring exists and we want to use
> ecryptfs, who adds the needed key only in the user keyring.
>
> TEST=Without this change, mounting an ecryptfs "partition" fails when a
> session keyring exists:
> ...
> [ 2686.047522] Could not find key with description: [dd6f92bd8660b36c]
> ...
> Although the key exits:
> keyctl show @us
> Keyring
>  549666721 --alswrv  0 65534  keyring: _uid_ses.0
>  346719914 --alswrv  0 65534   \_ keyring: _uid.0
>  235623693 --alswrv  0 0   \_ user: dd6f92bd8660b36c
>  747773852 --alswrv  0 0   \_ user: 7025717e50fd74a2
> With this change, ecryptfs can see the keys it needs.
>
> Note that 'keyctl show' still only shows the session keyring by default.
> We need to specify 'keyctl show @us' to see the user session keyring
> when the session keyring exits.
>
> Signed-off-by: Gwendal Grignou 
> ---
>  security/keys/process_keys.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index e6d50172..a77d66e 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -395,8 +395,8 @@ key_ref_t search_my_process_keyrings(struct 
> keyring_search_context *ctx)
> break;
> }
> }
> -   /* or search the user-session keyring */
> -   else if (ctx->cred->user->session_keyring) {
> +   /* finally search the user-session keyring */
> +   if (ctx->cred->user->session_keyring) {
> key_ref = keyring_search_aux(
> make_key_ref(ctx->cred->user->session_keyring, 1),
> ctx);
> --
> 2.7.0.rc3.207.g0ac5344
>

Re: [PATCH] keyrings: Allow searching the user session keyring

[Gwendal Grignou]

Resent to a larger audience.

On Thu, Mar 10, 2016 at 2:20 PM, Gwendal Grignou  wrote:
> Currently, if a session keyring exists, we are not searching in the
> user session or user keyrings.
>
> This is a problem when a session keyring exists and we want to use
> ecryptfs, who adds the needed key only in the user keyring.
>
> TEST=Without this change, mounting an ecryptfs "partition" fails when a
> session keyring exists:
> ...
> [ 2686.047522] Could not find key with description: [dd6f92bd8660b36c]
> ...
> Although the key exits:
> keyctl show @us
> Keyring
>  549666721 --alswrv  0 65534  keyring: _uid_ses.0
>  346719914 --alswrv  0 65534   \_ keyring: _uid.0
>  235623693 --alswrv  0 0   \_ user: dd6f92bd8660b36c
>  747773852 --alswrv  0 0   \_ user: 7025717e50fd74a2
> With this change, ecryptfs can see the keys it needs.
>
> Note that 'keyctl show' still only shows the session keyring by default.
> We need to specify 'keyctl show @us' to see the user session keyring
> when the session keyring exits.
>
> Signed-off-by: Gwendal Grignou 
> ---
>  security/keys/process_keys.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index e6d50172..a77d66e 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -395,8 +395,8 @@ key_ref_t search_my_process_keyrings(struct 
> keyring_search_context *ctx)
> break;
> }
> }
> -   /* or search the user-session keyring */
> -   else if (ctx->cred->user->session_keyring) {
> +   /* finally search the user-session keyring */
> +   if (ctx->cred->user->session_keyring) {
> key_ref = keyring_search_aux(
> make_key_ref(ctx->cred->user->session_keyring, 1),
> ctx);
> --
> 2.7.0.rc3.207.g0ac5344
>