On 10 Apr 2014, at 19:24, Andreas Schneider <[email protected]> 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
0001-Add-support-for-SHA2-algorithms.patch
Description: Binary data
