On 2011-12-19 09:35:44, Kees Cook wrote:
> 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!
I appreciate the review!
>
> > +#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?
I'll get that fixed. I had the 8 byte block size in as just a placeholder
until I could test the max length (I knew 16 byte block size max was 143
characters, but didn't know the 8 byte max). I should have merged the
two paths after discovering that they were both the same value.
> Also, what math is used to determine the "143"?
No math involved. Just testing.
>
> > + /* 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;
I should probably add a check here to make sure namelen isn't negative at this point.
>
> Would it be less fragile to just use this path for all the
> calculations? Almost everything is a literal value.
I agree that just having one path would be cleaner, but this calculation
is just a rounded down estimate and the hard-coded common cases above
should be sufficient the vast majority of the time.
For the most common case of lower_f_namelen being 255 and a
cipher_blocksize of 16, this math comes out to 130. To try to give
userspace apps a little more room to work with, I figured it would be
better to hard-code in the two common cases.
>
> > +#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?
These comments were actually a repeat of the comments right above this
line. That comment section is still there and is what I consider the
"official" in-code documentation of a tag 70 packet. Here is the comment
section:
/* Octet 0: Tag 70 identifier
* Octets 1-N1: Tag 70 packet size (includes cipher identifier
* and block-aligned encrypted filename size)
* Octets N1-N2: FNEK sig (ECRYPTFS_SIG_SIZE)
* Octet N2-N3: Cipher identifier (1 octet)
* Octets N3-N4: Block-aligned encrypted filename
* - Consists of a minimum number of random characters, a \0
* separator, and then the filename */
Tyler
>
> 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
Attachment:
signature.asc
Description: Digital signature
