Bug 1301621 - Parse URL ports as 16 bit; r?valentin draft
authorManish Goregaokar <manishsmail@gmail.com>
Fri, 09 Sep 2016 15:42:42 +0800
changeset 413586 f7874e8e2cb7bc68c30e64d4deee6c6a8e14096c
parent 411959 176aff980979bf588baed78c2824571a6a7f2b96
child 531250 f66eacd1c3674ec28235502224a45763bd19af96
push id29452
push userbmo:manishearth@gmail.com
push dateWed, 14 Sep 2016 13:57:52 +0000
reviewersvalentin
bugs1301621
milestone51.0a1
Bug 1301621 - Parse URL ports as 16 bit; r?valentin MozReview-Commit-ID: 5FbRUsYzJdy
netwerk/base/nsStandardURL.cpp
netwerk/base/nsURLParsers.cpp
netwerk/test/unit/test_bug652761.js
netwerk/test/unit/test_invalidport.js
netwerk/test/unit/test_large_port.js
netwerk/test/unit/xpcshell.ini
testing/web-platform/meta/url/url-constructor.html.ini
testing/web-platform/meta/url/url-setters.html.ini
toolkit/components/search/tests/xpcshell/test_location_error.js
--- a/netwerk/base/nsStandardURL.cpp
+++ b/netwerk/base/nsStandardURL.cpp
@@ -1283,16 +1283,18 @@ nsStandardURL::GetHost(nsACString &resul
 {
     result = Host();
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsStandardURL::GetPort(int32_t *result)
 {
+    // should never be more than 16 bit
+    MOZ_ASSERT(mPort <= std::numeric_limits<uint16_t>::max());
     *result = mPort;
     return NS_OK;
 }
 
 // result may contain unescaped UTF-8 characters
 NS_IMETHODIMP
 nsStandardURL::GetPath(nsACString &result)
 {
@@ -1960,18 +1962,19 @@ nsStandardURL::SetPort(int32_t port)
 {
     ENSURE_MUTABLE();
 
     LOG(("nsStandardURL::SetPort [port=%d]\n", port));
 
     if ((port == mPort) || (mPort == -1 && port == mDefaultPort))
         return NS_OK;
 
-    // ports must be >= 0
-    if (port < -1) // -1 == use default
+    // ports must be >= 0 and 16 bit
+    // -1 == use default
+    if (port < -1 || port > std::numeric_limits<uint16_t>::max())
         return NS_ERROR_MALFORMED_URI;
 
     if (mURLType == URLTYPE_NO_AUTHORITY) {
         NS_WARNING("cannot set port on no-auth url");
         return NS_ERROR_UNEXPECTED;
     }
 
     InvalidateCache();
@@ -3116,17 +3119,18 @@ NS_IMETHODIMP
 nsStandardURL::Init(uint32_t urlType,
                     int32_t defaultPort,
                     const nsACString &spec,
                     const char *charset,
                     nsIURI *baseURI)
 {
     ENSURE_MUTABLE();
 
-    if (spec.Length() > (uint32_t) net_GetURLMaxLength()) {
+    if (spec.Length() > (uint32_t) net_GetURLMaxLength() ||
+        defaultPort > std::numeric_limits<uint16_t>::max()) {
         return NS_ERROR_MALFORMED_URI;
     }
 
     InvalidateCache();
 
     switch (urlType) {
     case URLTYPE_STANDARD:
         mParser = net_GetStdURLParser();
@@ -3167,16 +3171,21 @@ nsStandardURL::Init(uint32_t urlType,
 
 NS_IMETHODIMP
 nsStandardURL::SetDefaultPort(int32_t aNewDefaultPort)
 {
     ENSURE_MUTABLE();
 
     InvalidateCache();
 
+    // should never be more than 16 bit
+    if (aNewDefaultPort >= std::numeric_limits<uint16_t>::max()) {
+        return NS_ERROR_MALFORMED_URI;
+    }
+
     // If we're already using the new default-port as a custom port, then clear
     // it off of our mSpec & set mPort to -1, to indicate that we'll be using
     // the default from now on (which happens to match what we already had).
     if (mPort == aNewDefaultPort) {
         ReplacePortInSpec(-1);
         mPort = -1;
     }
     mDefaultPort = aNewDefaultPort;
--- a/netwerk/base/nsURLParsers.cpp
+++ b/netwerk/base/nsURLParsers.cpp
@@ -595,17 +595,17 @@ nsAuthURLParser::ParseServerInfo(const c
             }
             else {
                 const char* nondigit = NS_strspnp("0123456789", buf.get());
                 if (nondigit && *nondigit)
                     return NS_ERROR_MALFORMED_URI;
 
                 nsresult err;
                 *port = buf.ToInteger(&err);
-                if (NS_FAILED(err) || *port < 0)
+                if (NS_FAILED(err) || *port < 0 || *port > std::numeric_limits<uint16_t>::max())
                     return NS_ERROR_MALFORMED_URI;
             }
         }
     }
     else {
         // serverinfo = <hostname>
         SET_RESULT(hostname, 0, serverinfoLen);
         if (port)
--- a/netwerk/test/unit/test_bug652761.js
+++ b/netwerk/test/unit/test_bug652761.js
@@ -1,20 +1,17 @@
 // This is just a crashtest for a url that is rejected at parse time (port 80,000)
 
 Cu.import("resource://gre/modules/NetUtil.jsm");
 
-function completeTest(request, data, ctx)
+function run_test()
 {
+    // Bug 1301621 makes invalid ports throw
+    Assert.throws(() => {
+        var chan = NetUtil.newChannel({
+          uri: "http://localhost:80000/",
+          loadUsingSystemPrincipal: true
+        });
+    }, "invalid port");
+
     do_test_finished();
 }
 
-function run_test()
-{
-    var chan = NetUtil.newChannel({
-      uri: "http://localhost:80000/",
-      loadUsingSystemPrincipal: true
-    });
-    var httpChan = chan.QueryInterface(Components.interfaces.nsIHttpChannel);
-    httpChan.asyncOpen2(new ChannelListener(completeTest,httpChan, CL_EXPECT_FAILURE));
-    do_test_pending();
-}
-
deleted file mode 100644
--- a/netwerk/test/unit/test_invalidport.js
+++ /dev/null
@@ -1,38 +0,0 @@
-// This is essentially a crashtest for accessing an out of range port
-// Perform the async open several times in order to induce exponential
-// scheduling behavior bugs.
-
-Cu.import("resource://gre/modules/NetUtil.jsm");
-
-var CC = Components.Constructor;
-
-var counter = 0;
-const iterations = 10;
-
-var listener = {
-  onStartRequest: function test_onStartR(request, ctx) {
-  },
-
-  onDataAvailable: function test_ODA() {
-    do_throw("Should not get any data!");
-  },
-
-  onStopRequest: function test_onStopR(request, ctx, status) {
-    if (counter++ == iterations)
-      do_test_finished();
-    else
-      execute_test();
-  },
-};
-
-function run_test() {
-  execute_test();
-  do_test_pending();
-}
-
-function execute_test() {
-  var chan = NetUtil.newChannel({uri: "http://localhost:75000", loadUsingSystemPrincipal: true});
-  chan.QueryInterface(Ci.nsIHttpChannel);
-  chan.asyncOpen2(listener);
-}
-
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_large_port.js
@@ -0,0 +1,36 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// Ensure that non-16-bit URIs are rejected
+
+"use strict";
+
+var Cc = Components.classes;
+var Ci = Components.interfaces;
+
+const StandardURL = Components.Constructor("@mozilla.org/network/standard-url;1",
+                                           "nsIStandardURL",
+                                           "init");
+function run_test()
+{
+    // Bug 1301621 makes invalid ports throw
+    Assert.throws(() => {
+        new StandardURL(Ci.nsIStandardURL.URLTYPE_AUTHORITY, 65536,
+                "http://localhost", "UTF-8", null)
+    }, "invalid port during creation");
+    let url = new StandardURL(Ci.nsIStandardURL.URLTYPE_AUTHORITY, 65535,
+                              "http://localhost", "UTF-8", null)
+                .QueryInterface(Ci.nsIStandardURL)
+
+    Assert.throws(() => {
+        url.setDefaultPort(65536);
+    }, "invalid port in setDefaultPort");
+    Assert.throws(() => {
+        url.port = 65536;
+    }, "invalid port in port setter");
+
+    do_check_eq(url.QueryInterface(Ci.nsIURI).port, -1);
+    do_test_finished();
+}
+
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@@ -227,18 +227,18 @@ skip-if = bits != 32
 [test_idn_blacklist.js]
 [test_idn_urls.js]
 [test_idna2008.js]
 # IDNA2008 depends on ICU, not available on android
 skip-if = os == "android"
 [test_immutable.js]
 skip-if = !hasNode
 run-sequentially = node server exceptions dont replay well
-[test_invalidport.js]
 [test_localstreams.js]
+[test_large_port.js]
 [test_mismatch_last-modified.js]
 [test_MIME_params.js]
 [test_mozTXTToHTMLConv.js]
 [test_multipart_byteranges.js]
 [test_multipart_streamconv.js]
 [test_multipart_streamconv_missing_lead_boundary.js]
 [test_nestedabout_serialize.js]
 [test_net_addr.js]
--- a/testing/web-platform/meta/url/url-constructor.html.ini
+++ b/testing/web-platform/meta/url/url-constructor.html.ini
@@ -259,19 +259,16 @@
     expected: FAIL
 
   [Parsing: <sc://ñ.test/> against <about:blank>]
     expected: FAIL
 
   [Parsing: <file:..> against <http://www.example.com/test>]
     expected: FAIL
 
-  [Parsing: <http://f:999999/c> against <http://example.org/foo/bar>]
-    expected: FAIL
-
   [Parsing: <http://www/foo%2Ehtml> against <about:blank>]
     expected: FAIL
 
   [Parsing: <http://example.com/foo/%2e%2> against <about:blank>]
     expected: FAIL
 
   [Parsing: <http://example.com/foo/%2e./%2e%2e/.%2e/%2e.bar> against <about:blank>]
     expected: FAIL
--- a/testing/web-platform/meta/url/url-setters.html.ini
+++ b/testing/web-platform/meta/url/url-setters.html.ini
@@ -70,19 +70,16 @@
     expected: FAIL
 
   [Setting <view-source+http://example.net/path>.host = 'example.com\\stuff' \\ is not a delimiter for non-special schemes, and it’s invalid in a domain]
     expected: FAIL
 
   [Setting <view-source+http://example.net/path>.host = 'example.com:8080stuff2' Anything other than ASCII digit stops the port parser in a setter but is not an error]
     expected: FAIL
 
-  [Setting <http://example.net/path>.host = 'example.com:65536' Port numbers are 16 bit integers, overflowing is an error. Hostname is still set, though.]
-    expected: FAIL
-
   [Setting <view-source+http://example.net/foo>.hostname = '' The empty host is OK for non-special schemes]
     expected: FAIL
 
   [Setting <a:/foo>.hostname = 'example.net' Path-only URLs can gain a host]
     expected: FAIL
 
   [Setting <http://example.net:8080>.hostname = '0x7F000001' IPv4 address syntax is normalized]
     expected: FAIL
@@ -100,19 +97,16 @@
     expected: FAIL
 
   [Setting <http://example.net:8080>.port = '' Port number is unchanged if empty in the new value. Note: this may change, see https://github.com/whatwg/url/pull/113]
     expected: FAIL
 
   [Setting <view-source+http://example.net/path>.port = '8080stuff2' Anything other than ASCII digit stops the port parser in a setter but is not an error]
     expected: FAIL
 
-  [Setting <http://example.net:8080/path>.port = '65536' Port numbers are 16 bit integers, overflowing is an error]
-    expected: FAIL
-
   [Setting <unix:/run/foo.socket?timeout=10>.pathname = '/var/log/../run/bar.socket']
     expected: FAIL
 
   [Setting <http://example.net/home?lang=fr#nav>.pathname = '\\a\\%2E\\b\\%2e.\\c' \\ is a segment delimiter for 'special' URLs]
     expected: FAIL
 
   [Setting <view-source+http://example.net/home?lang=fr#nav>.pathname = '\\a\\%2E\\b\\%2e.\\c' \\ is *not* a segment delimiter for non-'special' URLs]
     expected: FAIL
--- a/toolkit/components/search/tests/xpcshell/test_location_error.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_error.js
@@ -1,16 +1,16 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 function run_test() {
   installTestEngine();
 
-  // using a port > 2^32 causes an error to be reported.
-  let url = "http://localhost:111111111";
+  // We use an invalid port that parses but won't open
+  let url = "http://localhost:0";
 
   Services.prefs.setCharPref("browser.search.geoip.url", url);
   Services.search.init(() => {
     try {
       Services.prefs.getCharPref("browser.search.countryCode");
       ok(false, "not expecting countryCode to be set");
     } catch (ex) {}
     // should have an error recorded.