Fix out-of-bounds read in Lz4Immutable::decompress / IovecCursor::peekInto (T267530293)

Reviewed By: alikhtarov

Differential Revision: D114025354

fbshipit-source-id: 14fc6b1e29ec0343f00a9f4c77b1d897c68fa842

Author: generatedunixname1383054420177565

mcrouter/lib/IovecCursor.cpp

@@ -60,18 +60,27 @@ void IovecCursor::seek(size_t pos) {
 }
 
 void IovecCursor::peekInto(uint8_t* dest, size_t size) const {
+  // Defense-in-depth: callers are contractually required to guarantee that
+  // "size" bytes are available, but a precondition violation must degrade to a
+  // short read rather than dereferencing iov_ past its end (out-of-bounds
+  // read). Compliant callers never trip these guards.
+  size_t i = iovIndex_;
+  if (FOLLY_UNLIKELY(i >= iovLength_)) {
+    return;
+  }
   const uint8_t* cur =
-      reinterpret_cast<uint8_t*>(iov_[iovIndex_].iov_base) + curBufPos_;
+      reinterpret_cast<uint8_t*>(iov_[i].iov_base) + curBufPos_;
   size_t curLen = curBufLen_;
-  size_t i = iovIndex_;
 
   while (size > 0) {
     size_t toCopy = std::min(size, curLen);
     std::memcpy(dest, cur, toCopy);
     dest += toCopy;
     size -= toCopy;
     if (size > 0) {
-      ++i;
+      if (FOLLY_UNLIKELY(++i >= iovLength_)) {
+        return;
+      }
       cur = reinterpret_cast<uint8_t*>(iov_[i].iov_base);
       curLen = iov_[i].iov_len;
     }

mcrouter/lib/Lz4Immutable.cpp

@@ -509,22 +509,40 @@ std::unique_ptr<folly::IOBuf> Lz4Immutable::decompress(
   IovecCursor source(iov, iovcnt);
   IovecCursor match = dicCursor;
 
+  // The compressed stream is untrusted (it can originate from a remote
+  // backend). IovecCursor pushes bounds-checking onto its caller, so every read
+  // below must be guarded against the end of the source: an encoded length that
+  // exceeds the bytes actually present would otherwise drive the cursor past
+  // the source iovec and read out of bounds.
+  const auto sourceBytesLeft = [&source] {
+    return source.totalLength() - source.tell();
+  };
+
   // Main loop
   while (true) {
     // LZ4 token
+    if (FOLLY_UNLIKELY(sourceBytesLeft() < sizeof(uint8_t))) {
+      return nullptr;
+    }
     size_t token = source.read<uint8_t>();
 
     // Get literal length
     size_t literalLength = token >> kMlBits;
     if (literalLength == kRunMask) {
       size_t s;
       do {
+        if (FOLLY_UNLIKELY(sourceBytesLeft() < sizeof(uint8_t))) {
+          return nullptr;
+        }
         s = source.read<uint8_t>();
         literalLength += s;
       } while (FOLLY_LIKELY(s == 255));
     }
 
     // Copy literals
+    if (FOLLY_UNLIKELY(literalLength > sourceBytesLeft())) {
+      return nullptr;
+    }
     uint8_t* cpy = output + literalLength;
     if (cpy > outputLimit - kCopyLength) {
       if (cpy != outputLimit) {
@@ -538,6 +556,9 @@ std::unique_ptr<folly::IOBuf> Lz4Immutable::decompress(
     output = cpy;
 
     // Get match offset
+    if (FOLLY_UNLIKELY(sourceBytesLeft() < sizeof(uint16_t))) {
+      return nullptr;
+    }
     uint16_t offset = peekLE(source);
     source.advance(2);
     size_t outputProgress = static_cast<size_t>(output - outputStart);
@@ -555,6 +576,9 @@ std::unique_ptr<folly::IOBuf> Lz4Immutable::decompress(
     if (matchLength == kMlMask) {
       size_t s;
       do {
+        if (FOLLY_UNLIKELY(sourceBytesLeft() < sizeof(uint8_t))) {
+          return nullptr;
+        }
         s = source.read<uint8_t>();
         matchLength += s;
       } while (s == 255);

mcrouter/lib/test/Lz4ImmutableTest.cpp

@@ -240,6 +240,26 @@ TEST(Lz4Immutable, error_checks) {
   EXPECT_THROW(compressor.compress(&iov, 1), std::invalid_argument);
 }
 
+// Regression test for T267530293: a malformed compressed stream whose token
+// declares more literal bytes than the source actually contains used to drive
+// IovecCursor past the end of the source iovec (out-of-bounds read). A correct
+// decompressor must reject the stream and return nullptr instead of reading
+// past the source. Under ASAN this over-read would otherwise be a detectable
+// heap-buffer-overflow.
+TEST(Lz4Immutable, decompress_truncated_source) {
+  auto dictionary = getAsciiDictionary();
+  Lz4Immutable compressor(dictionary->clone());
+
+  // Single LZ4 token byte: literal length 5, match length 0, but zero literal
+  // bytes follow. A large declared output size keeps the output-bounds check
+  // from firing first, so the literal copy is reached with an exhausted source.
+  const uint8_t token = 0x50;
+  iovec iov = {const_cast<uint8_t*>(&token), sizeof(token)};
+
+  EXPECT_EQ(
+      compressor.decompress(&iov, 1, /* uncompressedSize */ 4096), nullptr);
+}
+
 TEST(Lz4Immutable, largeData_ascii) {
   auto dictionary = getAsciiDictionary();
   Lz4Immutable compressor(dictionary->clone());