| Message ID | 1503396313-16760-1-git-send-email-stefan.bader@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 08/22/17 12:05, Stefan Bader wrote: > From bc34a1e44f8f6e5e1dc62aaa92117fc74e2384b9 Mon Sep 17 00:00:00 2001 > From: Eric Biggers <ebiggers@google.com> > Date: Tue, 18 Apr 2017 15:31:09 +0100 > Subject: [PATCH] KEYS: fix keyctl_set_reqkey_keyring() to not leak thread > keyrings > > This fixes CVE-2017-7472. > > Running the following program as an unprivileged user exhausts kernel > memory by leaking thread keyrings: > > #include <keyutils.h> > > int main() > { > for (;;) > keyctl_set_reqkey_keyring(KEY_REQKEY_DEFL_THREAD_KEYRING); > } > > Fix it by only creating a new thread keyring if there wasn't one before. > To make things more consistent, make install_thread_keyring_to_cred() > and install_process_keyring_to_cred() both return 0 if the corresponding > keyring is already present. > > Fixes: d84f4f992cbd ("CRED: Inaugurate COW credentials") > Cc: stable@vger.kernel.org # 2.6.29+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > Signed-off-by: David Howells <dhowells@redhat.com> > > CVE-2017-7472 > > (cherry-picked from commit c9f838d104fed6f2f61d68164712e3204bf5271b) > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > Notes: > Has been released to Zesty and Xenial already. Cherry-picked with > some fuzz ignored (-C2). > > security/keys/keyctl.c | 11 ++++------- > security/keys/process_keys.c | 44 +++++++++++++++++++++++++++----------------- > 2 files changed, 31 insertions(+), 24 deletions(-) > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c > index cb1eef9..9360394 100644 > --- a/security/keys/keyctl.c > +++ b/security/keys/keyctl.c > @@ -1240,8 +1240,8 @@ error: > * Read or set the default keyring in which request_key() will cache keys and > * return the old setting. > * > - * If a process keyring is specified then this will be created if it doesn't > - * yet exist. The old setting will be returned if successful. > + * If a thread or process keyring is specified then it will be created if it > + * doesn't yet exist. The old setting will be returned if successful. > */ > long keyctl_set_reqkey_keyring(int reqkey_defl) > { > @@ -1266,11 +1266,8 @@ long keyctl_set_reqkey_keyring(int reqkey_defl) > > case KEY_REQKEY_DEFL_PROCESS_KEYRING: > ret = install_process_keyring_to_cred(new); > - if (ret < 0) { > - if (ret != -EEXIST) > - goto error; > - ret = 0; > - } > + if (ret < 0) > + goto error; > goto set; > > case KEY_REQKEY_DEFL_DEFAULT: > diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c > index 4e56371..18bad7c 100644 > --- a/security/keys/process_keys.c > +++ b/security/keys/process_keys.c > @@ -125,13 +125,18 @@ error: > } > > /* > - * Install a fresh thread keyring directly to new credentials. This keyring is > - * allowed to overrun the quota. > + * Install a thread keyring to the given credentials struct if it didn't have > + * one already. This is allowed to overrun the quota. > + * > + * Return: 0 if a thread keyring is now present; -errno on failure. > */ > int install_thread_keyring_to_cred(struct cred *new) > { > struct key *keyring; > > + if (new->thread_keyring) > + return 0; > + > keyring = keyring_alloc("_tid", new->uid, new->gid, new, > KEY_POS_ALL | KEY_USR_VIEW, > KEY_ALLOC_QUOTA_OVERRUN, NULL); > @@ -143,7 +148,9 @@ int install_thread_keyring_to_cred(struct cred *new) > } > > /* > - * Install a fresh thread keyring, discarding the old one. > + * Install a thread keyring to the current task if it didn't have one already. > + * > + * Return: 0 if a thread keyring is now present; -errno on failure. > */ > static int install_thread_keyring(void) > { > @@ -154,8 +161,6 @@ static int install_thread_keyring(void) > if (!new) > return -ENOMEM; > > - BUG_ON(new->thread_keyring); > - > ret = install_thread_keyring_to_cred(new); > if (ret < 0) { > abort_creds(new); > @@ -166,17 +171,17 @@ static int install_thread_keyring(void) > } > > /* > - * Install a process keyring directly to a credentials struct. > + * Install a process keyring to the given credentials struct if it didn't have > + * one already. This is allowed to overrun the quota. > * > - * Returns -EEXIST if there was already a process keyring, 0 if one installed, > - * and other value on any other error > + * Return: 0 if a process keyring is now present; -errno on failure. > */ > int install_process_keyring_to_cred(struct cred *new) > { > struct key *keyring; > > if (new->process_keyring) > - return -EEXIST; > + return 0; > > keyring = keyring_alloc("_pid", new->uid, new->gid, new, > KEY_POS_ALL | KEY_USR_VIEW, > @@ -189,11 +194,9 @@ int install_process_keyring_to_cred(struct cred *new) > } > > /* > - * Make sure a process keyring is installed for the current process. The > - * existing process keyring is not replaced. > + * Install a process keyring to the current task if it didn't have one already. > * > - * Returns 0 if there is a process keyring by the end of this function, some > - * error otherwise. > + * Return: 0 if a process keyring is now present; -errno on failure. > */ > static int install_process_keyring(void) > { > @@ -207,14 +210,18 @@ static int install_process_keyring(void) > ret = install_process_keyring_to_cred(new); > if (ret < 0) { > abort_creds(new); > - return ret != -EEXIST ? ret : 0; > + return ret; > } > > return commit_creds(new); > } > > /* > - * Install a session keyring directly to a credentials struct. > + * Install the given keyring as the session keyring of the given credentials > + * struct, replacing the existing one if any. If the given keyring is NULL, > + * then install a new anonymous session keyring. > + * > + * Return: 0 on success; -errno on failure. > */ > int install_session_keyring_to_cred(struct cred *cred, struct key *keyring) > { > @@ -249,8 +256,11 @@ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring) > } > > /* > - * Install a session keyring, discarding the old one. If a keyring is not > - * supplied, an empty one is invented. > + * Install the given keyring as the session keyring of the current task, > + * replacing the existing one if any. If the given keyring is NULL, then > + * install a new anonymous session keyring. > + * > + * Return: 0 on success; -errno on failure. > */ > static int install_session_keyring(struct key *keyring) > { >
On 22/08/17 11:05, Stefan Bader wrote: > From bc34a1e44f8f6e5e1dc62aaa92117fc74e2384b9 Mon Sep 17 00:00:00 2001 > From: Eric Biggers <ebiggers@google.com> > Date: Tue, 18 Apr 2017 15:31:09 +0100 > Subject: [PATCH] KEYS: fix keyctl_set_reqkey_keyring() to not leak thread > keyrings > > This fixes CVE-2017-7472. > > Running the following program as an unprivileged user exhausts kernel > memory by leaking thread keyrings: > > #include <keyutils.h> > > int main() > { > for (;;) > keyctl_set_reqkey_keyring(KEY_REQKEY_DEFL_THREAD_KEYRING); > } > > Fix it by only creating a new thread keyring if there wasn't one before. > To make things more consistent, make install_thread_keyring_to_cred() > and install_process_keyring_to_cred() both return 0 if the corresponding > keyring is already present. > > Fixes: d84f4f992cbd ("CRED: Inaugurate COW credentials") > Cc: stable@vger.kernel.org # 2.6.29+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > Signed-off-by: David Howells <dhowells@redhat.com> > > CVE-2017-7472 > > (cherry-picked from commit c9f838d104fed6f2f61d68164712e3204bf5271b) > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > Notes: > Has been released to Zesty and Xenial already. Cherry-picked with > some fuzz ignored (-C2). > > security/keys/keyctl.c | 11 ++++------- > security/keys/process_keys.c | 44 +++++++++++++++++++++++++++----------------- > 2 files changed, 31 insertions(+), 24 deletions(-) > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c > index cb1eef9..9360394 100644 > --- a/security/keys/keyctl.c > +++ b/security/keys/keyctl.c > @@ -1240,8 +1240,8 @@ error: > * Read or set the default keyring in which request_key() will cache keys and > * return the old setting. > * > - * If a process keyring is specified then this will be created if it doesn't > - * yet exist. The old setting will be returned if successful. > + * If a thread or process keyring is specified then it will be created if it > + * doesn't yet exist. The old setting will be returned if successful. > */ > long keyctl_set_reqkey_keyring(int reqkey_defl) > { > @@ -1266,11 +1266,8 @@ long keyctl_set_reqkey_keyring(int reqkey_defl) > > case KEY_REQKEY_DEFL_PROCESS_KEYRING: > ret = install_process_keyring_to_cred(new); > - if (ret < 0) { > - if (ret != -EEXIST) > - goto error; > - ret = 0; > - } > + if (ret < 0) > + goto error; > goto set; > > case KEY_REQKEY_DEFL_DEFAULT: > diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c > index 4e56371..18bad7c 100644 > --- a/security/keys/process_keys.c > +++ b/security/keys/process_keys.c > @@ -125,13 +125,18 @@ error: > } > > /* > - * Install a fresh thread keyring directly to new credentials. This keyring is > - * allowed to overrun the quota. > + * Install a thread keyring to the given credentials struct if it didn't have > + * one already. This is allowed to overrun the quota. > + * > + * Return: 0 if a thread keyring is now present; -errno on failure. > */ > int install_thread_keyring_to_cred(struct cred *new) > { > struct key *keyring; > > + if (new->thread_keyring) > + return 0; > + > keyring = keyring_alloc("_tid", new->uid, new->gid, new, > KEY_POS_ALL | KEY_USR_VIEW, > KEY_ALLOC_QUOTA_OVERRUN, NULL); > @@ -143,7 +148,9 @@ int install_thread_keyring_to_cred(struct cred *new) > } > > /* > - * Install a fresh thread keyring, discarding the old one. > + * Install a thread keyring to the current task if it didn't have one already. > + * > + * Return: 0 if a thread keyring is now present; -errno on failure. > */ > static int install_thread_keyring(void) > { > @@ -154,8 +161,6 @@ static int install_thread_keyring(void) > if (!new) > return -ENOMEM; > > - BUG_ON(new->thread_keyring); > - > ret = install_thread_keyring_to_cred(new); > if (ret < 0) { > abort_creds(new); > @@ -166,17 +171,17 @@ static int install_thread_keyring(void) > } > > /* > - * Install a process keyring directly to a credentials struct. > + * Install a process keyring to the given credentials struct if it didn't have > + * one already. This is allowed to overrun the quota. > * > - * Returns -EEXIST if there was already a process keyring, 0 if one installed, > - * and other value on any other error > + * Return: 0 if a process keyring is now present; -errno on failure. > */ > int install_process_keyring_to_cred(struct cred *new) > { > struct key *keyring; > > if (new->process_keyring) > - return -EEXIST; > + return 0; > > keyring = keyring_alloc("_pid", new->uid, new->gid, new, > KEY_POS_ALL | KEY_USR_VIEW, > @@ -189,11 +194,9 @@ int install_process_keyring_to_cred(struct cred *new) > } > > /* > - * Make sure a process keyring is installed for the current process. The > - * existing process keyring is not replaced. > + * Install a process keyring to the current task if it didn't have one already. > * > - * Returns 0 if there is a process keyring by the end of this function, some > - * error otherwise. > + * Return: 0 if a process keyring is now present; -errno on failure. > */ > static int install_process_keyring(void) > { > @@ -207,14 +210,18 @@ static int install_process_keyring(void) > ret = install_process_keyring_to_cred(new); > if (ret < 0) { > abort_creds(new); > - return ret != -EEXIST ? ret : 0; > + return ret; > } > > return commit_creds(new); > } > > /* > - * Install a session keyring directly to a credentials struct. > + * Install the given keyring as the session keyring of the given credentials > + * struct, replacing the existing one if any. If the given keyring is NULL, > + * then install a new anonymous session keyring. > + * > + * Return: 0 on success; -errno on failure. > */ > int install_session_keyring_to_cred(struct cred *cred, struct key *keyring) > { > @@ -249,8 +256,11 @@ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring) > } > > /* > - * Install a session keyring, discarding the old one. If a keyring is not > - * supplied, an empty one is invented. > + * Install the given keyring as the session keyring of the current task, > + * replacing the existing one if any. If the given keyring is NULL, then > + * install a new anonymous session keyring. > + * > + * Return: 0 on success; -errno on failure. > */ > static int install_session_keyring(struct key *keyring) > { > Clean cherry pick, looks good to me. Acked-by: Colin Ian King <colin.king@canonical.com>
Applied to trusty/master-next branch. Thanks.
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index cb1eef9..9360394 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -1240,8 +1240,8 @@ error: * Read or set the default keyring in which request_key() will cache keys and * return the old setting. * - * If a process keyring is specified then this will be created if it doesn't - * yet exist. The old setting will be returned if successful. + * If a thread or process keyring is specified then it will be created if it + * doesn't yet exist. The old setting will be returned if successful. */ long keyctl_set_reqkey_keyring(int reqkey_defl) { @@ -1266,11 +1266,8 @@ long keyctl_set_reqkey_keyring(int reqkey_defl) case KEY_REQKEY_DEFL_PROCESS_KEYRING: ret = install_process_keyring_to_cred(new); - if (ret < 0) { - if (ret != -EEXIST) - goto error; - ret = 0; - } + if (ret < 0) + goto error; goto set; case KEY_REQKEY_DEFL_DEFAULT: diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index 4e56371..18bad7c 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -125,13 +125,18 @@ error: } /* - * Install a fresh thread keyring directly to new credentials. This keyring is - * allowed to overrun the quota. + * Install a thread keyring to the given credentials struct if it didn't have + * one already. This is allowed to overrun the quota. + * + * Return: 0 if a thread keyring is now present; -errno on failure. */ int install_thread_keyring_to_cred(struct cred *new) { struct key *keyring; + if (new->thread_keyring) + return 0; + keyring = keyring_alloc("_tid", new->uid, new->gid, new, KEY_POS_ALL | KEY_USR_VIEW, KEY_ALLOC_QUOTA_OVERRUN, NULL); @@ -143,7 +148,9 @@ int install_thread_keyring_to_cred(struct cred *new) } /* - * Install a fresh thread keyring, discarding the old one. + * Install a thread keyring to the current task if it didn't have one already. + * + * Return: 0 if a thread keyring is now present; -errno on failure. */ static int install_thread_keyring(void) { @@ -154,8 +161,6 @@ static int install_thread_keyring(void) if (!new) return -ENOMEM; - BUG_ON(new->thread_keyring); - ret = install_thread_keyring_to_cred(new); if (ret < 0) { abort_creds(new); @@ -166,17 +171,17 @@ static int install_thread_keyring(void) } /* - * Install a process keyring directly to a credentials struct. + * Install a process keyring to the given credentials struct if it didn't have + * one already. This is allowed to overrun the quota. * - * Returns -EEXIST if there was already a process keyring, 0 if one installed, - * and other value on any other error + * Return: 0 if a process keyring is now present; -errno on failure. */ int install_process_keyring_to_cred(struct cred *new) { struct key *keyring; if (new->process_keyring) - return -EEXIST; + return 0; keyring = keyring_alloc("_pid", new->uid, new->gid, new, KEY_POS_ALL | KEY_USR_VIEW, @@ -189,11 +194,9 @@ int install_process_keyring_to_cred(struct cred *new) } /* - * Make sure a process keyring is installed for the current process. The - * existing process keyring is not replaced. + * Install a process keyring to the current task if it didn't have one already. * - * Returns 0 if there is a process keyring by the end of this function, some - * error otherwise. + * Return: 0 if a process keyring is now present; -errno on failure. */ static int install_process_keyring(void) { @@ -207,14 +210,18 @@ static int install_process_keyring(void) ret = install_process_keyring_to_cred(new); if (ret < 0) { abort_creds(new); - return ret != -EEXIST ? ret : 0; + return ret; } return commit_creds(new); } /* - * Install a session keyring directly to a credentials struct. + * Install the given keyring as the session keyring of the given credentials + * struct, replacing the existing one if any. If the given keyring is NULL, + * then install a new anonymous session keyring. + * + * Return: 0 on success; -errno on failure. */ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring) { @@ -249,8 +256,11 @@ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring) } /* - * Install a session keyring, discarding the old one. If a keyring is not - * supplied, an empty one is invented. + * Install the given keyring as the session keyring of the current task, + * replacing the existing one if any. If the given keyring is NULL, then + * install a new anonymous session keyring. + * + * Return: 0 on success; -errno on failure. */ static int install_session_keyring(struct key *keyring) {