Date: Fri, 13 Nov 2020 17:53:06 +0300 From: Yuri Pankov <ypankov@xsmail.com> To: Greg Veldman <freebsd@gregv.net> Cc: Andrea Venturoli <ml@netfence.it>, freebsd-ports@freebsd.org Subject: Re: Sudden trouble with net/rdesktop Message-ID: <4c0be401-f000-8bf8-605a-a6c6ea8558ed@xsmail.com> In-Reply-To: <20201113144423.GK1045@aurora.gregv.net> References: <1420575d-2622-2063-3a2f-bb0591b9f71c@netfence.it> <0af296a2-2896-87ba-3f8e-de68276a3baf@xsmail.com> <e7783ede-e860-8498-6acc-3f5b15d58e89@xsmail.com> <8d25cba0-63ca-076e-2340-22e31a3015d5@netfence.it> <a8c01ca0-7d4c-7d8a-e07c-b328ac0b9e20@xsmail.com> <20201113144423.GK1045@aurora.gregv.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4c0be401-f000-8bf8-605a-a6c6ea8558ed>