linux-ext4 - Re: [PATCH] fscrypto: make fname_encrypt() actually return length of ciphertext

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:   Wed, 14 Sep 2016 15:37:01 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers@...gle.com>
Cc:     linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, tytso@....edu,
        jaegeuk@...nel.org
Subject: Re: [PATCH] fscrypto: make fname_encrypt() actually return length of ciphertext

On Sep 14, 2016, at 2:57 PM, Eric Biggers <ebiggers@...gle.com> wrote:
> 
> This makes the return value match the comment.  Previously it would
> actually return 0 if encryption was successful.  No callers currently
> care, but this change should reduce the chance of future bugs.

This may be introducing a subtle bug in fscrypt_fname_usr_to_disk(), since
that function returns the status from fname_encrypt() directly and now it
returns the name length instead of 0 on success:

int fscrypt_fname_usr_to_disk(struct inode *inode,
                        const struct qstr *iname,
                        struct fscrypt_str *oname)
{
        if (fscrypt_is_dot_dotdot(iname)) {
                oname->name[0] = '.';
                oname->name[iname->len - 1] = '.';
                oname->len = iname->len;
                return oname->len;
        }
        if (inode->i_crypt_info)
                return fname_encrypt(inode, iname, oname);
        /*
         * Without a proper key, a user is not allowed to modify the filenames
         * in a directory. Consequently, a user space name cannot be mapped to
         * a disk-space name
         */
        return -EACCES;
}

This percolates further up to some of the callers, but in the cases that I
saw the check is "if (err < 0)" and the positive value is either ignored
or overwritten before being returned further up the call chain.  However,
that could be easily missed in the future and somewhere up the call chain
doing "if (rc)" would suddenly start to fail.

Since both "struct fscrypt_str" and "struct qstr" already hold the length
I don't think there is any benefit to returning the length to the caller.
Since (IMHO) this creates a non-trivial chance of introducing bugs in the
future it makes more sense to just change the function comment to match the
actual behaviour.

Cheers, Andreas

> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
> fs/crypto/fname.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 3108806..7f3239c 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -103,12 +103,13 @@ static int fname_encrypt(struct inode *inode,
> 	}
> 	kfree(alloc_buf);
> 	skcipher_request_free(req);
> -	if (res < 0)
> +	if (res < 0) {
> 		printk_ratelimited(KERN_ERR
> 				"%s: Error (error code %d)\n", __func__, res);
> -
> +		return res;
> +	}
> 	oname->len = ciphertext_len;
> -	return res;
> +	return ciphertext_len;
> }
> 
> /*
> --
> 2.8.0.rc3.226.g39d4020
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






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