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) _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev