From 600a1caeb2312fdee5ef1caf7d613c12a8b2424a Mon Sep 17 00:00:00 2001
From: Mike Crowe <mac@mcrowe.com>
Date: Wed, 11 Oct 2023 20:50:28 +0100
Subject: [PATCH] socks: return error if hostname too long for remote resolve
To: libcurl development <curl-library@cool.haxx.se>

Prior to this change the state machine attempted to change the remote
resolve to a local resolve if the hostname was longer than 255
characters. Unfortunately that did not work as intended and caused a
security issue.

Name resolvers cannot resolve hostnames longer than 255 characters.

Bug: https://curl.se/docs/CVE-2023-38545.html

Unfortunately CURLE_PROXY and CURLPX_LONG_HOSTNAME were introduced in
7.73.0 so they can't be used in 7.69.1. Let's use
CURLE_COULDNT_RESOLVE_HOST as the best available alternative and update
the test appropriately.

libcurl's test support has been improved considerably since 7.69.1 which
means that the test must be modified to remove use of %VERSION and
%TESTNUMBER and the stderr output can no longer be checked.

CVE: CVE-2023-38545
Upstream-Status: Backport [fb4415d8aee6c1045be932a34fe6107c2f5ed147]
Signed-off-by: Mike Crowe <mac@mcrowe.com>
---
 lib/socks.c             | 13 +++++----
 tests/data/Makefile.inc |  2 +-
 tests/data/test728      | 60 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 6 deletions(-)
 create mode 100644 tests/data/test728

diff --git a/lib/socks.c b/lib/socks.c
index 37099130e..f3bf40533 100644
--- a/lib/socks.c
+++ b/lib/socks.c
@@ -521,11 +521,14 @@ CURLcode Curl_SOCKS5(const char *proxy_user,
       infof(conn->data, "SOCKS5: connecting to HTTP proxy %s port %d\n",
             hostname, remote_port);
 
-    /* RFC1928 chapter 5 specifies max 255 chars for domain name in packet */
+    /* RFC1928 chapter 5 specifies max 255 chars for domain name in packet. */
     if(!socks5_resolve_local && hostname_len > 255) {
-      infof(conn->data, "SOCKS5: server resolving disabled for hostnames of "
-            "length > 255 [actual len=%zu]\n", hostname_len);
-      socks5_resolve_local = TRUE;
+      failf(data, "SOCKS5: the destination hostname is too long to be "
+            "resolved remotely by the proxy.");
+      /* This version of libcurl doesn't have CURLE_PROXY and
+       * therefore CURLPX_LONG_HOSTNAME, so let's report the best we
+       * can. */
+      return CURLE_COULDNT_RESOLVE_HOST;
     }
 
     if(auth & ~(CURLAUTH_BASIC | CURLAUTH_GSSAPI))
@@ -837,7 +840,7 @@ CURLcode Curl_SOCKS5(const char *proxy_user,
 
     if(!socks5_resolve_local) {
       socksreq[len++] = 3; /* ATYP: domain name = 3 */
-      socksreq[len++] = (char) hostname_len; /* one byte address length */
+      socksreq[len++] = (unsigned char) hostname_len; /* one byte length */
       memcpy(&socksreq[len], hostname, hostname_len); /* address w/o NULL */
       len += hostname_len;
       infof(data, "SOCKS5 connect to %s:%d (remotely resolved)\n",
diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc
index 3d8565c36..5ee2284ff 100644
--- a/tests/data/Makefile.inc
+++ b/tests/data/Makefile.inc
@@ -89,7 +89,7 @@ test662 test663 test664 test665 test666 test667 test668 \
 test670 test671 test672 test673 \
 \
 test700 test701 test702 test703 test704 test705 test706 test707 test708 \
-test709 test710 test711 test712 test713 test714 test715 test716 test717 \
+test709 test710 test711 test712 test713 test714 test715 test716 test717 test728 \
 \
 test800 test801 test802 test803 test804 test805 test806 test807 test808 \
 test809 test810 test811 test812 test813 test814 test815 test816 test817 \
diff --git a/tests/data/test728 b/tests/data/test728
new file mode 100644
index 000000000..7b1d8b2f3
--- /dev/null
+++ b/tests/data/test728
@@ -0,0 +1,60 @@
+<testcase>
+<info>
+<keywords>
+HTTP
+HTTP GET
+SOCKS5
+SOCKS5h
+followlocation
+</keywords>
+</info>
+
+#
+# Server-side
+<reply>
+# The hostname in this redirect is 256 characters and too long (> 255) for
+# SOCKS5 remote resolve. curl must return error CURLE_PROXY in this case.
+<data>
+HTTP/1.1 301 Moved Permanently
+Location: http://AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/
+Content-Length: 0
+Connection: close
+
+</data>
+</reply>
+
+#
+# Client-side
+<client>
+<features>
+proxy
+</features>
+<server>
+http
+socks5
+</server>
+ <name>
+SOCKS5h with HTTP redirect to hostname too long
+ </name>
+ <command>
+--no-progress-meter --location --proxy socks5h://%HOSTIP:%SOCKSPORT http://%HOSTIP:%HTTPPORT/728
+</command>
+</client>
+
+#
+# Verify data after the test has been "shot"
+<verify>
+<strip>
+^User-Agent:.*
+</strip>
+<protocol>
+GET /728 HTTP/1.1
+Host: %HOSTIP:%HTTPPORT
+Accept: */*
+
+</protocol>
+<errorcode>
+6
+</errorcode>
+</verify>
+</testcase>
-- 
2.39.2