a little note on sshbuf_reset()

[Damien Miller]

On Sat, 3 Feb 2024, PRIVET SUNSET wrote:

> Hello!
> I have a minor observation about code in sshbuf.c, not sure if it would be
> useful, but here it is.
> 
> sshbuf_reset() is currently implemented like this:
> 
> void
> sshbuf_reset(struct sshbuf *buf)
> {
> 	u_char *d;
> 
> 	if (buf->readonly || buf->refcount > 1) {
> 		/* Nonsensical. Just make buffer appear empty */
> 		buf->off = buf->size;
> 		return;
> 	}
> 	if (sshbuf_check_sanity(buf) != 0)
> 		return;
> 	buf->off = buf->size = 0;
> 	if (buf->alloc != SSHBUF_SIZE_INIT) {
> 		if ((d = recallocarray(buf->d, buf->alloc, SSHBUF_SIZE_INIT,
> 		    1)) != NULL) {
> 			buf->cd = buf->d = d;
> 			buf->alloc = SSHBUF_SIZE_INIT;
> 		}
> 	}
> 	explicit_bzero(buf->d, buf->alloc);
> }
> 
> This function allocates a new buffer of size SSHBUF_SIZE_INIT if
> buf->alloc != SSHBUF_SIZE_INIT, which can put buf in an inconsistent
> state if buf->max_size < SSHBUF_SIZE_INIT, because it will make
> buf->alloc > buf->max_size true, which will trigger an error with a
> next call to sshbuf_check_sanity(). For example, struct sshbuf *buf =
> sshbuf_new(); sshbuf_set_max_size(buf, 100); sshbuf_reset(buf); will
> lead to SSH_ERR_INTERNAL_ERROR. This code is of course just for
> demonstration, but the thing is that an sshbuf object can be put into
> invalid state through its public API. Or it is just assumed that no
> one will ever set ->max_size to a value less than SSHBUF_SIZE_INIT?
> Anyway, i thought that all invariants of sshbuf object must be
> preserved by its own API no matter how stupid the use of this API is,
> so i wrote this.
> 
> Also, why a call to sshbuf_check_sanity() in sshbuf_reset() is made
> after dereferencing a pointer which is potentialy a NULL pointer? I
> think a call to sshbuf_check_sanity() should precede other operations.

Thanks for taking a look. Your feedback is quite sensible.


diff --git a/sshbuf.c b/sshbuf.c
index d7f4e4ab6..9eacb4acf 100644
--- a/sshbuf.c
+++ b/sshbuf.c
@@ -197,13 +197,13 @@ sshbuf_reset(struct sshbuf *buf)
 {
 	u_char *d;
 
+	if (sshbuf_check_sanity(buf) != 0)
+		return;
 	if (buf->readonly || buf->refcount > 1) {
 		/* Nonsensical. Just make buffer appear empty */
 		buf->off = buf->size;
 		return;
 	}
-	if (sshbuf_check_sanity(buf) != 0)
-		return;
 	buf->off = buf->size = 0;
 	if (buf->alloc != SSHBUF_SIZE_INIT) {
 		if ((d = recallocarray(buf->d, buf->alloc, SSHBUF_SIZE_INIT,
@@ -249,6 +249,8 @@ sshbuf_set_max_size(struct sshbuf *buf, size_t max_size)
 	SSHBUF_DBG(("set max buf = %p len = %zu", buf, max_size));
 	if ((r = sshbuf_check_sanity(buf)) != 0)
 		return r;
+	if (max_size < SSHBUF_SIZE_INIT)
+		return SSH_ERR_INVALID_ARGUMENT;
 	if (max_size == buf->max_size)
 		return 0;
 	if (buf->readonly || buf->refcount > 1)