[network] Add some sanity checks to deserialized nsStandardURLs

Note: C++ lambda expressions are actually useful for once.

netwerk/base/nsStandardURL.cpp

@@ -315,6 +315,46 @@ DumpLeakedURLs::~DumpLeakedURLs()
 }
 #endif
 
+bool nsStandardURL::IsValid() {
+  auto checkSegment = [&](const nsStandardURL::URLSegment& aSeg) {
+    // Bad value
+    if (NS_WARN_IF(aSeg.mLen < -1)) {
+      return false;
+    }
+    if (aSeg.mLen == -1) {
+      return true;
+    }
+
+    // Position outside of string
+    if (NS_WARN_IF(aSeg.mPos + aSeg.mLen > mSpec.Length())) {
+      return false;
+    }
+
+    // Overflow
+    if (NS_WARN_IF(aSeg.mPos + aSeg.mLen < aSeg.mPos)) {
+      return false;
+    }
+
+    return true;
+  };
+
+  bool allSegmentsValid = checkSegment(mScheme) && checkSegment(mAuthority) &&
+                          checkSegment(mUsername) && checkSegment(mPassword) &&
+                          checkSegment(mHost) && checkSegment(mPath) &&
+                          checkSegment(mFilepath) && checkSegment(mDirectory) &&
+                          checkSegment(mBasename) && checkSegment(mExtension) &&
+                          checkSegment(mQuery) && checkSegment(mRef);
+  if (!allSegmentsValid) {
+    return false;
+  }
+
+  if (mScheme.mPos != 0) {
+    return false;
+  }
+
+  return true;
+}
+
 void
 nsStandardURL::InitGlobalObjects()
 {
@@ -2752,8 +2792,8 @@ nsStandardURL::SetFilePath(const nsACString &input)
         mSpec.Cut(mPath.mPos + 1, mFilepath.mLen - 1);
         // left shift query, and ref
         ShiftFromQuery(1 - mFilepath.mLen);
-        // One character for '/', and if we have a query or ref we add their
-        // length and one extra for each '?' or '#' characters
+        // One character for '/', and if we have a query or ref we add their
+        // length and one extra for each '?' or '#' characters
         mPath.mLen = 1 + (mQuery.mLen >= 0 ? (mQuery.mLen + 1) : 0) +
                      (mRef.mLen >= 0 ? (mRef.mLen + 1) : 0);
         // these contain only a '/'
@@ -3518,6 +3558,10 @@ nsStandardURL::Deserialize(const URIParams& aParams)
         return false;
     }
 
+    // If we exit early, make sure to clear the URL so we don't fail the sanity
+    // check in the destructor
+    auto clearOnExit = MakeScopeExit([&] { Clear(); });
+
     const StandardURLParams& params = aParams.get_StandardURLParams();
 
     mURLType = params.urlType();
@@ -3565,7 +3609,7 @@ nsStandardURL::Deserialize(const URIParams& aParams)
     mSupportsFileURL = params.supportsFileURL();
     mHostEncoding = params.hostEncoding();
 
-    // Some sanity checks
+    // Some segment sanity checks
     NS_ENSURE_TRUE(mScheme.mPos == 0, false);
     NS_ENSURE_TRUE(mScheme.mLen > 0, false);
     // Make sure scheme is followed by :// (3 characters)
@@ -3577,6 +3621,13 @@ nsStandardURL::Deserialize(const URIParams& aParams)
     NS_ENSURE_TRUE(mQuery.mLen == -1 || mSpec.CharAt(mQuery.mPos - 1) == '?', false);
     NS_ENSURE_TRUE(mRef.mLen == -1 || mSpec.CharAt(mRef.mPos - 1) == '#', false);
 
+    // Sanity-check the result
+    if (!IsValid()) {
+      return false;
+    }
+
+    clearOnExit.release();
+
     // mSpecEncoding and mHostA are just caches that can be recovered as needed.
     return true;
 }

netwerk/base/nsStandardURL.h

@@ -253,6 +253,9 @@ private:
     void FindHostLimit(nsACString::const_iterator& aStart,
                        nsACString::const_iterator& aEnd);
 
+    // Checks if the URL has a valid representation.
+    bool IsValid();
+
     // mSpec contains the normalized version of the URL spec (UTF-8 encoded).
     nsCString mSpec;
     int32_t   mDefaultPort;

netwerk/test/gtest/TestStandardURL.cpp

@@ -9,6 +9,10 @@
 #include "nsComponentManagerUtils.h"
 #include "nsIIPCSerializableURI.h"
 #include "mozilla/ipc/URIUtils.h"
+#include "mozilla/Unused.h"
+#include "nsSerializationHelper.h"
+#include "mozilla/Base64.h"
+#include "nsEscape.h"
 
 TEST(TestStandardURL, Simple) {
     nsCOMPtr<nsIURL> url( do_CreateInstance(NS_STANDARDURL_CONTRACTID) );
@@ -83,3 +87,50 @@ TEST(TestStandardURL, Deserialize_Bug1392739)
   nsCOMPtr<nsIIPCSerializableURI> url = do_CreateInstance(NS_STANDARDURL_CID);
   ASSERT_EQ(url->Deserialize(params), false);
 }
+
+TEST(TestStandardURL, CorruptSerialization)
+{
+  auto spec = "http://user:pass@example.com/path/to/file.ext?query#hash"_ns;
+
+  nsCOMPtr<nsIURI> uri;
+  nsresult rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID)
+                    .SetSpec(spec)
+                    .Finalize(uri);
+  ASSERT_EQ(rv, NS_OK);
+
+  nsAutoCString serialization;
+  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(uri);
+  ASSERT_TRUE(serializable);
+
+  // Check that the URL is normally serializable.
+  ASSERT_EQ(NS_OK, NS_SerializeToString(serializable, serialization));
+  nsCOMPtr<nsISupports> deserializedObject;
+  ASSERT_EQ(NS_OK, NS_DeserializeObject(serialization,
+                                        getter_AddRefs(deserializedObject)));
+
+  nsAutoCString canonicalBin;
+  Unused << Base64Decode(serialization, canonicalBin);
+
+// The spec serialization begins at byte 49
+// If the implementation of nsStandardURL::Write changes, this test will need
+// to be adjusted.
+#define SPEC_OFFSET 49
+
+  ASSERT_EQ(Substring(canonicalBin, SPEC_OFFSET, 7), "http://"_ns);
+
+  nsAutoCString corruptedBin = canonicalBin;
+  // change mScheme.mPos
+  corruptedBin.BeginWriting()[SPEC_OFFSET + spec.Length()] = 1;
+  Unused << Base64Encode(corruptedBin, serialization);
+  ASSERT_EQ(
+      NS_ERROR_MALFORMED_URI,
+      NS_DeserializeObject(serialization, getter_AddRefs(deserializedObject)));
+
+  corruptedBin = canonicalBin;
+  // change mScheme.mLen
+  corruptedBin.BeginWriting()[SPEC_OFFSET + spec.Length() + 4] = 127;
+  Unused << Base64Encode(corruptedBin, serialization);
+  ASSERT_EQ(
+      NS_ERROR_MALFORMED_URI,
+      NS_DeserializeObject(serialization, getter_AddRefs(deserializedObject)));
+}