On Mon, 23 May 2022, Ron Frederick wrote:
> >> ...snip...
> >> out:
> >> sshbuf_free(server_host_key_blob);
> >>
> >> I believe the problem here is that the call to sshpkt_getb_froms() is
> >> returning an sshbuf in server_host_key_blob which is a reference to
> >> the string being consumed from the packet being read, setting that
> >> original packet as its parent. As a result, the “ssh” buffer now has
> >> a refcount of 2, and when it returns to the top of the do {...} while
> >> and tries to read another packet into “ssh”, it gets the error about
> >> the sshbuf being “read-only” (for good reason).
> >
> > IMO the best fix for this would be to put a sshbuf_free() at the
> > start of the do {} loop (it will handle a NULL argument fine).
>
>
> Thanks Damien for the quick response.
>
> Given that “ssh” is actually a “struct ssh *” and not a “struct sshbuf *”, what would be the right argument to pass into sshbuf_free()? It looks like it would need to be something like ssh->state->incoming_packet, but this calling code probably shouldn’t be poking into the internals like that. I don’t see a wrapper in packet.c which would do a free of that internal buffer.
I'd do this:
> >> /* If we've sent them data, they should reply */
> >> do {
sshbuf_free(server_host_key_blob);
server_host_key_blob = NULL;
> >> type = ssh_packet_read(ssh);
> >> if (type == SSH2_MSG_KEXGSS_HOSTKEY) {
> >> debug("Received KEXGSS_HOSTKEY");
> >> if (server_host_key_blob)
> >> fatal("Server host key received more than once");
> >> if ((r = sshpkt_getb_froms(ssh, &server_host_key_blob)) != 0)
> >> fatal("Failed to read server host key: %s", ssh_err(r));
> >> }
> >> } while (type == SSH2_MSG_KEXGSS_HOSTKEY);
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
