Bug 1041186, Part 1: Improve buffer overflow protection in mozilla::p…

…kix, r=keeler

--HG--
extra : rebase_source : 0f4a33f2c66594930ba9c79233648c70e33ba27c

lib/mozpkix/include/pkix/Input.h

@@ -25,36 +25,65 @@
 #ifndef mozilla_pkix__Input_h
 #define mozilla_pkix__Input_h
 
+#include <cstring>
+
 #include "pkix/nullptr.h"
 #include "pkix/Result.h"
-#include "seccomon.h"
+#include "prlog.h"
 #include "stdint.h"
 
 namespace mozilla { namespace pkix {
 
-// Expect* functions advance the input mark and return Success if the input
-// matches the given criteria; they fail with the input mark in an undefined
-// state if the input does not match the criteria.
+class Input;
+
+// An InputBuffer is a safety-oriented immutable weak reference to a array of
+// bytes of a known size. The data can only be legally accessed by constructing
+// an Input object, which guarantees all accesses to the data are memory safe.
+// Neither InputBuffer not Input provide any facilities for modifying the data
+// they reference.
 //
-// Match* functions advance the input mark and return true if the input matches
-// the given criteria; they return false without changing the input mark if the
-// input does not match the criteria.
+// InputBuffers are small and should usually be passed by value, not by
+// reference, though for inline functions the distinction doesn't matter.
 //
-// Skip* functions unconditionally advance the input mark and return Success if
-// they are able to do so; otherwise they fail with the input mark in an
-// undefined state.
-class Input
+//    Result GoodExample(InputBuffer input);
+//    Result BadExample(const InputBuffer& input);
+//    Result WorseExample(const uint8_t* input, size_t len);
+//
+// Note that in the example, GoodExample has the same performance
+// characteristics as WorseExample, but with much better safety guarantees.
+class InputBuffer
 {
 public:
-  Input()
-    : input(nullptr)
-    , end(nullptr)
+  // This constructor is useful for input buffers that are statically known to
+  // be of a fixed size, e.g.:
+  //
+  //   static const uint8_t EXPECTED_BYTES[] = { 0x00, 0x01, 0x02 };
+  //   const InputBuffer expected(EXPECTED_BYTES);
+  //
+  // This is equivalent to (and preferred over):
+  //
+  //   static const uint8_t EXPECTED_BYTES[] = { 0x00, 0x01, 0x02 };
+  //   InputBuffer expected;
+  //   Result rv = expected.Init(EXPECTED_BYTES, sizeof EXPECTED_BYTES);
+  template <uint16_t N>
+  explicit InputBuffer(const uint8_t (&data)[N])
+    : data(data)
+    , len(N)
+  {
+  }
+
+  // Construct a valid, empty, Init-able InputBuffer.
+  InputBuffer()
+    : data(nullptr)
+    , len(0u)
   {
   }
 
+  // Initialize the input buffer. data must be non-null and len must be less
+  // than 65536. Init may not be called more than once.
   Result Init(const uint8_t* data, size_t len)
   {
-    if (input) {
+    if (this->data) {
       // already initialized
       return Result::FATAL_ERROR_INVALID_ARGS;
     }
@@ -63,25 +92,68 @@ class Input
       return Result::ERROR_BAD_DER;
     }
 
-    // XXX: this->input = input bug was not caught by tests! Why not?
-    //      this->end = end bug was not caught by tests! Why not?
-    this->input = data;
-    this->end = data + len;
+    this->data = data;
+    this->len = len;
 
     return Success;
   }
 
-  Result Expect(const uint8_t* expected, uint16_t expectedLen)
+  // Initialize the input buffer to be equivalent to the given input buffer.
+  // Init may not be called more than once.
+  //
+  // This is basically operator=, but it wasn't given that name because
+  // normally callers do not check the result of operator=, and normally
+  // operator= can be used multiple times.
+  Result Init(InputBuffer other)
+  {
+    return Init(other.data, other.len);
+  }
+
+  // Returns the length of the buffer.
+  //
+  // Having the return type be uint16_t instead of size_t avoids the need for
+  // callers to ensure that the result is small enough.
+  uint16_t GetLength() const { return static_cast<uint16_t>(len); }
+
+  // Don't use this. It is here because we have some "friend" functions that we
+  // don't want to declare in this header file.
+  const uint8_t* UnsafeGetData() const { return data; }
+
+private:
+  const uint8_t* data;
+  size_t len;
+
+  void operator=(const InputBuffer&) /* = delete */; // Use Init instead.
+};
+
+inline bool
+InputBuffersAreEqual(const InputBuffer& a, const InputBuffer& b)
+{
+  return a.GetLength() == b.GetLength() &&
+         !std::memcmp(a.UnsafeGetData(), b.UnsafeGetData(), a.GetLength());
+}
+
+// An Input is a cursor/iterator through the contents of an InputBuffer,
+// designed to maximize safety during parsing while minimizing the performance
+// cost of that safety. In particular, all methods do strict bounds checking to
+// ensure buffer overflows are impossible, and they are all inline so that the
+// compiler can coalesce as many of those checks together as possible.
+//
+// In general, Input allows for one byte of lookahead and no backtracking.
+// However, the Match* functions internally may have more lookahead.
+class Input
+{
+public:
+  Input()
+    : input(nullptr)
+    , end(nullptr)
+  {
+  }
+
+  explicit Input(InputBuffer buffer)
+    : input(buffer.UnsafeGetData())
+    , end(buffer.UnsafeGetData() + buffer.GetLength())
   {
-    Result rv = EnsureLength(expectedLen);
-    if (rv != Success) {
-      return rv;
-    }
-    if (memcmp(input, expected, expectedLen)) {
-      return Result::ERROR_BAD_DER;
-    }
-    input += expectedLen;
-    return Success;
   }
 
   bool Peek(uint8_t expectedByte) const
@@ -176,15 +248,16 @@ class Input
     return Success;
   }
 
-  Result Skip(uint16_t len, SECItem& skippedItem)
+  Result Skip(uint16_t len, InputBuffer& skippedItem)
   {
     Result rv = EnsureLength(len);
     if (rv != Success) {
       return rv;
     }
-    skippedItem.type = siBuffer;
-    skippedItem.data = const_cast<uint8_t*>(input);
-    skippedItem.len = len;
+    rv = skippedItem.Init(input, len);
+    if (rv != Success) {
+      return rv;
+    }
     input += len;
     return Success;
   }
@@ -216,19 +289,27 @@ class Input
 
   Mark GetMark() const { return Mark(*this, input); }
 
-  Result GetSECItem(SECItemType type, const Mark& mark, /*out*/ SECItem& item)
+  Result GetInputBuffer(const Mark& mark, /*out*/ InputBuffer& item)
   {
     if (&mark.input != this || mark.mark > input) {
       PR_NOT_REACHED("invalid mark");
       return Result::FATAL_ERROR_INVALID_ARGS;
     }
-    item.type = type;
-    item.data = const_cast<uint8_t*>(mark.mark);
-    item.len = static_cast<decltype(item.len)>(input - mark.mark);
-    return Success;
+    return item.Init(mark.mark, static_cast<uint16_t>(input - mark.mark));
   }
 
 private:
+  Result Init(const uint8_t* data, uint16_t len)
+  {
+    if (input) {
+      // already initialized
+      return Result::FATAL_ERROR_INVALID_ARGS;
+    }
+    input = data;
+    end = data + len;
+    return Success;
+  }
+
   const uint8_t* input;
   const uint8_t* end;
 

lib/mozpkix/include/pkix/pkix.h

@@ -88,12 +88,12 @@ namespace mozilla { namespace pkix {
 //                                    of active distrust.
 // TODO(bug 968451): Document more of these.
 
-Result BuildCertChain(TrustDomain& trustDomain, const SECItem& cert,
+Result BuildCertChain(TrustDomain& trustDomain, InputBuffer cert,
                       PRTime time, EndEntityOrCA endEntityOrCA,
                       KeyUsage requiredKeyUsageIfPresent,
                       KeyPurposeId requiredEKUIfPresent,
                       const CertPolicyId& requiredPolicy,
-                      /*optional*/ const SECItem* stapledOCSPResponse);
+                      /*optional*/ const InputBuffer* stapledOCSPResponse);
 
 static const size_t OCSP_REQUEST_MAX_LENGTH = 127;
 Result CreateEncodedOCSPRequest(TrustDomain& trustDomain,
@@ -116,7 +116,7 @@ Result CreateEncodedOCSPRequest(TrustDomain& trustDomain,
 Result VerifyEncodedOCSPResponse(TrustDomain& trustDomain,
                                  const CertID& certID, PRTime time,
                                  uint16_t maxLifetimeInDays,
-                                 const SECItem& encodedResponse,
+                                 InputBuffer encodedResponse,
                        /* out */ bool& expired,
               /* optional out */ PRTime* thisUpdate = nullptr,
               /* optional out */ PRTime* validThrough = nullptr);

lib/mozpkix/include/pkix/pkixnss.h

@@ -33,7 +33,7 @@ namespace mozilla { namespace pkix {
 
 // Verify the given signed data using the given public key.
 Result VerifySignedData(const SignedDataWithSignature& sd,
-                        const SECItem& subjectPublicKeyInfo,
+                        InputBuffer subjectPublicKeyInfo,
                         void* pkcs11PinArg);
 
 // Computes the SHA-1 hash of the data in the current item.
@@ -47,11 +47,11 @@ Result VerifySignedData(const SignedDataWithSignature& sd,
 // TODO: Taking the output buffer as (uint8_t*, size_t) is counter to our
 // other, extensive, memory safety efforts in mozilla::pkix, and we should find
 // a way to provide a more-obviously-safe interface.
-Result DigestBuf(const SECItem& item, /*out*/ uint8_t* digestBuf,
+Result DigestBuf(InputBuffer item, /*out*/ uint8_t* digestBuf,
                  size_t digestBufLen);
 
 // Checks, for RSA keys and DSA keys, that the modulus is at least 1024 bits.
-Result CheckPublicKey(const SECItem& subjectPublicKeyInfo);
+Result CheckPublicKey(InputBuffer subjectPublicKeyInfo);
 
 Result MapPRErrorCodeToResult(PRErrorCode errorCode);
 PRErrorCode MapResultToPRErrorCode(Result result);
@@ -76,6 +76,16 @@ enum ErrorCode {
 
 void RegisterErrorTable();
 
+inline SECItem UnsafeMapInputBufferToSECItem(InputBuffer ib)
+{
+  SECItem result = {
+    siBuffer,
+    const_cast<uint8_t*>(ib.UnsafeGetData()),
+    ib.GetLength()
+  };
+  return result;
+}
+
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix__pkixnss_h