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