Question about sshbuf

[Damien Miller]

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).