Question about sshbuf

[Ron Frederick]

On May 22, 2022, at 7:03 PM, Damien Miller <djm at mindrot.org> wrote:
> On Sat, 21 May 2022, Ron Frederick wrote:
>> I’m trying to build a version of OpenSSH 9.0p1 with the GSSAPI patch
>> at https://sources.debian.org/patches/openssh/1:9.0p1-1/gssapi.patch/.
>> The patch applied just fine and I had no issues with the build or the
>> tests (all of which passed).
>> 
>> That said, I think I’ve discovered a bug in the patch. I know that’s
>> not the responsibility of the OpenSSH maintainers, but I was hoping
>> to get some help from this list on a higher-level question about the
>> right way to work with sshbufs, as I haven’t had a lot of experience
>> actually making changes to OpenSSH.
>> 
>> The issue occurs when the GSS client kex code receives a
>> KEXGSS_HOSTKEY message. There’s code to handle this message, but it
>> runs into an issue where it reports "ssh_packet_read: read: internal
>> error: buffer is read-only”. When I looked into this more closely, the
>> issue is not actually that the sshbuf it is using is read-only. It’s
>> that the sshbuf has a refcount greater than 1 at the time the code
>> tries to reuse it. Here’s the code in question:
>> 
>>        struct sshbuf *server_host_key_blob = NULL;
>> 
>>        ...snip...
>>                        /* If we've sent them data, they should reply */
>>                        do {
>>                                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);
>>        ...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.
-- 
Ron Frederick
ronf at timeheart.net