Re: Support for SHA2 HMAC algorithms

[Aris Adamantiadis <aris@xxxxxxxxxxxx>]

Hi Dirkjan,

Thanks a lot for your contribution, that's going to be very useful. Just
a little remark:
+unsigned char *packet_encrypt(ssh_session session, void *data, uint32_t
len, enum ssh_hmac_e type) {
Would it be possible to remove the "type" parameter and have
packet_encrypt() function figure out by itself the type of hmac to use ?

I also think the hmac code could win at being a little more OO-styled
like we do for the ciphers (with function pointers instead of switch
statements). If we could get rid of our horrible
MD5()/SHA1()/SHA256()/... wrappers by using only EVP functions, it would
help (but probably out of scope of your patch).

Thanks,

Aris
Le 11/04/14 11:00, Dirkjan Bussink a écrit :
> 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
>