Re: Support for SHA2 HMAC algorithms

[Dirkjan Bussink <d.bussink@xxxxxxxxx>]

On 10 Apr 2014, at 19:24, Andreas Schneider <asn@xxxxxxxxxxxxxx> wrote:

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

Updated it with these changes.

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

Not really sure what case you’re talking about here. requested_size is not user controlled here, so not sure how a really huge value could be passed in.

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

Updated these.

> in ssh_packet_socket_callback()
> 
> if(session->current_crypto) {
> 
> please use session->current_crypto == NULL and current_macsize should be 
> size_t.

I’ve used != NULL here, since == NULL would be the inverted clause. Or do you intend something else here?

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

Updated.

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

I’ve added test cases to this file for each algorithm tested there in combination with hmac-sha2-256 and hmac-sha2-512.

Let me know if there’s further feedback if you dig in further.

Thanks!

— 
Dirkjan

Attachment: 0001-Add-support-for-SHA2-algorithms.patch
Description: Binary data