[Tigervnc-devel] potential vulnerability in TLS secType?
Hi, 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. -brian -- 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
Re: [Tigervnc-devel] potential vulnerability in TLS secType?
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
Re: [Tigervnc-devel] potential vulnerability in TLS secType?
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
Re: [Tigervnc-devel] potential vulnerability in TLS secType?
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
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
Re: [Tigervnc-devel] potential vulnerability in TLS secType?
Adam,
Martin's patch seems to work (my suggestion did not).
-brian
On Thu, May 5, 2011 at 5:45 AM, Adam Tkac 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
Re: [Tigervnc-devel] potential vulnerability in TLS secType?
On Thu, May 05, 2011 at 07:01:49AM -0400, Brian Hinz wrote:
> 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;
The readU8 is a flag to signal by the server to the client, that
it can't start the handshake and therefore has to sent the error as plaintext
back
(eg because it can't read the certificate).
Your are proposing a different protocol, which don't allow to return an error
message before the handshake completed.
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
Re: [Tigervnc-devel] potential vulnerability in TLS secType?
Hello Brian,
thanks for your feedback, I committed (r4400 and r4401) Martin's patch
with my modification.
Thanks for your report!
Regards, Adam
On 05/05/2011 01:27 PM, Brian Hinz wrote:
> Adam,
>
> Martin's patch seems to work (my suggestion did not).
>
> -brian
>
> On Thu, May 5, 2011 at 5:45 AM, Adam Tkac 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");
>>>
--
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay
___
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
