lkml.org 
[lkml]   [2014]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: PROBLEM: apparent out-of-bounds memory write in fs/ecryptfs/crypto.c
Hi Dmitry,

On Fri, Nov 21, 2014 at 04:44:02PM +0400, Dmitry Chernenkov wrote:
> Hi!
>
> The following issue was discovered using Kernel Address Sanitizer
> we're developing
> (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel,
> https://github.com/google/kasan/blob/kasan/Documentation/kasan.txt)
>
> The apparent problem is here (fs/ecryptfs/crypto.c:1866 .. I tested
> on kernel 3.14, but looks like the issue is still there upstream):
>
> static size_t ecryptfs_max_decoded_size(size_t encoded_size)
> {
> /* Not exact; conservatively long. Every block of 4
> * encoded characters decodes into a block of 3
> * decoded characters. This segment of code provides
> * the caller with the maximum amount of allocated
> * space that @dst will need to point to in a
> * subsequent call. */
> return ((encoded_size + 1) * 3) / 4;
> }
>
> /**
> * ecryptfs_decode_from_filename
> * @dst: If NULL, this function only sets @dst_size and returns. If
> * non-NULL, this function decodes the encoded octets in @src
> * into the memory that @dst points to.
> * @dst_size: Set to the size of the decoded string.
> * @src: The encoded set of octets to decode.
> * @src_size: The size of the encoded set of octets to decode.
> */
> static void
> ecryptfs_decode_from_filename(unsigned char *dst, size_t *dst_size,
> const unsigned char *src, size_t src_size)
> {
> u8 current_bit_offset = 0;
> size_t src_byte_offset = 0;
> size_t dst_byte_offset = 0;
>
> if (dst == NULL) {
> (*dst_size) = ecryptfs_max_decoded_size(src_size);
> goto out;
> }
> while (src_byte_offset < src_size) {
> unsigned char src_byte =
> filename_rev_map[(int)src[src_byte_offset]];
>
> switch (current_bit_offset) {
> case 0:
> dst[dst_byte_offset] = (src_byte << 2);
> current_bit_offset = 6;
> break;
> case 6:
> dst[dst_byte_offset++] |= (src_byte >> 4);
> dst[dst_byte_offset] = ((src_byte & 0xF)
> << 4);
> current_bit_offset = 4;
> break;
> case 4:
> dst[dst_byte_offset++] |= (src_byte >> 2);
> dst[dst_byte_offset] = (src_byte << 6);
> current_bit_offset = 2;
> break;
> case 2:
> dst[dst_byte_offset++] |= (src_byte);
> dst[dst_byte_offset] = 0;
> current_bit_offset = 0;
> break;
> }
> src_byte_offset++;
> }
> (*dst_size) = dst_byte_offset;
> out:
> return;
> }
> For src_size multiple of 4 (which I assume is usually the case), the
> line "dst[dst_byte_offset] = 0;" writes at dst[dst_size] reported in
> the ecryptfs_max_decoded_size. The caller mallocs exactly dst_size
> bytes for dst, so the write is outside the allocated space. Depending
> on allocator, this can be exploited to overwrite the next object's
> first byte. I didn't exactly understand whether dst should be a
> 0-terminated string, so we either need to skip writing the trailing
> zero or allocate 1 more byte.

Thanks for reporting this!

Based on a quick read, it looks like the only consumer of the decoded
memory is ecryptfs_parse_tag_70_packet, and it uses the passed size for
its parsing. It seems like dropping this line from
ecryptfs_decode_from_filename() would solve the issue:

dst[dst_byte_offset] = 0;

The state machine goes from case 2 back to case 0. If we're at the end,
dst_byte_offset is the correct value, and the rest of the buffer is
untouched. If we have more to go, case 0 will strictly initialize the
contents of dst[dst_byte_offset] since it uses "=" not "|=".

Thoughts?

-Kees

--
Kees Cook
Chrome OS Security


\
 
 \ /
  Last update: 2014-11-22 01:41    [W:0.031 / U:4.440 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site