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

[Jakub Jelen <jjelen@xxxxxxxxxx>]

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.