[openssh-commits] [openssh] branch master updated: upstream: rewrite SOCKS4/4A/5 parsing code to use sshbuf functions
git+noreply at mindrot.org
git+noreply at mindrot.org
Sun Jan 4 20:57:05 AEDT 2026
This is an automated email from the git hooks/post-receive script.
djm pushed a commit to branch master
in repository openssh.
The following commit(s) were added to refs/heads/master by this push:
new a6f8f793d upstream: rewrite SOCKS4/4A/5 parsing code to use sshbuf functions
a6f8f793d is described below
commit a6f8f793d427a831be1b350741faa4f34066d55f
Author: djm at openbsd.org <djm at openbsd.org>
AuthorDate: Sun Jan 4 09:52:58 2026 +0000
upstream: rewrite SOCKS4/4A/5 parsing code to use sshbuf functions
instead of manual pointer fiddling. Should make the code safer and easier to
read. feedback/ok markus@
OpenBSD-Commit-ID: 5ebd841fbd78d8395774f002a19c1ddcf91ad047
---
channels.c | 385 +++++++++++++++++++++++++++++++------------------------------
1 file changed, 196 insertions(+), 189 deletions(-)
diff --git a/channels.c b/channels.c
index 80014ff34..efade6d81 100644
--- a/channels.c
+++ b/channels.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.c,v 1.452 2025/10/07 08:02:32 djm Exp $ */
+/* $OpenBSD: channels.c,v 1.453 2026/01/04 09:52:58 djm Exp $ */
/*
* Author: Tatu Ylonen <ylo at cs.hut.fi>
* Copyright (c) 1995 Tatu Ylonen <ylo at cs.hut.fi>, Espoo, Finland
@@ -1507,115 +1507,93 @@ channel_pre_mux_client(struct ssh *ssh, Channel *c)
}
}
+static inline int
+socks_decode_error(Channel *c, int status, const char *func, const char *msg)
+{
+ if (status == SSH_ERR_MESSAGE_INCOMPLETE)
+ return 0;
+ else {
+ debug_r(status, "%s: channel %d: decode %s",
+ func, c->self, msg);
+ return -1;
+ }
+}
+
/* try to decode a socks4 header */
static int
channel_decode_socks4(Channel *c, struct sshbuf *input, struct sshbuf *output)
{
- const u_char *p;
- char *host;
- u_int len, have, i, found, need;
- char username[256];
- struct {
- u_int8_t version;
- u_int8_t command;
- u_int16_t dest_port;
- struct in_addr dest_addr;
- } s4_req, s4_rsp;
- int r;
+ uint8_t socks_ver, socks_cmd, dest_addr[4];
+ uint16_t dest_port;
+ char *user = NULL, *host = NULL;
+ int success = -1, socks4a = 0, r;
+ struct sshbuf *b = NULL;
+
+ if (sshbuf_len(input) < 9)
+ return 0;
+
+ /* We may not have a complete message, so work on a dup of the buffer */
+ if ((b = sshbuf_fromb(input)) == NULL)
+ fatal_f("sshbuf_fromb failed");
debug2("channel %d: decode socks4", c->self);
-
- have = sshbuf_len(input);
- len = sizeof(s4_req);
- if (have < len)
- return 0;
- p = sshbuf_ptr(input);
-
- need = 1;
- /* SOCKS4A uses an invalid IP address 0.0.0.x */
- if (p[4] == 0 && p[5] == 0 && p[6] == 0 && p[7] != 0) {
- debug2("channel %d: socks4a request", c->self);
- /* ... and needs an extra string (the hostname) */
- need = 2;
+ if ((r = sshbuf_get_u8(b, &socks_ver)) != 0 ||
+ (r = sshbuf_get_u8(b, &socks_cmd)) != 0 ||
+ (r = sshbuf_get_u16(b, &dest_port)) != 0 ||
+ (r = sshbuf_get(b, &dest_addr, sizeof(dest_addr))) != 0 ||
+ (r = sshbuf_get_nulterminated_string(b, 1024, &user, NULL)) != 0) {
+ success = socks_decode_error(c, r, __func__, "header");
+ goto out;
}
- /* Check for terminating NUL on the string(s) */
- for (found = 0, i = len; i < have; i++) {
- if (p[i] == '\0') {
- found++;
- if (found == need)
- break;
- }
- if (i > 1024) {
- /* the peer is probably sending garbage */
- debug("channel %d: decode socks4: too long",
- c->self);
- return -1;
+
+ /* Is this a SOCKS4A request? (indicated by an address of 0.0.0.x) */
+ if (dest_addr[0] == 0 && dest_addr[1] == 0 &&
+ dest_addr[2] == 0 && dest_addr[3] != 0) {
+ /* If so, then the hostname follows, also nul-terminated */
+ if ((r = sshbuf_get_nulterminated_string(b, 1024,
+ &host, NULL)) != 0) {
+ success = socks_decode_error(c, r, __func__, "host");
+ goto out;
}
+ socks4a = 1;
+ } else {
+ /* Plain SOCKS4 passes an IPv4 binary address; reconstruct */
+ xasprintf(&host, "%d.%d.%d.%d",
+ dest_addr[0], dest_addr[1], dest_addr[2], dest_addr[3]);
}
- if (found < need)
- return 0;
- if ((r = sshbuf_get(input, &s4_req.version, 1)) != 0 ||
- (r = sshbuf_get(input, &s4_req.command, 1)) != 0 ||
- (r = sshbuf_get(input, &s4_req.dest_port, 2)) != 0 ||
- (r = sshbuf_get(input, &s4_req.dest_addr, 4)) != 0) {
- debug_r(r, "channels %d: decode socks4", c->self);
- return -1;
- }
- have = sshbuf_len(input);
- p = sshbuf_ptr(input);
- if (memchr(p, '\0', have) == NULL) {
- error("channel %d: decode socks4: unterminated user", c->self);
- return -1;
- }
- len = strlen(p);
- debug2("channel %d: decode socks4: user %s/%d", c->self, p, len);
- len++; /* trailing '\0' */
- strlcpy(username, p, sizeof(username));
- if ((r = sshbuf_consume(input, len)) != 0)
+
+ /* We have a complete SOCKS4 message; consume it from input */
+ if ((r = sshbuf_consume_upto_child(input, b)) != 0)
fatal_fr(r, "channel %d: consume", c->self);
+
+ /* Handle the request */
+ debug2("channel %d: %s: user=\"%s\" command=%d destination=[%s]:%d",
+ c->self, socks4a ? "SOCKS4A" : "SOCKS4", user, (int)socks_cmd,
+ host, dest_port);
+ if (socks_cmd != 1) {
+ debug("channel %d: cannot handle %s command 0x%02x",
+ c->self, socks4a ? "SOCKS4A" : "SOCKS4", socks_cmd);
+ goto out;
+ }
free(c->path);
- c->path = NULL;
- if (need == 1) { /* SOCKS4: one string */
- host = inet_ntoa(s4_req.dest_addr);
- c->path = xstrdup(host);
- } else { /* SOCKS4A: two strings */
- have = sshbuf_len(input);
- p = sshbuf_ptr(input);
- if (memchr(p, '\0', have) == NULL) {
- error("channel %d: decode socks4a: host not nul "
- "terminated", c->self);
- return -1;
- }
- len = strlen(p);
- debug2("channel %d: decode socks4a: host %s/%d",
- c->self, p, len);
- len++; /* trailing '\0' */
- if (len > NI_MAXHOST) {
- error("channel %d: hostname \"%.100s\" too long",
- c->self, p);
- return -1;
- }
- c->path = xstrdup(p);
- if ((r = sshbuf_consume(input, len)) != 0)
- fatal_fr(r, "channel %d: consume", c->self);
- }
- c->host_port = ntohs(s4_req.dest_port);
+ c->path = host;
+ host = NULL; /* transferred */
+ c->host_port = dest_port;
- debug2("channel %d: dynamic request: socks4 host %s port %u command %u",
- c->self, c->path, c->host_port, s4_req.command);
+ /* Reply to the SOCKS4 client */
+ if ((r = sshbuf_put_u8(output, 0)) != 0 || /* vn: 0 for reply */
+ (r = sshbuf_put_u8(output, 90)) != 0 || /* cd: req granted */
+ (r = sshbuf_put_u16(output, 0)) != 0 || /* port: ignored */
+ (r = sshbuf_put_u32(output, ntohl(INADDR_ANY))) != 0) /* ignored */
+ fatal_fr(r, "channel %d: compose reply", c->self);
- if (s4_req.command != 1) {
- debug("channel %d: cannot handle: %s cn %d",
- c->self, need == 1 ? "SOCKS4" : "SOCKS4A", s4_req.command);
- return -1;
- }
- s4_rsp.version = 0; /* vn: 0 for reply */
- s4_rsp.command = 90; /* cd: req granted */
- s4_rsp.dest_port = 0; /* ignored */
- s4_rsp.dest_addr.s_addr = INADDR_ANY; /* ignored */
- if ((r = sshbuf_put(output, &s4_rsp, sizeof(s4_rsp))) != 0)
- fatal_fr(r, "channel %d: append reply", c->self);
- return 1;
+ /* success */
+ success = 1;
+ out:
+ sshbuf_free(b);
+ free(user);
+ free(host);
+ return success;
}
/* try to decode a socks5 header */
@@ -1627,73 +1605,110 @@ channel_decode_socks4(Channel *c, struct sshbuf *input, struct sshbuf *output)
#define SSH_SOCKS5_CONNECT 0x01
#define SSH_SOCKS5_SUCCESS 0x00
+/*
+ * Handles SOCKS5 authentication. Note 'b' must be a dup of 'input'
+ * Returns 0 on insufficient queued date, 1 on authentication success or
+ * -1 on error.
+ */
+static int
+channel_socks5_check_auth(Channel *c, struct sshbuf *b, struct sshbuf *input,
+ struct sshbuf *output)
+{
+ uint8_t socks_ver;
+ uint8_t nmethods, method;
+ int r;
+ u_int i, found;
+
+ /* format: ver | nmethods | methods */
+ if ((r = sshbuf_get_u8(b, &socks_ver)) != 0)
+ return socks_decode_error(c, r, __func__, "version");
+ if (socks_ver != 5) /* shouldn't happen; checked by caller^2 */
+ fatal_fr(r, "channel %d: internal error: not socks5", c->self);
+ if ((r = sshbuf_get_u8(b, &nmethods)) != 0)
+ return socks_decode_error(c, r, __func__, "methods");
+ for (found = i = 0; i < nmethods; i++) {
+ if ((r = sshbuf_get_u8(b, &method)) != 0)
+ return socks_decode_error(c, r, __func__, "method");
+ if (method == SSH_SOCKS5_NOAUTH)
+ found = 1;
+ }
+ if (!found) {
+ debug("channel %d: didn't request SSH_SOCKS5_NOAUTH", c->self);
+ return -1;
+ }
+ /* Consume completed request */
+ if ((r = sshbuf_consume_upto_child(input, b)) != 0)
+ fatal_fr(r, "channel %d: consume", c->self);
+
+ /* Compose reply: version, method */
+ if ((r = sshbuf_put_u8(output, 0x05)) != 0 ||
+ (r = sshbuf_put_u8(output, SSH_SOCKS5_NOAUTH)) != 0)
+ fatal_fr(r, "channel %d: append reply", c->self);
+ /* success */
+ debug2("channel %d: socks5 auth done", c->self);
+ return 1;
+}
+
static int
channel_decode_socks5(Channel *c, struct sshbuf *input, struct sshbuf *output)
{
- /* XXX use get/put_u8 instead of trusting struct padding */
- struct {
- u_int8_t version;
- u_int8_t command;
- u_int8_t reserved;
- u_int8_t atyp;
- } s5_req, s5_rsp;
- u_int16_t dest_port;
+ uint8_t socks_ver, socks_cmd, socks_reserved, socks_atyp, addrlen;
+ uint16_t dest_port;
char dest_addr[255+1], ntop[INET6_ADDRSTRLEN];
- const u_char *p;
- u_int have, need, i, found, nmethods, addrlen, af;
- int r;
+ u_int af;
+ int r, success = -1;;
+ struct sshbuf *b = NULL;
+
+ debug2("channel %d: decode socks5 %s", c->self,
+ (c->flags & SSH_SOCKS5_AUTHDONE) ? "request" : "auth");
+
+ /* We may not have a complete message, so work on a dup of the buffer */
+ if ((b = sshbuf_fromb(input)) == NULL)
+ fatal_f("sshbuf_fromb failed");
- debug2("channel %d: decode socks5", c->self);
- p = sshbuf_ptr(input);
- if (p[0] != 0x05)
- return -1;
- have = sshbuf_len(input);
if (!(c->flags & SSH_SOCKS5_AUTHDONE)) {
- /* format: ver | nmethods | methods */
- if (have < 2)
- return 0;
- nmethods = p[1];
- if (have < nmethods + 2)
- return 0;
- /* look for method: "NO AUTHENTICATION REQUIRED" */
- for (found = 0, i = 2; i < nmethods + 2; i++) {
- if (p[i] == SSH_SOCKS5_NOAUTH) {
- found = 1;
- break;
- }
+ if ((r = channel_socks5_check_auth(c, b, input, output)) != 1) {
+ success = r;
+ goto out;
}
- if (!found) {
- debug("channel %d: method SSH_SOCKS5_NOAUTH not found",
- c->self);
- return -1;
- }
- if ((r = sshbuf_consume(input, nmethods + 2)) != 0)
- fatal_fr(r, "channel %d: consume", c->self);
- /* version, method */
- if ((r = sshbuf_put_u8(output, 0x05)) != 0 ||
- (r = sshbuf_put_u8(output, SSH_SOCKS5_NOAUTH)) != 0)
- fatal_fr(r, "channel %d: append reply", c->self);
c->flags |= SSH_SOCKS5_AUTHDONE;
- debug2("channel %d: socks5 auth done", c->self);
- return 0; /* need more */
+ /* Continue to parse request in case client speculated ahead */
}
+
+ /* Request messages (auth or connect) always start with the version */
+ if ((r = sshbuf_get_u8(b, &socks_ver)) != 0) {
+ success = socks_decode_error(c, r, __func__, "version");
+ goto out;
+ }
+ if (socks_ver != 5) /* shouldn't happen */
+ fatal_fr(r, "channel %d: internal error: not socks5", c->self);
+
+ /* Parse SOCKS5 request header */
debug2("channel %d: socks5 post auth", c->self);
- if (have < sizeof(s5_req)+1)
- return 0; /* need more */
- memcpy(&s5_req, p, sizeof(s5_req));
- if (s5_req.version != 0x05 ||
- s5_req.command != SSH_SOCKS5_CONNECT ||
- s5_req.reserved != 0x00) {
+ if ((r = sshbuf_get_u8(b, &socks_cmd)) != 0 ||
+ (r = sshbuf_get_u8(b, &socks_reserved)) != 0 ||
+ (r = sshbuf_get_u8(b, &socks_atyp)) != 0) {
+ success = socks_decode_error(c, r, __func__, "request header");
+ goto out;
+ }
+
+ if (socks_ver != 0x05 ||
+ socks_cmd != SSH_SOCKS5_CONNECT ||
+ socks_reserved != 0x00) {
debug2("channel %d: only socks5 connect supported", c->self);
- return -1;
+ goto out;
}
- switch (s5_req.atyp){
+
+ switch (socks_atyp) {
case SSH_SOCKS5_IPV4:
addrlen = 4;
af = AF_INET;
break;
case SSH_SOCKS5_DOMAIN:
- addrlen = p[sizeof(s5_req)];
+ if ((r = sshbuf_get_u8(b, &addrlen)) != 0) {
+ success = socks_decode_error(c, r, __func__, "addrlen");
+ goto out;
+ }
af = -1;
break;
case SSH_SOCKS5_IPV6:
@@ -1701,57 +1716,48 @@ channel_decode_socks5(Channel *c, struct sshbuf *input, struct sshbuf *output)
af = AF_INET6;
break;
default:
- debug2("channel %d: bad socks5 atyp %d", c->self, s5_req.atyp);
- return -1;
+ debug2("channel %d: bad socks5 atyp %d", c->self, socks_atyp);
+ goto out;
}
- need = sizeof(s5_req) + addrlen + 2;
- if (s5_req.atyp == SSH_SOCKS5_DOMAIN)
- need++;
- if (have < need)
- return 0;
- if ((r = sshbuf_consume(input, sizeof(s5_req))) != 0)
- fatal_fr(r, "channel %d: consume", c->self);
- if (s5_req.atyp == SSH_SOCKS5_DOMAIN) {
- /* host string length */
- if ((r = sshbuf_consume(input, 1)) != 0)
- fatal_fr(r, "channel %d: consume", c->self);
- }
- if ((r = sshbuf_get(input, &dest_addr, addrlen)) != 0 ||
- (r = sshbuf_get(input, &dest_port, 2)) != 0) {
- debug_r(r, "channel %d: parse addr/port", c->self);
- return -1;
+ if ((r = sshbuf_get(b, &dest_addr, addrlen)) != 0 ||
+ (r = sshbuf_get_u16(b, &dest_port)) != 0) {
+ success = socks_decode_error(c, r, __func__, "addr/port");
+ goto out;
}
dest_addr[addrlen] = '\0';
+
+ /* We have a complete SOCKS5 request; consume it from input */
+ if ((r = sshbuf_consume_upto_child(input, b)) != 0)
+ fatal_fr(r, "channel %d: consume", c->self);
+
free(c->path);
c->path = NULL;
- if (s5_req.atyp == SSH_SOCKS5_DOMAIN) {
- if (addrlen >= NI_MAXHOST) {
- error("channel %d: dynamic request: socks5 hostname "
- "\"%.100s\" too long", c->self, dest_addr);
- return -1;
- }
+ if (socks_atyp == SSH_SOCKS5_DOMAIN)
c->path = xstrdup(dest_addr);
- } else {
+ else {
if (inet_ntop(af, dest_addr, ntop, sizeof(ntop)) == NULL)
return -1;
c->path = xstrdup(ntop);
}
- c->host_port = ntohs(dest_port);
+ c->host_port = dest_port;
debug2("channel %d: dynamic request: socks5 host %s port %u command %u",
- c->self, c->path, c->host_port, s5_req.command);
+ c->self, c->path, c->host_port, socks_cmd);
- s5_rsp.version = 0x05;
- s5_rsp.command = SSH_SOCKS5_SUCCESS;
- s5_rsp.reserved = 0; /* ignored */
- s5_rsp.atyp = SSH_SOCKS5_IPV4;
- dest_port = 0; /* ignored */
-
- if ((r = sshbuf_put(output, &s5_rsp, sizeof(s5_rsp))) != 0 ||
- (r = sshbuf_put_u32(output, ntohl(INADDR_ANY))) != 0 ||
- (r = sshbuf_put(output, &dest_port, sizeof(dest_port))) != 0)
+ /* Reply */
+ if ((r = sshbuf_put_u8(output, 0x05)) != 0 || /* version */
+ (r = sshbuf_put_u8(output, SSH_SOCKS5_SUCCESS)) != 0 || /* cmd */
+ (r = sshbuf_put_u8(output, 0)) != 0 || /* reserved, ignored */
+ (r = sshbuf_put_u8(output, SSH_SOCKS5_IPV4)) != 0 || /* addrtype */
+ (r = sshbuf_put_u32(output, ntohl(INADDR_ANY))) != 0 || /* addr */
+ (r = sshbuf_put_u16(output, dest_port)) != 0) /* port */
fatal_fr(r, "channel %d: append reply", c->self);
- return 1;
+
+ /* success */
+ success = 1;
+ out:
+ sshbuf_free(b);
+ return success;
}
Channel *
@@ -1783,9 +1789,9 @@ channel_connect_stdio_fwd(struct ssh *ssh,
static void
channel_pre_dynamic(struct ssh *ssh, Channel *c)
{
- const u_char *p;
u_int have;
- int ret;
+ u_char ver;
+ int r, ret;
c->io_want = 0;
have = sshbuf_len(c->input);
@@ -1798,9 +1804,9 @@ channel_pre_dynamic(struct ssh *ssh, Channel *c)
return;
}
/* try to guess the protocol */
- p = sshbuf_ptr(c->input);
- /* XXX sshbuf_peek_u8? */
- switch (p[0]) {
+ if ((r = sshbuf_peek_u8(c->input, 0, &ver)) != 0)
+ fatal_fr(r, "sshbuf_peek_u8");
+ switch (ver) {
case 0x04:
ret = channel_decode_socks4(c, c->input, c->output);
break;
@@ -1808,6 +1814,7 @@ channel_pre_dynamic(struct ssh *ssh, Channel *c)
ret = channel_decode_socks5(c, c->input, c->output);
break;
default:
+ debug2_f("channel %d: unknown SOCKS version %u", c->self, ver);
ret = -1;
break;
}
--
To stop receiving notification emails like this one, please contact
djm at mindrot.org.
More information about the openssh-commits
mailing list