Re: [PATCH] Allow SSH2_MSG_EXT_INFO when authenticated

[Jakub Jelen <jjelen@xxxxxxxxxx>]

On Tue, 2018-12-11 at 07:22 -0500, Anderson Sasaki wrote:
> Hello,
> 
> ----- Original Message -----
> > From: "Jakub Jelen" <jjelen@xxxxxxxxxx>
> > To: libssh@xxxxxxxxxx
> > Sent: Tuesday, December 11, 2018 12:10:34 PM
> > Subject: Re: [PATCH] Allow SSH2_MSG_EXT_INFO when authenticated
> > 
> > On Mon, 2018-12-10 at 09:10 -0500, Anderson Sasaki wrote:
> > > Hello,
> > > 
> > > Continuing the investigation of the curl issue [1], I found the
> > > actual problem which is a regression introduced by the CVE-2018-
> > > 10933
> > > fix.
> > > The SSH_MSG_EXT_INFO, used in key exchange, is being filtered
> > > when
> > > the user is already authenticated. This breaks the re-keying.
> > > Follows attached a patch to fix this regression. It can be
> > > reviewed
> > > in gitlab [2].
> > 
> > I don't think your patch is right. The SSH_MSG_EXT_INFO is
> > acceptable
> > only during the first key exchange. See the discussion in OpenSSH
> > bug
> > about this + the RFC:
> > 
> > https://bugzilla.mindrot.org/show_bug.cgi?id=2929
> > 
> > >    o  As the next packet following the server's first
> > SSH_MSG_NEWKEYS. [0]
> > 
> > The bug in OpenSSH server was that it send the EXT_INFO when the
> > ext-
> > info-c was sent by the libssh client in the rekey request (also
> > wrongly, but already fixed in [1]).
> > 
> > [0] https://tools.ietf.org/html/rfc8308#section-2.4
> > [1] https://gitlab.com/jjelen/libssh-mirror/commit/83f2ac4a
> 
> Thanks for pointing this.
> 
> Yes, I knew that, and discussed with Andreas about the RFC and that
> OpenSSH is not compliant.
> We agreed that OpenSSH needs a patch, but also that we can allow it
> to not break users who are trying to access servers which are not
> compliant.
> We know that being strict in this caused problems for users, like in
> curl case [0].
> 
> Sorry for not being clear about this in my patch..
> I'm afraid we will have to keep this for a long time, even after
> OpenSSH is fixed, until the fixed version is widely adopted.

OK, thank you for explaing the issue pointing out what exact problem
you had. You are right. My bad that I overlooked that the server-
initialized rekeying resets the session_state (sigh ...) before we got
to compose our kex message. The following patch should do the right
thing in all the cases that I was able to test:

https://gitlab.com/jjelen/libssh-mirror/commit/1356bc6a5

But as I said before, this bug is in libssh, which is in this case
sending the "ext-info-c" appended to the kex list, which "provokes" the
OpenSSH to do the "bad thing" of answering with SSH_EXT_INFO message.
Fixing it with the patch above should solve the issue also without 
relaxing the filter.

On top of this commit, there are more commits implementing the rekeying
based on the recommendation in RFC 4344 and is ready for review, mostly
from the architecture point of view.

Thanks,
-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.