net: reject non-address SocketAddress.parse input

benchmark/net/net-socketaddress-parse.js

@@ -0,0 +1,43 @@
+'use strict';
+
+const common = require('../common.js');
+const { SocketAddress } = require('net');
+
+const inputs = {
+  'ipv4': [
+    '127.0.0.1',
+    '10.168.209.250',
+    '255.255.255.255',
+  ],
+  'ipv4-port': [
+    '127.0.0.1:80',
+    '10.168.209.250:8080',
+    '255.255.255.255:65535',
+  ],
+  'ipv6': [
+    '[::1]',
+    '[2001:db8::1]',
+    '[fe80::1ff:fe23:4567:890a]',
+  ],
+  'ipv6-port': [
+    '[::1]:80',
+    '[2001:db8::1]:8080',
+    '[::ffff:127.0.0.1]:65535',
+  ],
+};
+
+const bench = common.createBenchmark(main, {
+  n: [1e6],
+  input: Object.keys(inputs),
+});
+
+function main({ n, input }) {
+  const values = inputs[input];
+  const length = values.length;
+
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    SocketAddress.parse(values[i % length]);
+  }
+  bench.end(n);
+}

doc/api/net.md

@@ -438,13 +438,42 @@
 added:
   - v23.4.0
   - v22.13.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/00000
+    description: Input is now parsed strictly. URL syntax and legacy IPv4
+                 formats such as octal, hexadecimal and shorthand notation
+                 are no longer accepted.
 -->
 
 * `input` {string} An input string containing an IP address and optional port,
   e.g. `123.1.2.3:1234` or `[1::1]:1234`.
 * Returns: {net.SocketAddress} Returns a `SocketAddress` if parsing was successful.
   Otherwise returns `undefined`.
 
+The entire input must match one of the following forms:
+
+```text
+socket-address      = ipv4-socket-address / ipv6-socket-address
+ipv4-socket-address = ipv4-address [ ":" port ]
+ipv6-socket-address = "[" ipv6-address [ "%" scope-id ] "]" [ ":" port ]
+
+ipv4-address        = octet 3( "." octet )
+octet               = 1*3DIGIT    ; no leading zeros; value <= 255
+ipv6-address        = RFC 4291 textual form: groups of 1*4HEXDIG (leading
+                      zeros allowed, case-insensitive), "::" compression, and
+                      an optional trailing embedded ipv4-address
+port                = 1*DIGIT     ; leading zeros allowed; value <= 65535
+scope-id            = 1*DIGIT     ; leading zeros allowed; value <= 4294967295
+```
+
+Anything else returns `undefined`, including URL components such as userinfo,
+paths, queries and fragments, surrounding whitespace, host names, non-ASCII
+digits, and the legacy IPv4 notations that permit octal (`0177.0.0.1`),
+hexadecimal (`0x7f.0.0.1`), integer (`2130706433`) and shorthand (`127.1`)
+addresses. An IPv6 zone id must be numeric; interface names such as
+`%eth0` are not accepted.
+
 ## Class: `net.Server`
 
 <!-- YAML

lib/internal/socketaddress.js

@@ -7,6 +7,7 @@ const {
 
 const {
   SocketAddress: _SocketAddress,
+  parseSocketAddress,
   AF_INET,
   AF_INET6,
 } = internalBinding('block_list');
@@ -37,11 +38,23 @@ const {
   kDeserialize,
 } = require('internal/worker/js_transferable');
 
-const { URLParse } = require('internal/url');
-
 const kHandle = Symbol('kHandle');
 const kDetail = Symbol('kDetail');
 
+class InternalSocketAddress {
+  constructor(handle) {
+    markTransferMode(this, true, false);
+
+    this[kHandle] = handle;
+    this[kDetail] = this[kHandle]?.detail({
+      address: undefined,
+      port: undefined,
+      family: undefined,
+      flowlabel: undefined,
+    });
+  }
+}
+
 class SocketAddress {
   static isSocketAddress(value) {
     return value?.[kHandle] !== undefined;
@@ -149,40 +162,9 @@ class SocketAddress {
    */
   static parse(input) {
     validateString(input, 'input');
-    // While URL.parse is not expected to throw, there are several
-    // other pieces here that do... the destucturing, the SocketAddress
-    // constructor, etc. So we wrap this in a try/catch to be safe.
-    try {
-      const {
-        hostname: address,
-        port,
-      } = URLParse(`http://${input}`);
-      if (address.startsWith('[') && address.endsWith(']')) {
-        return new SocketAddress({
-          address: address.slice(1, -1),
-          port: port | 0,
-          family: 'ipv6',
-        });
-      }
-      return new SocketAddress({ address, port: port | 0 });
-    } catch {
-      // Ignore errors here. Return undefined if the input cannot
-      // be successfully parsed or is not a proper socket address.
-    }
-  }
-}
-
-class InternalSocketAddress {
-  constructor(handle) {
-    markTransferMode(this, true, false);
-
-    this[kHandle] = handle;
-    this[kDetail] = this[kHandle]?.detail({
-      address: undefined,
-      port: undefined,
-      family: undefined,
-      flowlabel: undefined,
-    });
+    const handle = parseSocketAddress(input);
+    if (handle === undefined) return undefined;
+    return new InternalSocketAddress(handle);
   }
 }
 

node.gyp

@@ -162,6 +162,7 @@
       'src/node_shadow_realm.cc',
       'src/node_snapshotable.cc',
       'src/node_sockaddr.cc',
+      'src/node_sockaddr_parser.cc',
       'src/node_stat_watcher.cc',
       'src/node_symbols.cc',
       'src/node_task_queue.cc',
@@ -296,6 +297,7 @@
       'src/node_snapshot_builder.h',
       'src/node_sockaddr.h',
       'src/node_sockaddr-inl.h',
+      'src/node_sockaddr_parser.h',
       'src/node_stat_watcher.h',
       'src/node_union_bytes.h',
       'src/node_url.h',

src/node_sockaddr.cc

@@ -7,10 +7,13 @@
 #include "node_errors.h"
 #include "node_hash.h"
 #include "node_sockaddr-inl.h"  // NOLINT(build/include_inline)
+#include "node_sockaddr_parser.h"
 #include "uv.h"
 
 #include <memory>
+#include <optional>
 #include <string>
+#include <string_view>
 #include <vector>
 
 namespace node {
@@ -69,6 +72,31 @@ bool SocketAddress::New(int32_t family,
       family, host, port, reinterpret_cast<sockaddr_storage*>(addr->storage()));
 }
 
+bool SocketAddress::Parse(std::string_view input, SocketAddress* addr) {
+  std::optional<sockaddr_parser::parse_result> parsed =
+      sockaddr_parser::ParseSocketAddress(input);
+  if (!parsed.has_value()) return false;
+
+  CHECK_LE(parsed->host.size(), sockaddr_parser::kMaxHostLength);
+
+  char host[sockaddr_parser::kMaxHostLength + 1];
+  host[parsed->host.copy(host, parsed->host.size())] = '\0';
+
+  if (!New(parsed->is_ipv6 ? AF_INET6 : AF_INET, host, parsed->port, addr)) {
+    return false;
+  }
+
+  // libuv resolves a zone id as an interface name, never as a number.
+  // TODO(@araujogui): sin6_scope_id is neither exposed to JS nor hashed, so
+  // scoped addresses do not round-trip and collide in BlockList.
+  if (parsed->is_ipv6) {
+    reinterpret_cast<sockaddr_in6*>(addr->storage())->sin6_scope_id =
+        parsed->scope_id;
+  }
+
+  return true;
+}
+
 size_t SocketAddress::Hash::operator()(const SocketAddress& addr) const {
   // Hash only the meaningful bytes (family + port + address), not the
   // full 128-byte sockaddr_storage.
@@ -84,6 +112,8 @@ size_t SocketAddress::Hash::operator()(const SocketAddress& addr) const {
     case AF_INET6: {
       const sockaddr_in6* ipv6 =
           reinterpret_cast<const sockaddr_in6*>(addr.raw());
+      // TODO(@araujogui): sin6_scope_id is not hashed, so addresses that
+      // differ only by zone id collide.
       uint8_t buf[18];
       memcpy(buf, &ipv6->sin6_port, 2);
       memcpy(buf + 2, &ipv6->sin6_addr, 16);
@@ -1126,6 +1156,8 @@ void SocketAddressBase::Initialize(Environment* env, Local<Object> target) {
                          "SocketAddress",
                          GetConstructorTemplate(env),
                          SetConstructorFunctionFlag::NONE);
+
+  SetMethod(env->context(), target, "parseSocketAddress", Parse);
 }
 
 BaseObjectPtr<SocketAddressBase> SocketAddressBase::Create(
@@ -1164,6 +1196,20 @@ void SocketAddressBase::New(const FunctionCallbackInfo<Value>& args) {
   new SocketAddressBase(env, args.This(), std::move(addr));
 }
 
+void SocketAddressBase::Parse(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  CHECK(args[0]->IsString());  // input
+
+  Utf8Value input(env->isolate(), args[0]);
+
+  auto addr = std::make_shared<SocketAddress>();
+  if (!SocketAddress::Parse(input.ToStringView(), addr.get())) return;
+
+  BaseObjectPtr<SocketAddressBase> base =
+      SocketAddressBase::Create(env, std::move(addr));
+  if (base) args.GetReturnValue().Set(base->object());
+}
+
 void SocketAddressBase::Detail(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   CHECK(args[0]->IsObject());

src/node_sockaddr.h

@@ -15,6 +15,7 @@
 #include <list>
 #include <memory>
 #include <string>
+#include <string_view>
 #include <unordered_map>
 
 namespace node {
@@ -59,6 +60,9 @@ class SocketAddress : public MemoryRetainer {
 
   static bool New(const char* host, uint32_t port, SocketAddress* addr);
 
+  // Returns true if parsing input as an "ip[:port]" socket address succeeded.
+  static bool Parse(std::string_view input, SocketAddress* addr);
+
   // Returns the port for an IPv4 or IPv6 address.
   inline static int GetPort(const sockaddr* addr);
   inline static int GetPort(const sockaddr_storage* addr);
@@ -157,6 +161,7 @@ class SocketAddressBase : public BaseObject {
       Environment* env, std::shared_ptr<SocketAddress> address);
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void Parse(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Detail(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void LegacyDetail(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void GetFlowLabel(const v8::FunctionCallbackInfo<v8::Value>& args);