linux-kernel - Re: [PATCH] crypto: crypto_user.c: Cleaning up missing null-terminate in conjunction with strncpy

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 for Android: free password hash cracker in your pocket

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

Date:	Sat, 26 Jul 2014 21:17:50 -0700
From:	Mark D Rustad <mrustad@...il.com>
To:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
Cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>,
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] crypto: crypto_user.c:  Cleaning up missing null-terminate in conjunction with strncpy

Rickard,

On Jul 26, 2014, at 7:15 AM, Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se> wrote:

> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
> ---
> crypto/crypto_user.c |   12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index e2a34fe..09b465e 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -77,7 +77,7 @@ static int crypto_report_cipher(struct sk_buff *skb, struct crypto_alg *alg)
> {
> 	struct crypto_report_cipher rcipher;
> 
> -	strncpy(rcipher.type, "cipher", sizeof(rcipher.type));
> +	strlcpy(rcipher.type, "cipher", sizeof(rcipher.type));
> 
> 	rcipher.blocksize = alg->cra_blocksize;
> 	rcipher.min_keysize = alg->cra_cipher.cia_min_keysize;

This patch is an example of what I mentioned in my previous message. I figured I should go back and take a look at more of the patches. It looks to me like all of the changes in this patch create information leaks, because strlcpy only copies to the terminator, leaving the rest of the destination area in an otherwise uninitialized stack area alone. That structure is then copied whole into an skb and sent as a netlink message.

That will leak kernel information into userspace.

All of these patches need to be considered in regard to information leakage.

> @@ -96,7 +96,7 @@ static int crypto_report_comp(struct sk_buff *skb, struct crypto_alg *alg)
> {
> 	struct crypto_report_comp rcomp;
> 
> -	strncpy(rcomp.type, "compression", sizeof(rcomp.type));
> +	strlcpy(rcomp.type, "compression", sizeof(rcomp.type));
> 	if (nla_put(skb, CRYPTOCFGA_REPORT_COMPRESS,
> 		    sizeof(struct crypto_report_comp), &rcomp))
> 		goto nla_put_failure;
> @@ -109,10 +109,10 @@ nla_put_failure:
> static int crypto_report_one(struct crypto_alg *alg,
> 			     struct crypto_user_alg *ualg, struct sk_buff *skb)
> {
> -	strncpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
> -	strncpy(ualg->cru_driver_name, alg->cra_driver_name,
> +	strlcpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
> +	strlcpy(ualg->cru_driver_name, alg->cra_driver_name,
> 		sizeof(ualg->cru_driver_name));
> -	strncpy(ualg->cru_module_name, module_name(alg->cra_module),
> +	strlcpy(ualg->cru_module_name, module_name(alg->cra_module),
> 		sizeof(ualg->cru_module_name));
> 
> 	ualg->cru_type = 0;
> @@ -125,7 +125,7 @@ static int crypto_report_one(struct crypto_alg *alg,
> 	if (alg->cra_flags & CRYPTO_ALG_LARVAL) {
> 		struct crypto_report_larval rl;
> 
> -		strncpy(rl.type, "larval", sizeof(rl.type));
> +		strlcpy(rl.type, "larval", sizeof(rl.type));
> 		if (nla_put(skb, CRYPTOCFGA_REPORT_LARVAL,
> 			    sizeof(struct crypto_report_larval), &rl))
> 			goto nla_put_failure;

-- 
Mark Rustad, MRustad@...il.com


Download attachment "signature.asc" of type "application/pgp-signature" (842 bytes)