On 5/22/22 02:17, 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).
There’s already code in place to free server_host_key_blob, so I think the solution here is probably to make a copy of this blob into a new sshbuf rather than getting a reference to the existing memory. However, I’m not sure what the best function is to use in place of sshpkt_getb_froms() which will guarantee a new allocation is performed. Anyone have a suggestion on this? The sshbuf_get_stringb() seems close, but it doesn’t take into account that “ssh” needs to be accessed as a packet. I see various sshpkt_get_string functions, but not specifically an sshpkt_get_stringb(). It wouldn’t be hard to add, but if it’s not already present there’s probably a good reason for that. Is there a better way to do this?
As an aside, I’m guessing this refcounting capability was added after the patch was originally written, and the patch was never updated to consider this issue, meaning there might be other places in the patch where this occurs. Is there anything I can read that might provide more information about which functions began returning references rather than always making copies of buffers?
Thanks in advance for any help you can provide!
We track the gsskex patches in the following github repository:
https://github.com/openssh-gsskex/openssh-gsskex/
I did not read into details about that, but believe I already saw this
issue and we were fixing it:
https://github.com/openssh-gsskex/openssh-gsskex/pull/19
Unfortunately, the repository is not completely up to date, but both
Colin and Dmitry should be able to help you around here.
Regards,
--
Jakub Jelen
Crypto Team, Security Engineering
Red Hat, Inc.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
