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). _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
