Sudden trouble with net/rdesktop

[Yuri Pankov]

Greg Veldman wrote:
> On Fri, Nov 13, 2020 at 03:52:54PM +0300, Yuri Pankov wrote:
>> OK, I was able to reproduce this; actually, it hit that assertion for
>> all Windows 10 20H2 installs I have, all are updated to latest patches,
>> so I can't tell when this started to happen.  Adding a debug print
>> before that assert() shows the following:
>>
>> $ DISPLAY=mercury:0.0
>> /usr/ports/net/rdesktop/work/rdesktop-1.9.0/rdesktop orion
>> len=128 size=128
>> Assertion failed: ((len * 2) < size), function _utils_data_to_hex, file
>> utils.c, line 501.
>>
>> So we need the len of 128 we need size of digest buf to be > 256, the
>> following patch worked for me:
>>
>> polaris:ypankov:/usr/ports/net/rdesktop$ cat files/patch-utils.c
>> --- utils.c.orig        2020-11-13 12:40:51 UTC
>> +++ utils.c
>> @@ -584,7 +584,7 @@ _utils_cert_get_info(gnutls_x509_crt_t cert, char *out
>>    {
>>           char buf[128];
>>           size_t buf_size;
>> -       char digest[128];
>> +       char digest[512];
>>           gnutls_x509_dn_t dn;
>>           time_t expire_ts, activated_ts;
> 
> This seems like a reasonable change, but I'll make one minor
> note.  Later on in _utils_cert_get_info() the contents of
> digest are written to sha1 and sha256, which are both size
> 256.  In the initial implementation both buf and digest are
> of the same size, which would seem to indicate that buf was
> not expected to be completely full (in fact less than half
> full based on the assertion in _utils_data_to_hex()).  However
> if defined to 512 chars and ever somehow filled beyond 256
> (in fact a bit less than that, since a static string is
> prepended to the call to snprintf()), the writes to sha1 and
> sha256 would be incomplete, which I haven't completely read
> through but would very likely lead to Bad Things(tm) later on.
> 
> All this to say, perhaps a more conservative approach would be
> to increase digest to 256 chars instead of 512.  I don't currently
> have a Windows 10 20H2 machine to test on, but would you mind
> trying with 256 instead of 512 and see if that also fixes the
> issue?  If so, let me know and I'll submit a PR to get this
> patch added.

size is 128 now, so with digest of 256 we fail the ((len * 2) <  size) 
assertion, 257 works though (or changing the assertion to be <=).

Actually, this does not look like a real fix.  I have looked more into 
it and something is still very wrong -- _utils_cert_get_info() calls 
gnutls_x509_crt_get_fingerprint() and does NOT check the return value 
while it is -72 (GNUTLS_E_ASN1_VALUE_NOT_VALID), while the only 
*documented* return values are 0 (got fingerprint successfully) and -51 
(GNUTLS_E_SHORT_MEMORY_BUFFER, not enough buffer space).  I don't know 
if it was the same previously as I don't have a system to test against. 
  If yes, then this patch could be used in ports at least until upstream 
fixes the problem properly; if no, then we would need a proper fix first.

Also note that sha1 and sha256 fingerprints reported are the *same*, 
which doesn't look right with this approach.