Re: [PATCH] diffie-hellman-group-exchange-sha256

[Aris Adamantiadis <aris@xxxxxxxxxxxx>]

Le 23/01/15 15:40, Yanis Kurganov a écrit :
> It's a final version with modern SSH_MSG_KEY_DH_GEX_REQUEST.
> Some clients (for example, Tera Term) use only this message.
>
> 2015-01-23 13:52 GMT+03:00 Yanis Kurganov <yanis.kurganov@xxxxxxxxx
> <mailto:yanis.kurganov@xxxxxxxxx>>:
>
>     Andreas, sorry, I missed something for server (for group1-14 algos).
>     Patch Diffie-Hellman Group Exchange - attempt 2
>
>
Hi Yanis,

Sorry it took me so much time, but I'm finally on holidays and have more
time to work :)
I have carefully reviewed your patch. Unfortunately, I cannot accept it
as-is. Here is my feedback:
- The patch is too big. Some new functionalities are introduced while
some core code is refactorized. It makes it hard to figure out what
really changed, and also harder to debug or git bisect.
- Some changes are too intrusive and should probably have been discussed
before. For example, the pre-initialization of group1 and group14
parameters that moved into a runtime operation.
- I really don't like the way the new packet handlers are implemented.
It's not your fault, the single function pointers array only works
correctly when the overlapping packets numbers have similar functions.
It's not the case with GEX anymore and it forced you to use a dirty hack
for the multiplexing. I'm implementing right now a way to have
independent callbacks for each key exchange method, so this problem goes
away.
- The current implementation will blindly accept any group that is
provided by the server. I want to check on what OpenSSH does but I'm
certain we should at least check the parameter size and maybe some basic
characteristics (is it a strong prime, is the generator/prime congruency
relation compliant with the RFC).
- I wasn't really expecting the server-side to serve group1 or group14
instead of groups in /etc/ssh/moduli. The whole point of
dh-group-exchange is to use different groups to discourage the use of
fixed groups like group1 & group14 that are more likely to be cracked
and have efficient attacks around. Serving these two groups will solve a
technical problem (client xxx won't connect) but at the cost of
introducing new security problems.

Your code however correctly and neatly implements the packet parsing and
packet sending, together with the corner case of SSH_MSG_GEX_OLD_INIT.
My proposition now is that I'll work on a better way of decoupling the
different key exchange methods, and implement GEX by using as much of
your code as I can. I'll make sure you get your name as the commiter so
you're properly credited for you work.

Best regards,

Aris