FreeRDP/FreeRDP — pull request #1257 by simon-engledew (2013-05-22 08:14:06+00:00)
Title: Ber Encoding Issue

We were experiencing a really hard to track down issue in FreeRDP that seemed to occur when the Username + Domain + Password hit around the 127 character mark. NLA authentication would appear to succeed, but the next SSL_Read would fail and the server would report a protocol error:

SSL_read: Failure in SSL library (protocol error?)
SSL_read: error:14094438:SSL routines:SSL3_READ_BYTES:tlsv1 alert internal error

I span up FreeRDP in valgrind and it revealed the following issue:

==16046== Invalid write of size 1
==16046== at 0x402689F: memcpy (mc_replace_strmem.c:497)
==16046== by 0x44291FC: ber_write_octet_string (ber.c:308)
==16046== by 0x43D0078: credssp_write_ts_password_creds (nla.c:864)
==16046== by 0x43D02B0: credssp_write_ts_credentials (nla.c:932)
==16046== by 0x43D0315: credssp_encode_ts_credentials (nla.c:949)
==16046== by 0x43D034C: credssp_encrypt_ts_credentials (nla.c:960)
==16046== by 0x43CF0E4: credssp_client_authenticate (nla.c:350)
==16046== by 0x43CF88D: credssp_authenticate (nla.c:630)
==16046== by 0x43FD750: transport_connect_nla (transport.c:157)
==16046== by 0x43D150A: nego_security_connect (nego.c:180)
==16046== by 0x43D1434: nego_connect (nego.c:159)
==16046== by 0x43F50EA: rdp_client_connect (connection.c:141)
==16046== Address 0x6bf5868 is 1 bytes after a block of size 143 alloc'd
==16046== at 0x4024F20: malloc (vg_replace_malloc.c:236)
==16046== by 0x4481BED: sspi_SecBufferAlloc (sspi.c:213)
==16046== by 0x43D02EB: credssp_encode_ts_credentials (nla.c:946)
==16046== by 0x43D034C: credssp_encrypt_ts_credentials (nla.c:960)
==16046== by 0x43CF0E4: credssp_client_authenticate (nla.c:350)
==16046== by 0x43CF88D: credssp_authenticate (nla.c:630)
==16046== by 0x43FD750: transport_connect_nla (transport.c:157)
==16046== by 0x43D150A: nego_security_connect (nego.c:180)
==16046== by 0x43D1434: nego_connect (nego.c:159)
==16046== by 0x43F50EA: rdp_client_connect (connection.c:141)
==16046== by 0x43E8303: freerdp_connect (freerdp.c:86)
==16046== by 0x6F160FD: lxrdp_connect (xrdp-freerdp.c:88)

ber_write_octet_string was attempting to write into a buffer which was not quite big enough.

I tracked the issue down to the BER Encoding, which does not behave correctly around several of the boundary conditions. After pouring over the spec for a while I fixed the way length is encoded and this revealed a second issue: that the ber_skip and ber_write methods would often return contradicting sizes, and that the get_content_length method would not behave deterministically for lengths around 127.

Now (in nla.c) instead of calculating the overall length for the buffer size and then subtracting the width of the header and length components to use for the inner sequence, this patch does the reverse - measuring the inner packet size when creating a sequence and then adding the necessary length for the sequence encoding when actually creating a buffer to store the result. This behaves itself and correctly creates the right buffer size for each packet although it is marginally less efficient. Given more time I could probably come up with something a lot sexier that removes the need to measure and then write as separate passes (this would be the preferred architecture) but sadly I am doing this at work and I have a million and one other things to do that are all top priority. This fix seems to work, even if it is not optimal. Hopefully you might see a follow up pull request if I get some spare time. ;)

As an aside, I have also changed the way integers are written in ber.c. The specification says that they should be two's complement but there is no two's complement handling in the code. Ideally this should be expanded to handle non-uints but for now I just ensure that the most significant bit is never set to 1 and the number is always a positive representation. This didn't seem to affect the protocol negotiation either way however I include this for completeness.