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)