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