From: Damien Miller Subject: sshbuf_consume_upto_child To: tech@openbsd.org Cc: openssh@openssh.com Date: Fri, 21 Nov 2025 14:57:32 +1100 Hi, This is another piece of refactoring the SOCKS code in ssh - adding a sshbuf_consume_upto_child() function. The sshbuf API supports a notion of parent/child buffers. A child buffer may be created from an existing buffer, which becomes a parenti until the child buffer is deleted using sshbuf_free(). The data in a parent is read-only for as long as it has any child buffers, and only read operations (e.g. sshbuf_get_*) are permitted. Child buffer data is always read-only. I'd like to make it easier to use this API to do incremental parsing of potentially-incomplete headers, because we can't know a priori whether we have a complete SOCKS header in the buffer. So I'd like to do the following on a dynamic forwarding channel whenever we receive data: 1. Make a child buffer of the current queued channel data 2. Attempt to parse the full SOCKS header from the child buffer 3. If the header is complete then process the request and consume the header from the parent buffer. 4. If the header is incomplete then free the child buffer (leaving the parent buffer intact) and try again when more data is received. sshbuf_consume_upto_child() helps with step #3 - given a parent and a child buffer, it will update the parent to match data consumed in the child. It will fail if passed unrelated buffers or if the parent has advanced past the child (i.e. had has more data read from it) before the call to sshbuf_consume_upto_child(). Ok? Index: usr.bin/ssh/sshbuf.c =================================================================== RCS file: /cvs/src/usr.bin/ssh/sshbuf.c,v diff -u -p -r1.23 sshbuf.c --- usr.bin/ssh/sshbuf.c 14 Aug 2024 15:42:18 -0000 1.23 +++ usr.bin/ssh/sshbuf.c 21 Nov 2025 03:57:05 -0000 @@ -423,3 +423,23 @@ sshbuf_consume_end(struct sshbuf *buf, s return 0; } +int +sshbuf_consume_upto_child(struct sshbuf *buf, const struct sshbuf *child) +{ + int r; + + if ((r = sshbuf_check_sanity(buf)) != 0 || + (r = sshbuf_check_sanity(child)) != 0) + return r; + /* This function is only used for parent/child buffers */ + if (child->parent != buf) + return SSH_ERR_INVALID_ARGUMENT; + /* Nonsensical if the parent has advanced past the child */ + if (sshbuf_len(child) > sshbuf_len(buf)) + return SSH_ERR_INVALID_ARGUMENT; + /* More paranoia, shouldn't happen */ + if (child->cd < buf->cd) + return SSH_ERR_INTERNAL_ERROR; + /* Advance */ + return sshbuf_consume(buf, sshbuf_len(buf) - sshbuf_len(child)); +} Index: usr.bin/ssh/sshbuf.h =================================================================== RCS file: /cvs/src/usr.bin/ssh/sshbuf.h,v diff -u -p -r1.33 sshbuf.h --- usr.bin/ssh/sshbuf.h 21 Nov 2025 01:29:06 -0000 1.33 +++ usr.bin/ssh/sshbuf.h 21 Nov 2025 03:57:05 -0000 @@ -149,6 +149,24 @@ int sshbuf_consume(struct sshbuf *buf, s */ int sshbuf_consume_end(struct sshbuf *buf, size_t len); +/* + * Consume data from a parent buffer up to that of a child buffer (i.e. + * one created by sshbuf_fromb()). + * + * Intended to be used in a pattern like: + * + * b = sshbuf_fromb(parent); + * sshbuf_get_string(b, &foo, &foostr); + * sshbuf_get_u32(b, &bar); + * sshbuf_consume_upto_child(parent, b); + * + * After which, both "b" and "parent" will point to the same data. + * + * "child" must be a direct child of "buf" (i.e. neither an unrelated buffer + * nor a grandchild) which has consumed data past that of "buf". + */ +int sshbuf_consume_upto_child(struct sshbuf *buf, const struct sshbuf *child); + /* Extract or deposit some bytes */ int sshbuf_get(struct sshbuf *buf, void *v, size_t len); int sshbuf_put(struct sshbuf *buf, const void *v, size_t len); Index: regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf.c =================================================================== RCS file: /cvs/src/regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf.c,v diff -u -p -r1.2 test_sshbuf.c --- regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf.c 14 Dec 2021 21:25:27 -0000 1.2 +++ regress/usr.bin/ssh/unittests/sshbuf/test_sshbuf.c 21 Nov 2025 03:57:05 -0000 @@ -24,7 +24,8 @@ void sshbuf_tests(void); void sshbuf_tests(void) { - struct sshbuf *p1; + struct sshbuf *p1, *p2, *p3; + u_int v32; const u_char *cdp; u_char *dp; size_t sz; @@ -233,5 +234,39 @@ sshbuf_tests(void) ASSERT_SIZE_T_EQ(sshbuf_len(p1), 0); ASSERT_SIZE_T_EQ(sshbuf_avail(p1), 1223); sshbuf_free(p1); + TEST_DONE(); + + TEST_START("sshbuf_consume_upto_child"); + p1 = sshbuf_new(); + ASSERT_PTR_NE(p1, NULL); + p2 = sshbuf_new(); + ASSERT_PTR_NE(p2, NULL); + /* Unrelated buffers */ + ASSERT_INT_EQ(sshbuf_consume_upto_child(p1, p2), + SSH_ERR_INVALID_ARGUMENT); + /* Simple success case */ + ASSERT_INT_EQ(sshbuf_put_u32(p1, 0xdeadbeef), 0); + ASSERT_INT_EQ(sshbuf_put_u32(p1, 0x01020304), 0); + ASSERT_INT_EQ(sshbuf_put_u32(p1, 0xfeedface), 0); + ASSERT_SIZE_T_EQ(sshbuf_len(p1), 12); + p3 = sshbuf_fromb(p1); + ASSERT_PTR_NE(p3, NULL); + ASSERT_INT_EQ(sshbuf_get_u32(p3, &v32), 0); + ASSERT_U32_EQ(v32, 0xdeadbeef); + ASSERT_SIZE_T_EQ(sshbuf_len(p3), 8); + ASSERT_INT_EQ(sshbuf_consume_upto_child(p1, p3), 0); + ASSERT_SIZE_T_EQ(sshbuf_len(p1), sshbuf_len(p3)); + ASSERT_PTR_EQ(sshbuf_ptr(p1), sshbuf_ptr(p3)); + sshbuf_free(p3); + /* Parent already consumed past child */ + p3 = sshbuf_fromb(p1); + ASSERT_PTR_NE(p3, NULL); + ASSERT_INT_EQ(sshbuf_get_u32(p1, &v32), 0); + ASSERT_U32_EQ(v32, 0x01020304); + ASSERT_INT_EQ(sshbuf_consume_upto_child(p1, p3), + SSH_ERR_INVALID_ARGUMENT); + sshbuf_free(p1); + sshbuf_free(p2); + sshbuf_free(p3); TEST_DONE(); }