On Mon, Dec 19, 2011 at 8:59 AM, Tyler Hicks <tyhicks@xxxxxxxxxxxxx> wrote:
> statfs() calls on eCryptfs files returned the wrong filesystem type and,
> when using filename encryption, the wrong maximum filename length.
Thanks for getting this fixed up!
> +#define ENC_NAME_MAX_BLOCKLEN_8 143
> +#define ENC_NAME_MAX_BLOCKLEN_16 143
...
> + /* Return an exact amount for the common cases */
> + if (lower_namelen == NAME_MAX) {
> + if (cipher_blocksize == 8) {
> + (*namelen) = ENC_NAME_MAX_BLOCKLEN_8;
> + return 0;
> + } else if (cipher_blocksize == 16) {
> + (*namelen) = ENC_NAME_MAX_BLOCKLEN_16;
> + return 0;
> + }
> + }
Why different paths here when the defines are the same? Also, what
math is used to determine the "143"?
> + /* Return a safe estimate for the uncommon cases */
> + (*namelen) -= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE;
> + /* Since this is the max decoded size, subtract 1 "decoded block" len */
> + (*namelen) = ecryptfs_max_decoded_size(*namelen) - 3;
> + (*namelen) -= ECRYPTFS_TAG_70_MAX_METADATA_SIZE;
> + (*namelen) -= ECRYPTFS_FILENAME_MIN_RANDOM_PREPEND_BYTES;
> + /* Worst case is that the filename is padded nearly a full block size */
> + (*namelen) -= cipher_blocksize - 1;
Would it be less fragile to just use this path for all the
calculations? Almost everything is a literal value.
> +#define ECRYPTFS_TAG_70_MIN_METADATA_SIZE (1 + 1 + ECRYPTFS_SIG_SIZE + 1 + 1)
> +#define ECRYPTFS_TAG_70_MAX_METADATA_SIZE (ECRYPTFS_TAG_70_MIN_METADATA_SIZE \
> + + 1)
...
> - s->max_packet_size = (1 /* Tag 70 identifier */
> - + 3 /* Max Tag 70 packet size */
> - + ECRYPTFS_SIG_SIZE /* FNEK sig */
> - + 1 /* Cipher identifier */
> + s->max_packet_size = (ECRYPTFS_TAG_70_MAX_METADATA_SIZE
Would it make sense to reproduce the sizing comments from here into
the #defines above, just to avoid losing this documentation?
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
--
Kees Cook
ChromeOS Security
--
To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
