Re: [PATCH] eCryptfs: Improve statfs reporting

[Tyler Hicks <tyhicks@canonical.com>]

[-- Attachment #1: Type: text/plain, Size: 4357 bytes --]

On 2011-12-19 09:35:44, Kees Cook wrote:
> On Mon, Dec 19, 2011 at 8:59 AM, Tyler Hicks <tyhicks@canonical.com> 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@chromium.org>
> 
> -- 
> Kees Cook
> ChromeOS Security
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]