Index | Thread | Search

From:
Damien Miller <djm@mindrot.org>
Subject:
sshbuf_get_nulterminated_string()
To:
tech@openbsd.org
Cc:
openssh@openssh.com
Date:
Thu, 20 Nov 2025 11:42:55 +1100

Download raw body.

Thread
  • Damien Miller:

    sshbuf_get_nulterminated_string()

Hi,

This adds a sshbuf_get_nulterminated_string() to OpenSSH's sshbuf code
to extract a null-terminated string from a buffer.

Mostly sshbuf is used to work with the RFC4253 SSH wire encoding,
but there are a few places where we need to interact with data that
is formatted using other rules. One example is the dynamic "-D"
forwarding code, where we need to parse SOCKS headers, and these
do use some nul-terminated strings.

I'd like to rewrite the SOCKS code to use the safe sshbuf for parsing
instead of its current manual string-fiddling.

We also need to support incremental parsing as ssh's channel layer
may not have received a complete SOCKS header. This is supported
by this function returning SSH_ERR_MESSAGE_INCOMPLETE when there
wasn't a nul-terminated string of the maximum desired length in the
buffer.

ok?

Index: usr.bin/ssh/sshbuf-getput-basic.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/sshbuf-getput-basic.c,v
diff -u -p -r1.13 sshbuf-getput-basic.c
--- usr.bin/ssh/sshbuf-getput-basic.c	25 May 2022 06:03:44 -0000	1.13
+++ usr.bin/ssh/sshbuf-getput-basic.c	20 Nov 2025 00:34:41 -0000
@@ -627,3 +627,36 @@ sshbuf_get_bignum2_bytes_direct(struct s
 	}
 	return 0;
 }
+
+int
+sshbuf_get_nulterminated_string(struct sshbuf *buf, size_t maxlen,
+    char **valp, size_t *lenp)
+{
+	u_char zero = 0;
+	const char *val = NULL;
+	size_t len = 0;
+	int r;
+
+	if (valp != NULL)
+		*valp = NULL;
+	if (lenp != NULL)
+		*lenp = 0;
+	if ((r = sshbuf_find(buf, 0, &zero, sizeof(zero), &len)) != 0) {
+		if (r == SSH_ERR_INVALID_FORMAT && sshbuf_len(buf) < maxlen)
+			return SSH_ERR_MESSAGE_INCOMPLETE;
+		return r;
+	}
+	if (len > maxlen)
+		return SSH_ERR_INVALID_FORMAT;
+	/* can strdup() because it's definitely nul-terminated */
+	if ((val = strdup(sshbuf_ptr(buf))) == NULL)
+		return SSH_ERR_ALLOC_FAIL;
+	if ((r = sshbuf_consume(buf, len + 1)) != 0) {
+		free(val);
+		return r;
+	}
+	/* success */
+	*valp = val;
+	*lenp = len;
+	return 0;
+}
Index: usr.bin/ssh/sshbuf.h
===================================================================
RCS file: /cvs/src/usr.bin/ssh/sshbuf.h,v
diff -u -p -r1.32 sshbuf.h
--- usr.bin/ssh/sshbuf.h	2 Sep 2025 09:41:23 -0000	1.32
+++ usr.bin/ssh/sshbuf.h	20 Nov 2025 00:34:41 -0000
@@ -231,6 +231,10 @@ int	sshbuf_put_ec(struct sshbuf *buf, co
 int	sshbuf_put_eckey(struct sshbuf *buf, const EC_KEY *v);
 int	sshbuf_put_ec_pkey(struct sshbuf *buf, EVP_PKEY *pkey);
 
+/* Functions to extract or store various non-SSH wire encoded values */
+int	sshbuf_get_nulterminated_string(struct sshbuf *buf, size_t maxlen,
+	    char **valp, size_t *lenp);
+
 /* Dump the contents of the buffer in a human-readable format */
 void	sshbuf_dump(const struct sshbuf *buf, FILE *f);
 
Index: regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf_getput_basic.c
===================================================================
RCS file: /cvs/src/regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf_getput_basic.c,v
diff -u -p -r1.5 test_sshbuf_getput_basic.c
--- regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf_getput_basic.c	15 Sep 2025 03:00:22 -0000	1.5
+++ regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf_getput_basic.c	20 Nov 2025 00:34:41 -0000
@@ -710,4 +710,53 @@ sshbuf_getput_basic_tests(void)
 	sshbuf_free(p1);
 	free(s2);
 	TEST_DONE();
+
+	TEST_START("sshbuf_get_nulterminated_string");
+	p1 = sshbuf_new();
+	ASSERT_INT_EQ(sshbuf_put(p1, "hello", 5), 0);
+	ASSERT_INT_EQ(sshbuf_put_u8(p1, 0), 0);
+	ASSERT_INT_EQ(sshbuf_put(p1, "there", 5), 0);
+	ASSERT_SIZE_T_EQ(sshbuf_len(p1), 11);
+	/* short maxlen */
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, 1, &s2, &s),
+	    SSH_ERR_INVALID_FORMAT);
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, 4, &s2, &s),
+	    SSH_ERR_INVALID_FORMAT);
+	ASSERT_PTR_EQ(s2, NULL);
+	ASSERT_SIZE_T_EQ(s, 0);
+	ASSERT_SIZE_T_EQ(sshbuf_len(p1), 11);
+	/* minimim usable maxlen */
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, 5, &s2, &s), 0);
+	ASSERT_STRING_EQ(s2, "hello");
+	ASSERT_SIZE_T_EQ(s, 5);
+	ASSERT_SIZE_T_EQ(sshbuf_len(p1), 5);
+	free(s2);
+	/* final string is not nul-terminated */
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, 5, &s2, &s),
+	    SSH_ERR_INVALID_FORMAT);
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, 6, &s2, &s),
+	    SSH_ERR_MESSAGE_INCOMPLETE);
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, SIZE_MAX, &s2, &s),
+	    SSH_ERR_MESSAGE_INCOMPLETE);
+	ASSERT_PTR_EQ(s2, NULL);
+	ASSERT_SIZE_T_EQ(s, 0);
+	/* prepare for round 2 */
+	ASSERT_INT_EQ(sshbuf_put_u8(p1, 0), 0);
+	ASSERT_INT_EQ(sshbuf_put(p1, "it is", 5), 0);
+	ASSERT_SIZE_T_EQ(sshbuf_len(p1), 11);
+	/* maxlen = string length + 1 */
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, 6, &s2, &s), 0);
+	ASSERT_STRING_EQ(s2, "there");
+	ASSERT_SIZE_T_EQ(s, 5);
+	ASSERT_SIZE_T_EQ(sshbuf_len(p1), 5);
+	free(s2);
+	/* maxlen > buffer len */
+	ASSERT_INT_EQ(sshbuf_put_u8(p1, 0), 0);
+	ASSERT_INT_EQ(sshbuf_get_nulterminated_string(p1, SIZE_MAX, &s2, &s), 0);
+	ASSERT_STRING_EQ(s2, "it is");
+	ASSERT_SIZE_T_EQ(s, 5);
+	ASSERT_SIZE_T_EQ(sshbuf_len(p1), 0);
+	free(s2);
+	sshbuf_free(p1);
+	TEST_DONE();
 }