Re: Question about sshbuf

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]


On May 22, 2022, at 7:03 PM, Damien Miller <djm@xxxxxxxxxxx> 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
>> 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

openssh-unix-dev mailing list

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux