url: fix port overflow checking

This patch adds (port > 0xffff) check after each digit in the loop and prevents integer overflow. PR-URL: #15794 Refs: web-platform-tests/wpt#7602 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
nodejs · Oct 18, 2017 · f5e56aa · f5e56aa
1 parent 497bceb
commit f5e56aa
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 6 deletions.
diff --git a/src/node_url.cc b/src/node_url.cc
@@ -1597,10 +1597,11 @@ void URL::Parse(const char* input,
                    ch == '#' ||
                    special_back_slash) {
           if (buffer.size() > 0) {
-            int port = 0;
-            for (size_t i = 0; i < buffer.size(); i++)
+            unsigned port = 0;
+            // the condition port <= 0xffff prevents integer overflow
+            for (size_t i = 0; port <= 0xffff && i < buffer.size(); i++)
               port = port * 10 + buffer[i] - '0';
-            if (port < 0 || port > 0xffff) {
+            if (port > 0xffff) {
               // TODO(TimothyGu): This hack is currently needed for the host
               // setter since it needs access to hostname if it is valid, and
               // if the FAILED flag is set the entire response to JS layer
@@ -1611,7 +1612,8 @@ void URL::Parse(const char* input,
                 url->flags |= URL_FLAGS_FAILED;
               return;
             }
-            url->port = NormalizePort(url->scheme, port);
+            // the port is valid
+            url->port = NormalizePort(url->scheme, static_cast<int>(port));
             buffer.clear();
           } else if (has_state_override) {
             // TODO(TimothyGu): Similar case as above.

diff --git a/test/fixtures/url-tests.js b/test/fixtures/url-tests.js
@@ -2,7 +2,7 @@
 
 /* The following tests are copied from WPT. Modifications to them should be
    upstreamed first. Refs:
-   https://github.com/w3c/web-platform-tests/blob/5d149f0/url/urltestdata.json
+   https://github.com/w3c/web-platform-tests/blob/11757f1/url/urltestdata.json
    License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html
 */
 module.exports =
@@ -5811,6 +5811,24 @@ module.exports =
     "base": "about:blank",
     "failure": true
   },
+  "Port overflow (2^32 + 81)",
+  {
+    "input": "http://f:4294967377/c",
+    "base": "http://example.org/",
+    "failure": true
+  },
+  "Port overflow (2^64 + 81)",
+  {
+    "input": "http://f:18446744073709551697/c",
+    "base": "http://example.org/",
+    "failure": true
+  },
+  "Port overflow (2^128 + 81)",
+  {
+    "input": "http://f:340282366920938463463374607431768211537/c",
+    "base": "http://example.org/",
+    "failure": true
+  },
   "# Non-special-URL path tests",
   {
     "input": "sc://ñ",

diff --git a/test/parallel/test-whatwg-url-parsing.js b/test/parallel/test-whatwg-url-parsing.js
@@ -26,7 +26,7 @@ const failureTests = tests.filter((test) => test.failure).concat([
 ]);
 
 const expectedError = common.expectsError(
-  { code: 'ERR_INVALID_URL', type: TypeError }, 110);
+  { code: 'ERR_INVALID_URL', type: TypeError }, failureTests.length);
 
 for (const test of failureTests) {
   assert.throws(