Bugs when using rsa-sha2 (+patches)

[Tilo Eckert <tilo.eckert@xxxxxxx>]

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.

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. 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.

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 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"). 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,
>>>