Adam,
Martin's patch seems to work (my suggestion did not).
-brian
On Thu, May 5, 2011 at 5:45 AM, Adam Tkac <at...@redhat.com> wrote:
> Hello Brian,
>
> thanks for notification about this issue, you are right that password
> might been sent over network without proper validation of the X.509 certs.
>
> Can you please test if attached patch solves this issue? It is Martin's
> patch with little modification (result is rdr::U32 instead of int).
>
> Regards, Adam
>
> On 05/05/2011 08:43 AM, Martin Koegler wrote:
> > On Wed, May 04, 2011 at 10:51:06PM -0400, Brian Hinz wrote:
> >> I think that I just stumbled onto a possible security vulnerability in
> >> CSecurityTLS. It seems as though CSecurityTLS::processMsg returns true
> >> before the handshake has completed (possibly due to the "if (is.readU8()
> ==
> >> 0)" test on line 174). In the case of secTypes like x509plain, the user
> is
> >> prompted for a username and password (meaning the client is processing
> phase
> >> 2 of the security stack) before the certificate has been verified. I
> >> noticed this while testing a known bad certificate - presumably this
> means
> >> that the username & password are sent in the clear since the TLS
> handshake
> >> never completes.
> > Thanks for spotting this. The server should send 0, if he can't start the
> handshake - so it
> > is still possible to sent the error messages back to the client.
> >
> > The problem is, that a malicus server could send 0 and then
> authentification
> > succeded. It is caused by the fact, the I reused the common code for
> returing the
> > error message.
> >
> > A solution can be, that the error fetching code from
> CConnection::processSecurityResultMsg
> > is inlined in CSecurityTLS::processMsg, like the following (untested):
> >
> > diff --git a/common/rfb/CSecurityTLS.cxx b/common/rfb/CSecurityTLS.cxx
> > index 6028792..17ed93c 100644
> > --- a/common/rfb/CSecurityTLS.cxx
> > +++ b/common/rfb/CSecurityTLS.cxx
> > @@ -171,8 +171,15 @@ bool CSecurityTLS::processMsg(CConnection* cc)
> > if (!is->checkNoWait(1))
> > return false;
> >
> > - if (is->readU8() == 0)
> > - return true;
> > + if (is->readU8() == 0) {
> > + int result = is->readU32();
> > + CharArray reason;
> > + if (result == secResultFailed || result == secResultTooMany)
> > + reason.buf = is->readString();
> > + else
> > + reason.buf = strDup("Authentication failure (protocol error)");
> > + throw AuthFailureException(reason.buf);
> > + }
> >
> > if (gnutls_init(&session, GNUTLS_CLIENT) != GNUTLS_E_SUCCESS)
> > throw AuthFailureException("gnutls_init failed");
> >
>
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd
_______________________________________________
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
