Re: Bugs when using rsa-sha2 (+patches)

[Jakub Jelen <jjelen@xxxxxxxxxx>]

Hello Tilo,
Thank you for the valuable comments. See the comments inline.

On Mon, 2018-11-26 at 10:58 +0100, Tilo Eckert wrote:
> Hello Jakub,
> 
> I explored a similar solution to yours as an alternative, but
> encountered the same ECDSA issue you mentioned which is caused by the
> weird way that sig->type_c is set. It didn't make much sense to me
> that
> ssh_pki_import_signature_blob() reads the signature key type string
> from
> the signature blob, converts it to key type and hash type enums, only
> to
> convert them back to a key type string in pki_signature_from_blob(),
> which does not yield the same string the server sent in case of
> ECDSA.

I assume this is based on the history, when it was very easy to have
type_c representation of the key type as a const string (either RSA or
DSA). This approach gets more complicated when we have to deal with
ECDSA keys and with the new SHA2 extension. It looks like the ECDSA
implementation was not very tested so far. So lets fix it.

> The assignment "sig->type_c = pubkey->type_c" in your solution
> troubles
> me because the key type string from the public key and signature
> portion
> of the KEX message sent by the server are not the same string.

They are the same string for everything but RSA keys with SHA2
extension. There is similar approach used for the ECDSA key import
(setting default for all key types and overwriting for ECDSA based on
the NID). But you are right, we should check first that the key matches
base type of the signature before doing this. Added to my branch.

> If
> anything, sig->type_c should be set to the value of variable "alg" in
> ssh_pki_import_signature_blob(), which contains the actual key type
> string the server included in its signature.

This would be also good solution, and it was also one of the
possibilities that I was exploring initially, but the problem is that
the string in type_c is constant and we would not be able to free it,
unless we would do a lot of refactoring around, which I wanted to
avoid.

> Instead of replacing the "ssh_match_group(..., server_key->type_c)"
> check with "ssh_match_group(..., sig->type_c)", my alternative
> solution
> involved checking sig->type_c in addition to the verification of
> server_key->type_c (as in my previous patch set).

I was also considering this check, but in the end decided it would be
bogus -- the signature verification should not really pass if there
would be key/signature mismatch, unless there would go something
terribly wrong.

But it is always be safe than sorry. See below.

> I was thinking about
> the case when a bad server sends incompatible server_key->type_c and
> sig->type_c (e.g. server sends a public key with type "ssh-dss", but
> the
> key type string in the signature is "ssh-rsa").

This test should go to ssh_pki_signature_verify() to catch the same
possible issue also in other places where we are validating signatures.
And since I am "deriving" the signature type from the key type, the
same check should also to the pki_signature_from_blob(), verifying that
the base type of public key matches the received type of signature,
before we assign the type_c from the public key to the signature.

This can be hardly tested with the real ssh session, but the unit tests
should be able to catch it. I wrote a test to go through all the key
types, try the import of non-matching signatures and verification of
already created and imported keys and really, this could have crash
badly.

I added few more checks and now the flags look sane and the cross-
verify fails when it should.

I added these commits to my branch. Does this address your concerns?

https://gitlab.com/jjelen/libssh-mirror/commits/rsa-sha2-bug

Thank you,
Jakub

> I'm not sure if
> signature import/verification fails correctly in all possible cases.
> I
> didn't thoroughly investigate this, though.
> 
> Kind regards
> Tilo
> 
> Am 22.11.2018 um 11:02 schrieb Jakub Jelen:
> > Hello Tilo,
> > thank you for having a look into that.
> > 
> > I thought about a simpler solution, where you would check just the
> > sig-
> > > type_c value (this will contain the string rsa-sha2-*) against
> > > the
> > list of allowed methods. But having a better look, this value was
> > not
> > properly set for the ECDSA keys (sigh ...), but fixing this for
> > ECDSA
> > makes the code simpler.
> > 
> > I reworked your patch set a bit to something (I hope) is simpler,
> > but
> > should do the same thing:
> > 
> > https://gitlab.com/jjelen/libssh-mirror/commits/rsa-sha2-bug
> > 
> > Could you check if this looks good and works for you?
> > 
> > Thanks,
> > Jakub
> > 
> > 
> > On Wed, 2018-11-21 at 12:03 +0100, Tilo Eckert wrote:
> > > Hi Jakub,
> > > 
> > > the new patch set contains my previous rsa-sha2 related patches
> > > as
> > > well
> > > as fixes for the rsa-sha2 downgrade / ssh-rsa upgrade issues and
> > > the
> > > ECDSA signature bypass when ECC is turned off. Please check if my
> > > fixes
> > > are sufficient.
> > > 
> > > Initially I attempted to verify hostkey and signature algos
> > > separately,
> > > but that didn't work out very well. It would have meant a lot of
> > > redundancy. After realizing that hostkey and signature need to be
> > > considered together, performing the necessary checks in the new
> > > hostkey
> > > type validation function seemed straight-forward.
> > > 
> > > If you or someone else has an idea on how to talk OpenSSH into
> > > being
> > > a
> > > naughty server or if you can come up with some other way to
> > > emulate
> > > an
> > > rsa-sha2 downgrade attack, feel free to add a test.
> > > 
> > > Regards,
> > > Tilo
> > > 
> > > > > The function ssh_pki_signature_verify_blob() is used by
> > > > > server
> > > > > (messages.c) and client code (packet_cb.c). So, I think one
> > > > > way
> > > > > to
> > > > > fix
> > > > > it is to remove the ssh_pki_import_signature_blob() call from
> > > > > ssh_pki_signature_verify_blob(), move it to the two callers
> > > > > and
> > > > > pass
> > > > > sig
> > > > > instead of sig_blob to the verify function. That would allow
> > > > > us
> > > > > to
> > > > > check
> > > > > the signature type in ssh_packet_newkeys() depending on the
> > > > > allowed
> > > > > hostkey types.
> > > > 
> > > > That sounds like a way to go. The
> > > > ssh_pki_signature_verify_blob()
> > > > unfortunately does not have slightest idea what is the allowed
> > > > list
> > > > to
> > > > verify against.
> > > > 
> > > > We will not have the same problem in the server (messages.c),
> > > > because
> > > > server does not have any configuration which would allow to
> > > > disable
> > > > some authentication algorithms.
> > > > 
> > > > > I think there may be another unrelated security issue in
> > > > > ssh_pki_signature_verify_blob(): When compiling without ECC
> > > > > support
> > > > > and
> > > > > the server sends an unexpected ECDSA signature, it would be
> > > > > imported
> > > > > successfully, "if (key->type == SSH_KEYTYPE_ECDSA)" would be
> > > > > true,
> > > > > but
> > > > > the block does nothing. The function returns rc, which is
> > > > > still 0
> > > > > from
> > > > > the import. Maybe I missed something, but this looks to me
> > > > > like a
> > > > > signature verification bypass. We should return SSH_ERROR if
> > > > > ECC
> > > > > is
> > > > > unsupported.
> > > > 
> > > > I think you are right here. Setting the rc = SSH_ERROR in the
> > > > else
> > > > branch should fix this problem, but there might be more issues
> > > > like
> > > > this throughout the code.
> > > > 
> > > > Would you like to submit a patch for these two issues?
> > > > 
> > > > Thanks,
> > > > 
> 
> 
> 
-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.