* [PATCH] KEYS: reject NULL restriction string when type is specified [not found] <089e082676ecdedb0b055f394f11@google.com> @ 2017-11-30 20:51 ` Eric Biggers 2017-12-01 1:22 ` Mat Martineau 0 siblings, 1 reply; 4+ messages in thread From: Eric Biggers @ 2017-11-30 20:51 UTC (permalink / raw) To: keyrings, David Howells Cc: syzbot, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, Mat Martineau, Eric Biggers, stable From: Eric Biggers <ebiggers@google.com> keyctl_restrict_keyring() allows through a NULL restriction when the "type" is non-NULL, which causes a NULL pointer dereference in asymmetric_lookup_restriction() when it calls strcmp() on the restriction string. But no key types actually use a "NULL restriction" to mean anything, so update keyctl_restrict_keyring() to reject it with EINVAL. Reported-by: syzbot <syzkaller@googlegroups.com> Fixes: 97d3aa0f3134 ("KEYS: Add a lookup_restriction function for the asymmetric key type") Cc: <stable@vger.kernel.org> # v4.12+ Signed-off-by: Eric Biggers <ebiggers@google.com> --- security/keys/keyctl.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 76d22f726ae4..1ffe60bb2845 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -1588,9 +1588,8 @@ long keyctl_session_to_parent(void) * The caller must have Setattr permission to change keyring restrictions. * * The requested type name may be a NULL pointer to reject all attempts - * to link to the keyring. If _type is non-NULL, _restriction can be - * NULL or a pointer to a string describing the restriction. If _type is - * NULL, _restriction must also be NULL. + * to link to the keyring. In this case, _restriction must also be NULL. + * Otherwise, both _type and _restriction must be non-NULL. * * Returns 0 if successful. */ @@ -1598,7 +1597,6 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type, const char __user *_restriction) { key_ref_t key_ref; - bool link_reject = !_type; char type[32]; char *restriction = NULL; long ret; @@ -1607,31 +1605,29 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type, if (IS_ERR(key_ref)) return PTR_ERR(key_ref); + ret = -EINVAL; if (_type) { - ret = key_get_type_from_user(type, _type, sizeof(type)); - if (ret < 0) + if (!_restriction) goto error; - } - if (_restriction) { - if (!_type) { - ret = -EINVAL; + ret = key_get_type_from_user(type, _type, sizeof(type)); + if (ret < 0) goto error; - } restriction = strndup_user(_restriction, PAGE_SIZE); if (IS_ERR(restriction)) { ret = PTR_ERR(restriction); goto error; } + } else { + if (_restriction) + goto error; } - ret = keyring_restrict(key_ref, link_reject ? NULL : type, restriction); + ret = keyring_restrict(key_ref, _type ? type : NULL, restriction); kfree(restriction); - error: key_ref_put(key_ref); - return ret; } -- 2.15.0.531.g2ccb3012c9-goog ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KEYS: reject NULL restriction string when type is specified 2017-11-30 20:51 ` [PATCH] KEYS: reject NULL restriction string when type is specified Eric Biggers @ 2017-12-01 1:22 ` Mat Martineau 2017-12-08 14:45 ` David Howells 0 siblings, 1 reply; 4+ messages in thread From: Mat Martineau @ 2017-12-01 1:22 UTC (permalink / raw) To: Eric Biggers Cc: keyrings, David Howells, syzbot, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, Eric Biggers, stable Eric, On Thu, 30 Nov 2017, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > keyctl_restrict_keyring() allows through a NULL restriction when the > "type" is non-NULL, which causes a NULL pointer dereference in > asymmetric_lookup_restriction() when it calls strcmp() on the > restriction string. > > But no key types actually use a "NULL restriction" to mean anything, so > update keyctl_restrict_keyring() to reject it with EINVAL. Since this fixes the bug for the asymmetric key type and ensures that other key types won't make the same mistake, I agree this is the way to fix it. I did not find any issues in the patch. Thanks, Mat > Reported-by: syzbot <syzkaller@googlegroups.com> > Fixes: 97d3aa0f3134 ("KEYS: Add a lookup_restriction function for the asymmetric key type") > Cc: <stable@vger.kernel.org> # v4.12+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > security/keys/keyctl.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c > index 76d22f726ae4..1ffe60bb2845 100644 > --- a/security/keys/keyctl.c > +++ b/security/keys/keyctl.c > @@ -1588,9 +1588,8 @@ long keyctl_session_to_parent(void) > * The caller must have Setattr permission to change keyring restrictions. > * > * The requested type name may be a NULL pointer to reject all attempts > - * to link to the keyring. If _type is non-NULL, _restriction can be > - * NULL or a pointer to a string describing the restriction. If _type is > - * NULL, _restriction must also be NULL. > + * to link to the keyring. In this case, _restriction must also be NULL. > + * Otherwise, both _type and _restriction must be non-NULL. > * > * Returns 0 if successful. > */ > @@ -1598,7 +1597,6 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type, > const char __user *_restriction) > { > key_ref_t key_ref; > - bool link_reject = !_type; > char type[32]; > char *restriction = NULL; > long ret; > @@ -1607,31 +1605,29 @@ long keyctl_restrict_keyring(key_serial_t id, const char __user *_type, > if (IS_ERR(key_ref)) > return PTR_ERR(key_ref); > > + ret = -EINVAL; > if (_type) { > - ret = key_get_type_from_user(type, _type, sizeof(type)); > - if (ret < 0) > + if (!_restriction) > goto error; > - } > > - if (_restriction) { > - if (!_type) { > - ret = -EINVAL; > + ret = key_get_type_from_user(type, _type, sizeof(type)); > + if (ret < 0) > goto error; > - } > > restriction = strndup_user(_restriction, PAGE_SIZE); > if (IS_ERR(restriction)) { > ret = PTR_ERR(restriction); > goto error; > } > + } else { > + if (_restriction) > + goto error; > } > > - ret = keyring_restrict(key_ref, link_reject ? NULL : type, restriction); > + ret = keyring_restrict(key_ref, _type ? type : NULL, restriction); > kfree(restriction); > - > error: > key_ref_put(key_ref); > - > return ret; > } > > -- > 2.15.0.531.g2ccb3012c9-goog > > -- Mat Martineau Intel OTC ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KEYS: reject NULL restriction string when type is specified 2017-12-01 1:22 ` Mat Martineau @ 2017-12-08 14:45 ` David Howells 2017-12-08 17:52 ` Mat Martineau 0 siblings, 1 reply; 4+ messages in thread From: David Howells @ 2017-12-08 14:45 UTC (permalink / raw) To: Mat Martineau Cc: dhowells, Eric Biggers, keyrings, syzbot, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, Eric Biggers, stable Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > Since this fixes the bug for the asymmetric key type and ensures that other > key types won't make the same mistake, I agree this is the way to fix it. I > did not find any issues in the patch. Can I put that down as a Reviewed-by? David ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KEYS: reject NULL restriction string when type is specified 2017-12-08 14:45 ` David Howells @ 2017-12-08 17:52 ` Mat Martineau 0 siblings, 0 replies; 4+ messages in thread From: Mat Martineau @ 2017-12-08 17:52 UTC (permalink / raw) To: David Howells Cc: Eric Biggers, keyrings, syzbot, davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs, Eric Biggers, stable On Fri, 8 Dec 2017, David Howells wrote: > Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > >> Since this fixes the bug for the asymmetric key type and ensures that other >> key types won't make the same mistake, I agree this is the way to fix it. I >> did not find any issues in the patch. > > Can I put that down as a Reviewed-by? Yes. Looks like I missed the window for your pull request, though - I'll be sure to add Reviewed-by in future reviews. -- Mat Martineau Intel OTC ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-08 17:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <089e082676ecdedb0b055f394f11@google.com>
2017-11-30 20:51 ` [PATCH] KEYS: reject NULL restriction string when type is specified Eric Biggers
2017-12-01 1:22 ` Mat Martineau
2017-12-08 14:45 ` David Howells
2017-12-08 17:52 ` Mat Martineau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).