linux-kernel - Re: [PATCH] KEYS: reject NULL restriction string when type is specified

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Nov 2017 17:22:05 -0800 (PST)
From:   Mat Martineau <mathew.j.martineau@...ux.intel.com>
To:     Eric Biggers <ebiggers3@...il.com>
cc:     keyrings@...r.kernel.org, David Howells <dhowells@...hat.com>,
        syzbot 
        <bot+25a1a3685abe943a89512250aece10bdd9aa4827@...kaller.appspotmail.com>,
        davem@...emloft.net, herbert@...dor.apana.org.au,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com,
        Eric Biggers <ebiggers@...gle.com>, stable@...r.kernel.org
Subject: Re: [PATCH] KEYS: reject NULL restriction string when type is
 specified


Eric,

On Thu, 30 Nov 2017, Eric Biggers wrote:

> From: Eric Biggers <ebiggers@...gle.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@...glegroups.com>
> Fixes: 97d3aa0f3134 ("KEYS: Add a lookup_restriction function for the asymmetric key type")
> Cc: <stable@...r.kernel.org> # v4.12+
> Signed-off-by: Eric Biggers <ebiggers@...gle.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