Bug 1784348 - improve checks while parsing MIME parameters. r=necko-r…

…eviewers,jesup,valentin Differential Revision: https://phabricator.services.mozilla.com/D172110
mozilla · Mar 14, 2023 · a8ecb95 · a8ecb95
1 parent 1e1ec80
commit a8ecb95
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 11 deletions.
diff --git a/netwerk/mime/nsIMIMEHeaderParam.idl b/netwerk/mime/nsIMIMEHeaderParam.idl
@@ -132,7 +132,7 @@ interface nsIMIMEHeaderParam : nsISupports {
    */
 
   [noscript]
-  string getParameterInternal(in string aHeaderVal,
+  string getParameterInternal(in ACString aHeaderVal,
                               in string aParamName,
                               out string aCharset,
                               out string aLang);

diff --git a/netwerk/mime/nsMIMEHeaderParamImpl.cpp b/netwerk/mime/nsMIMEHeaderParamImpl.cpp
@@ -120,6 +120,27 @@ nsresult nsMIMEHeaderParamImpl::GetParameterHTTP(const nsACString& aHeaderVal,
                         false, nullptr, aResult);
 }
 
+/* static */
+// detects any non-null characters pass null
+bool nsMIMEHeaderParamImpl::ContainsTrailingCharPastNull(
+    const nsACString& aVal) {
+  nsACString::const_iterator first;
+  aVal.BeginReading(first);
+  nsACString::const_iterator end;
+  aVal.EndReading(end);
+
+  if (FindCharInReadable(L'\0', first, end)) {
+    while (first != end) {
+      if (*first != '\0') {
+        // contains trailing characters past the null character
+        return true;
+      }
+      ++first;
+    }
+  }
+  return false;
+}
+
 // XXX : aTryLocaleCharset is not yet effective.
 /* static */
 nsresult nsMIMEHeaderParamImpl::DoGetParameter(
@@ -133,9 +154,8 @@ nsresult nsMIMEHeaderParamImpl::DoGetParameter(
   // aDecoding (5987 being a subset of 2231) and return charset.)
   nsCString med;
   nsCString charset;
-  rv = DoParameterInternal(PromiseFlatCString(aHeaderVal).get(), aParamName,
-                           aDecoding, getter_Copies(charset), aLang,
-                           getter_Copies(med));
+  rv = DoParameterInternal(aHeaderVal, aParamName, aDecoding,
+                           getter_Copies(charset), aLang, getter_Copies(med));
   if (NS_FAILED(rv)) return rv;
 
   // convert to UTF-8 after charset conversion and RFC 2047 decoding
@@ -370,7 +390,7 @@ bool IsValidOctetSequenceForCharset(const nsACString& aCharset,
 // The format of these header lines  is
 // <token> [ ';' <token> '=' <token-or-quoted-string> ]*
 NS_IMETHODIMP
-nsMIMEHeaderParamImpl::GetParameterInternal(const char* aHeaderValue,
+nsMIMEHeaderParamImpl::GetParameterInternal(const nsACString& aHeaderValue,
                                             const char* aParamName,
                                             char** aCharset, char** aLang,
                                             char** aResult) {
@@ -380,9 +400,23 @@ nsMIMEHeaderParamImpl::GetParameterInternal(const char* aHeaderValue,
 
 /* static */
 nsresult nsMIMEHeaderParamImpl::DoParameterInternal(
-    const char* aHeaderValue, const char* aParamName, ParamDecoding aDecoding,
-    char** aCharset, char** aLang, char** aResult) {
-  if (!aHeaderValue || !*aHeaderValue || !aResult) return NS_ERROR_INVALID_ARG;
+    const nsACString& aHeaderValue, const char* aParamName,
+    ParamDecoding aDecoding, char** aCharset, char** aLang, char** aResult) {
+  if (aHeaderValue.IsEmpty() || !aResult) {
+    return NS_ERROR_INVALID_ARG;
+  }
+
+  if (ContainsTrailingCharPastNull(aHeaderValue)) {
+    // See Bug 1784348
+    return NS_ERROR_INVALID_ARG;
+  }
+
+  const nsCString& flat = PromiseFlatCString(aHeaderValue);
+  const char* str = flat.get();
+
+  if (!*str) {
+    return NS_ERROR_INVALID_ARG;
+  }
 
   *aResult = nullptr;
 
@@ -395,8 +429,6 @@ nsresult nsMIMEHeaderParamImpl::DoParameterInternal(
   // them for HTTP header fields later on, see bug 776324
   bool acceptContinuations = true;
 
-  const char* str = aHeaderValue;
-
   // skip leading white space.
   for (; *str && nsCRT::IsAsciiSpace(*str); ++str) {
     ;

diff --git a/netwerk/mime/nsMIMEHeaderParamImpl.h b/netwerk/mime/nsMIMEHeaderParamImpl.h
@@ -33,10 +33,12 @@ class nsMIMEHeaderParamImpl : public nsIMIMEHeaderParam {
                                  bool aTryLocaleCharset, char** aLang,
                                  nsAString& aResult);
 
-  static nsresult DoParameterInternal(const char* aHeaderValue,
+  static nsresult DoParameterInternal(const nsACString& aHeaderVal,
                                       const char* aParamName,
                                       ParamDecoding aDecoding, char** aCharset,
                                       char** aLang, char** aResult);
+
+  static bool ContainsTrailingCharPastNull(const nsACString& aVal);
 };
 
 #endif
diff --git a/netwerk/test/unit/test_MIME_params.js b/netwerk/test/unit/test_MIME_params.js
@@ -579,6 +579,20 @@ var tests = [
 
   // Check that whitespace processing can't crash.
   ["attachment; filename =      ", "attachment", ""],
+
+  // Bug 1784348
+  [
+    "attachment; filename=foo.exe\0.pdf",
+    Cr.NS_ERROR_ILLEGAL_VALUE,
+    Cr.NS_ERROR_INVALID_ARG,
+  ],
+  [
+    "attachment; filename=\0\0foo\0",
+    Cr.NS_ERROR_ILLEGAL_VALUE,
+    Cr.NS_ERROR_INVALID_ARG,
+  ],
+  ["attachment; filename=foo\0\0\0", "attachment", "foo"],
+  ["attachment; filename=\0\0\0", "attachment", ""],
 ];
 
 var rfc5987paramtests = [