Re: Libidn serious bug on Windows x64

[Evgeny Grin]

On 21.07.2016 23:25, Evgeny Grin wrote:
> On 20.07.2016 20:03, Simon Josefsson wrote:
>> Evgeny Grin <address@hidden> writes:
>>
>>> Hi!
>>>
>>> While debugging cURL on Windows x64 crash with simple run of "curl -v
>>> http://яндекс.рф";, I discovered that crash caused by calling
>>> tld_check_lz(). Further investigations reveal pointer to size_t is cast
>>> to pointer to long in stringprep_utf8_to_ucs4() which in invalid on
>>> Win64. As result - very high number is stored in items_written and in
>>> tld_get_4() dereferenced an invalid pointer.
>>> I can't make a minimal example to illustrate it as in minimal example
>>> initial nullifying hides the problem. Hope that invalid casting is
>>> obvious problem.
>>
>> Hello.  Does this problem still exist with 1.32?  What size does
>> 'size_t' and 'long' have on Windows x64?
> 
> Yes, the same bug is present in 1.32, 1.33 and in current git master.
> Currently libidn keep crashing on Windows.
> MinGW package includes my patches to prevent crashing:
> https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-libidn/0003-nfkc.c-Fix-Win64-crash.patch
> https://github.com/Alexpux/MINGW-packages/blob/master/mingw-w64-libidn/0004-nfkc.c-Fixed-invalid-var-types.patch
> 
> Generally, you can't assume that sizeof(int) == sizeof(long) or
> sizeof(long) == sizeof(long long) or sizeof(long) == sizeof(void*)
> https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
> 
> On Windows x64 sizeof(int) == 4, sizeof(long) == 4, sizeof(long long) ==
> 8, sizeof(void*) == 8.
> 
> But casting pointer to one type to pointer to other type is bad idea in
> general, unless you 100% sure that such casting is ALWAYS and ANYWHERE
> valid.
> I sent already some patches for fixing this bug.
> http://lists.gnu.org/archive/html/help-libidn/2016-04/msg00002.html
> 
> Additional (unrelated) fixes attached.


Main fix for crash is here:
http://lists.gnu.org/archive/html/help-libidn/2016-04/msg00010.html

Crash path:
Client calls tld_check_lz()
tld_check_lz() calls tld_check_8z()
tld_check_8z() calls stringprep_utf8_to_ucs4() with last parameter
pointer to size_t type _uninitialized_ variable 'ilen'.
stringprep_utf8_to_ucs4() calls g_utf8_to_ucs4_fast() with casting
pointer to size_t (8 bytes variable) to pointer to long (4 bytes variable)
g_utf8_to_ucs4_fast() writes only 4 bytes to 'items_written' (leaving
another 4 bytes uninitialized).
stringprep_utf8_to_ucs4() returns to tld_check_8z(): 'ilen' contains
lower 4 bytes with valid data and high 4 bytes with random data.
tld_check_8z() calls tld_check_4() with `ilen' specifies several
gigabytes of data to be processed.
Read memory outside the buffer.


And I'd like to point attention to other bug:
Currently only tld_check_4() and tld_check_4z() return valid position in
'errpos'.
tld_check_8z(), tld_check_lz() return same number as tld_check_4(), but
this position is incorrect for UTF-8 and for local encoding.

--
Best Wishes,
Evgeny Grin

signature.asc
Description: OpenPGP digital signature