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
