Re: [PATCH 0/2] kex: fix RFC8332 RSA extension selection bug

[Jakub Jelen <jjelen@xxxxxxxxxx>]

On Mon, 2019-02-04 at 19:10 -0500, Jon Simons wrote:
> Included here is an update to the pkd tests to reproduce a bug in
> RFC8332 RSA extension selection, as well as a fix which makes the
> test pass.
> 
> When libssh server is provided "rsa-sha2-256,rsa-sha2-512" by the
> client for host key algorithms, it will unconditionally reply using
> the rsa-sha2-512 variant.  But, the server should respect the
> client's preference in this case and use rsa-sha2-256.
> 
> Also available here:
> 
>  * 
> https://github.com/simonsj/libssh/tree/simonsj/patch/fix-rfc8332-bug-2-4-2019
>  * 
> https://gitlab.com/simonsj1/libssh-mirror/tree/simonsj/patch/fix-rfc8332-bug-2-4-2019
> 
> Jon Simons (2):
>   tests/pkd: repro rsa-sha2-{256,512} negotiation bug
>   kex: honor client preference for rsa-sha2-{256,512} host key
>     algorithms
> 
>  src/kex.c              | 24 ++++++++++++++++++++++++
>  tests/pkd/pkd_client.h | 15 +++++++++------
>  tests/pkd/pkd_hello.c  |  8 ++++++++
>  3 files changed, 41 insertions(+), 6 deletions(-)

Hello,
this generally looks good to me. Thank you for having a look into this
and fixing this corner case.

I am only a bit concerned about constructions like

+                } else if (0 == strcmp(rsa_sig_ext, "rsa-sha2-512")) {

I did not find explicit note in the coding guidelines forbidding this,
but I did not find the expression nowhere in the libssh code so I would
rather see it written the other way round:

+                } else if (strcmp(rsa_sig_ext, "rsa-sha2-512") == 0) {

but ideally completely outside of the if-condition as hinted by the
coding style [1].

[1] 
https://gitlab.com/libssh/libssh-mirror/blob/master/README.CodingStyle#L344


Thanks,
-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.