Re: Support for SHA2 HMAC algorithms

[Andreas Schneider <asn@xxxxxxxxxxxxxx>]

On Thursday 10 April 2014 14:35:55 Dirkjan Bussink wrote:
> Hi all,

Hi Dirkjan,

> I’ve written a patch that adds support to libssh for SHA2 HMAC algorithms.
> Newer versions of OpenSSH are disabling SHA1 as the default, so it would be
> good to be prepared here for this future.

That's awesome!

> 
> http://www.openssh.com/cgi-bin/man.cgi?query=sshd_config
> 
> The default is:
> 
> umac-64-etm@xxxxxxxxxxx,umac-128-etm@xxxxxxxxxxx,
> hmac-sha2-256-etm@xxxxxxxxxxx,hmac-sha2-512-etm@xxxxxxxxxxx,
> umac-64@xxxxxxxxxxx,umac-128@xxxxxxxxxxx,
> hmac-sha2-256,hmac-sha2-512
> 
> If this patch needs any changes to be accepted, please let me know.

I have to leave soon so I just did a quick scan. Here are some comments:


crypto->digest_len is size_t

so hmac_digest_len() should return size_t

generate_one_key() should use 'size_t requested'. The name is not the best, 
maybe requested_size.


I don't have to fully go trough the while loop of generate_one_key() but I 
think we might need to check for an overflow here.

In generate_session_keys() for all see if-clause I would like to see two 
lines. It is easier to debug.

int rc;

rc = generate_one_key();
if (rc < 0) {
    ...
}

I know our CodingStyle is horrible but try to apply what you see in pki.c.


in ssh_packet_socket_callback()

if(session->current_crypto) {

please use session->current_crypto == NULL and current_macsize should be 
size_t.


In packet_send2() -> if (ssh_buffer_add_data() < 0) should be two lines, see 
above.


These are only cosmetic things. I will do a in-depth review in the next days.


It would be great if you could submit another patch with testcases!


I think you can copy tests/client/torture_algorithms.c and modify it.


Thanks again for your contribution!


Best regards,


	-- andreas


-- 
Andreas Schneider                   GPG-ID: CC014E3D
www.cryptomilk.org                asn@xxxxxxxxxxxxxx