Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unbreak gsi-openssh by not holding the sshbuf structures originated f… #19

Open
wants to merge 1 commit into
base: fedora/master
Choose a base branch
from

Conversation

Jakuje
Copy link
Member

@Jakuje Jakuje commented Mar 2, 2021

…rom incoming packet buffer

Should fix #18

(the PR is based on my current Fedora with OpenSSH 8.5p1 (pre-release) branch but it should apply also to the older one with possible slight tweaks)

kexgssc.c Outdated
do {
type = ssh_packet_read(ssh);
if (type == SSH2_MSG_KEXGSS_HOSTKEY) {
char *tmp = NULL;
size_t tmp_len = 0;
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)
if ((r = sshpkt_get_string(ssh, &tmp, &tmp_len)) != 0)
fatal("sshpkt failed: %s", ssh_err(r));
if ((server_host_key_blob = sshbuf_from(tmp, tmp_len)) == NULL)
fatal("sshbuf_from failed");
}
} while (type == SSH2_MSG_KEXGSS_HOSTKEY);

Copy link

@fscheiner fscheiner Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that change be made in the kexgss_client() function? Currently it's made in kexgssgex_client() and GSS GEX was always working for me and actually the client also always worked for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the servers are not sending hostkeys as part of the gss key exchange:

https://github.com/openssh-gsskex/openssh-gsskex/pull/19/files#diff-67c1dce8b9dfa4db6593655232d7fdcffd2a607e9ebd3522a7d2cf97e1d26547R137

I am not sure if there is actually any other implementation that would do that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the servers are not sending hostkeys as part of the gss key exchange:

https://github.com/openssh-gsskex/openssh-gsskex/pull/19/files#diff-67c1dce8b9dfa4db6593655232d7fdcffd2a607e9ebd3522a7d2cf97e1d26547R137

Interesting. But when I had packet debugging enabled I could see the server sending packets that included data that looked like a certificate or a certificate chain as it included the server's DN and the signer's DN and the DN that signed the signer. Strange. :-/

But still, the change is in the wrong function - kexgssgex_client() instead of kexgss_client() - , or do I misunderstand something here?

Apart from that I just built a GSI-OpenSSH 8.0p1 with the patch to kexgss_server() and...

....this works! :-)

[johndoe@host ~]$ gsissh -vvv -oGSSAPIKexAlgorithms=gss-curve25519-sha256- -p 2222 host.domain.tld
OpenSSH_8.0p1c-GSI GSI-hpn14v19, OpenSSL 1.1.1c FIPS  28 May 2019
[...]
debug1: Local version string SSH-2.0-OpenSSH_8.0p1c-GSI GSI-hpn14v19
debug1: Remote protocol version 2.0, remote software version OpenSSH_8.0p1c-GSI GSI-hpn14v19
[...]
debug1: kex: algorithm: gss-curve25519-sha256-vz8J1E9PzLr8b1K+0remTg==
debug1: kex: host key algorithm: ecdsa-sha2-nistp256
debug1: REQUESTED ENC.NAME is 'aes256-gcm@openssh.com'
debug1: kex: server->client cipher: aes256-gcm@openssh.com MAC: <implicit> compression: none
debug1: REQUESTED ENC.NAME is 'aes256-gcm@openssh.com'
debug1: kex: client->server cipher: aes256-gcm@openssh.com MAC: <implicit> compression: none
debug1: kex: gss-curve25519-sha256-vz8J1E9PzLr8b1K+0remTg== need=32 dh_need=32
debug1: kex: gss-curve25519-sha256-vz8J1E9PzLr8b1K+0remTg== need=32 dh_need=32
debug1: Calling gss_init_sec_context
debug1: Delegating credentials
debug3: send packet: type 30
debug3: receive packet: type 31
debug1: Received GSSAPI_CONTINUE
debug1: Calling gss_init_sec_context
debug1: Delegating credentials
debug3: send packet: type 31
debug3: receive packet: type 31
debug1: Received GSSAPI_CONTINUE
debug1: Calling gss_init_sec_context
debug1: Delegating credentials
debug3: send packet: type 31
debug3: receive packet: type 31
debug1: Received GSSAPI_CONTINUE
debug1: Calling gss_init_sec_context
debug1: Delegating credentials
debug3: send packet: type 31
debug3: receive packet: type 32
debug1: Received GSSAPI_COMPLETE
[...]
debug1: Authentication succeeded (gssapi-keyex).
Authenticated to host.domain.tld ([172.16.1.34]:2222).
[...]
[johndoe2@host ~]$ 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. It should be in both of them as both of them use the sshpkt_getb_froms() function which is causing this issue. I was putting this together late in the evening yesterday so I did not notice it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I created new GSI-OpenSSH RPMs with these client changes in and the resulting binaries also work for me (tested both with gss-curve25519-sha256- and gss-gex-sha1- methods). Great work @Jakuje! :-)

How should we integrate that into GSI-OpenSSH 8.0p1 (for EPEL8)? Should I provide the changes from this PR here as additional patch to the maintainer (@ellert) or should we just adapt the "openssh-8.0p1-gssapi-keyex.patch" patch over at https://src.fedoraproject.org/rpms/gsi-openssh/tree/epel8?

I will also go through the newer GSI-OpenSSH versions - 8.3p1 and 8.4p1 ATM - and check with the changes from this PR here.


Slightly off topic: Is GSS group exchange generally flawed or why wasn't a SHA256 based version implemented?

…rom incoming packet buffer

Keeping buffers from sshpkt_getb_froms() for breaks further packet
processing because the "derived" buffer keeps a "link" to te parent
buffer, which is then considered readonly.

This addresses the visible issue in the kexgss_server(), which
demonstrated with GSI (requiring more round trips than GSSAPI
with kerberos).

The additional two places in the client were never hit, because the host
keys are never sent as part of the gssapi key exchange but in case we
would have different server or we would start sending hostkeys as the
code is ready for that, we would hit it anyway.

Fixes openssh-gsskex#18
@Jakuje
Copy link
Member Author

Jakuje commented Mar 3, 2021

Filling a epel8 bug would be probably a way to get this fixed in the gsi-openssh in EPEL8.

I would like @cjwatson to have a look into this change too before we will merge it and update original openssh packages in Fedora (and Debian probably too).

In theory, for Fedora, you should not need this separate change but it should go into the original openssh package (from where the gsi-openssh inherits). But it might take some time so if you need it fast, separate Fedora bugs are way to go.

What do you mean by flawed. I am not aware of any issues with group exchange (except for not being FIPS compliant recently).

@fscheiner
Copy link

What do you mean by flawed. I am not aware of any issues with group exchange (except for not being FIPS compliant recently).

My impression was, that SHA1 based GSS KEX methods should be replaced by SHA256 based ones. But as there didn't realize a SHA256 based GEX method I assumed group exchange might be flawed in some other way, too, apart from being SHA1 based.

@Jakuje
Copy link
Member Author

Jakuje commented Mar 3, 2021

AFAIK they can be flawed when non-prime or some arbitrary chosen numbers/groups are used for the group exchange. The group exchange was perceived to have advantage against other DH kex methods as it was not based on a limited list of (NIST?) "approved" primes/groups, which might be specially crafted to have some properties or that there might be some pre-computation attacks against these. Using SHA1 can make it easier for attacker to affect the prime selection in theory (if it would be even more flawed than believed these days).

NIST solved this in FIPS by allowing group exchange only with the approved primes (the MODP groups used in other key exchange methods) basically downgrading this method to normal DH key exchange (from my basic understanding). So if you want something independent, gss-curve25519-sha256- should be your choice.

@fscheiner
Copy link

Filling a epel8 bug would be probably a way to get this fixed in the gsi-openssh in EPEL8.
In theory, for Fedora, you should not need this separate change but it should go into the original openssh package (from where the gsi-openssh inherits). But it might take some time so if you need it fast, separate Fedora bugs are way to go.

Ok, I'll check with the maintainer of GSI-OpenSSH. @ellert: what would you prefer, separate bugs for Fedora/EPEL or pulling the changes from the to be updated OpenSSH packages from Fedora/EPEL?

@fscheiner
Copy link

AFAIK they can be flawed when non-prime or some arbitrary chosen numbers/groups are used for the group exchange. The group exchange was perceived to have advantage against other DH kex methods as it was not based on a limited list of (NIST?) "approved" primes/groups, which might be specially crafted to have some properties or that there might be some pre-computation attacks against these. Using SHA1 can make it easier for attacker to affect the prime selection in theory (if it would be even more flawed than believed these days).

NIST solved this in FIPS by allowing group exchange only with the approved primes (the MODP groups used in other key exchange methods) basically downgrading this method to normal DH key exchange (from my basic understanding). So if you want something independent, gss-curve25519-sha256- should be your choice.

Thanks for the explanation. And it's really nice and also a relief to have curve25519 available for GSS.

@fscheiner
Copy link

fscheiner commented Mar 4, 2021

@Jakuje: Here's the howto for the installation and setup of GSI-OpenSSH. If something doesn't work as expected, please contact me.

=> moved to #18 (#18 (comment))

Copy link

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@cjwatson cjwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay! I'm not very familiar with the packet/buffer internals, but after some reading around I agree that this seems like a reasonable change.

@Jakuje Jakuje mentioned this pull request Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants