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

[Mark D Rustad]

Rickard,

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

> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail