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");
>
Index: common/rfb/CSecurityTLS.cxx
===================================================================
--- common/rfb/CSecurityTLS.cxx (revision 4399)
+++ common/rfb/CSecurityTLS.cxx (working copy)
@@ -171,8 +171,15 @@
if (!is->checkNoWait(1))
return false;
- if (is->readU8() == 0)
- return true;
+ if (is->readU8() == 0) {
+ rdr::U32 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
