Re: [Tigervnc-devel] potential vulnerability in TLS secType?

2011-05-10 Thread Adam Tkac
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 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);



--
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


Re: [Tigervnc-devel] potential vulnerability in TLS secType?

2011-05-05 Thread Adam Tkac
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?

2011-05-05 Thread Brian Hinz
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.atwrote:

 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?

2011-05-05 Thread Brian Hinz
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


Re: [Tigervnc-devel] potential vulnerability in TLS secType?

2011-05-05 Thread Martin Koegler
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