On Mon, Dec 19, 2011 at 11:47 AM, Tyler Hicks <tyhicks@xxxxxxxxxxxxx> wrote:
> 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.
Heh, good idea. :)
>> 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.
Ah-ha, okay, sounds good to me.
>> 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:
Ah-ha. Excellent. That's what I get for only reading the diff. ;)
-Kees
--
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
