Question about sshbuf

[Ron Frederick]

On May 23, 2022, at 3:55 PM, Damien Miller <djm at mindrot.org> wrote:
> 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);


I don’t think that will work, as server_host_key_blob is used outside of the do..while loop. It needs to survive for the lifetime of the call to kexgss_client(). See line 282 for where it is used. 
-- 
Ron Frederick
ronf at timeheart.net