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 -- 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
