Wouldn't this (also untested) work as well, and have the advantage of
relying on gnutls to verify that the handshake was completed?
diff -Nr -C 6 rfb.unix/CSecurityTLS.cxx.bak rfb.unix/CSecurityTLS.cxx
*** rfb.unix/CSecurityTLS.cxx.bak 2011-05-05 06:54:11.018963720 -0400
--- rfb.unix/CSecurityTLS.cxx 2011-05-05 06:55:24.826533250 -0400
***************
*** 168,182 ****
initGlobal();
if (!session) {
if (!is->checkNoWait(1))
return false;
- if (is->readU8() == 0)
- return true;
-
if (gnutls_init(&session, GNUTLS_CLIENT) != GNUTLS_E_SUCCESS)
throw AuthFailureException("gnutls_init failed");
if (gnutls_set_default_priority(session) != GNUTLS_E_SUCCESS)
throw AuthFailureException("gnutls_set_default_priority failed");
--- 168,179 ----
***************
*** 197,208 ****
--- 194,208 ----
if (err != GNUTLS_E_SUCCESS) {
vlog.error("TLS Handshake failed: %s\n", gnutls_strerror (err));
shutdown(false);
throw AuthFailureException("TLS Handshake failed");
}
+ if (is->readU8() == 0)
+ return true;
+
checkSession();
cc->setStreams(fis = new rdr::TLSInStream(is, session),
fos = new rdr::TLSOutStream(os, session));
return true;
On Thu, May 5, 2011 at 2:43 AM, Martin Koegler
<mkoeg...@auto.tuwien.ac.at>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");
>
> Regards,
> Martin Kögler
>
>
>
> ------------------------------------------------------------------------------
> 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
>
>
------------------------------------------------------------------------------
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
