@asn-the-goblin-slayer:
"the name_parse() function in libevent's DNS code is vulnerable to a buffer overread.
971 if (cp != name_out) {
972 if (cp + 1 >= end) return -1;
973 *cp++ = '.';
974 }
975 if (cp + label_len >= end) return -1;
976 memcpy(cp, packet + j, label_len);
977 cp += label_len;
978 j += label_len;
No check is made against length before the memcpy occurs.
This was found through the Tor bug bounty program and the discovery should be credited to 'Guido Vranken'."
Reproducer for gdb (https://gist.github.com/azat/e4fcf540e9b89ab86d02):
set $PROT_NONE=0x0
set $PROT_READ=0x1
set $PROT_WRITE=0x2
set $MAP_ANONYMOUS=0x20
set $MAP_SHARED=0x01
set $MAP_FIXED=0x10
set $MAP_32BIT=0x40
start
set $length=202
# overread
set $length=2
# allocate with mmap to have a seg fault on page boundary
set $l=(1<<20)*2
p mmap(0, $l, $PROT_READ|$PROT_WRITE, $MAP_ANONYMOUS|$MAP_SHARED|$MAP_32BIT, -1, 0)
set $packet=(char *)$1+$l-$length
# hack the packet
set $packet[0]=63
set $packet[1]='/'
p malloc(sizeof(int))
set $idx=(int *)$2
set $idx[0]=0
set $name_out_len=202
p malloc($name_out_len)
set $name_out=$3
# have WRITE only mapping to fail on read
set $end=$1+$l
p (void *)mmap($end, 1<<12, $PROT_NONE, $MAP_ANONYMOUS|$MAP_SHARED|$MAP_FIXED|$MAP_32BIT, -1, 0)
set $m=$4
p name_parse($packet, $length, $idx, $name_out, $name_out_len)
x/2s (char *)$name_out
Before this patch:
$ gdb -ex 'source gdb' dns-example
$1 = 1073741824
$2 = (void *) 0x633010
$3 = (void *) 0x633030
$4 = (void *) 0x40200000
Program received signal SIGSEGV, Segmentation fault.
__memcpy_sse2_unaligned () at memcpy-sse2-unaligned.S:33
After this patch:
$ gdb -ex 'source gdb' dns-example
$1 = 1073741824
$2 = (void *) 0x633010
$3 = (void *) 0x633030
$4 = (void *) 0x40200000
$5 = -1
0x633030: "/"
0x633032: ""
(gdb) p $m
$6 = (void *) 0x40200000
(gdb) p $1
$7 = 1073741824
(gdb) p/x $1
$8 = 0x40000000
(gdb) quit
P.S. plus drop one condition duplicate.
Fixes: #317
If you call evdns_base_free() with @fail_requests == 1, then it will defer
callback with DNS_ERR_SHUTDOWN, but that callback (internal) uses
data->evdns_base, but we already freed that evdns base, so we can't do
this, fix this by checking @result to DNS_ERR_SHUTDOWN.
Fixes: regress dns/client_fail_requests_getaddrinfo
Fixes: #269
Otherwise we will trigger next UAF:
$ valgrind --vgdb-error=1 regress --no-fork +dns/client_fail_requests
==24733== Memcheck, a memory error detector
==24733== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==24733== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==24733== Command: regress --no-fork +dns/client_fail_requests
==24733==
==24733==
==24733== TO DEBUG THIS PROCESS USING GDB: start GDB like this
==24733== /path/to/gdb regress
==24733== and then give GDB the following command
==24733== target remote | /usr/lib/valgrind/../../bin/vgdb --pid=24733
==24733== --pid is optional if only one valgrind process is running
==24733==
dns/client_fail_requests: ==24733== Invalid read of size 4
==24733== at 0x4C3352: request_finished (evdns.c:662)
==24733== by 0x4CC8B7: evdns_base_free_and_unlock (evdns.c:4048)
==24733== by 0x4CCAFD: evdns_base_free (evdns.c:4088)
==24733== by 0x458E95: dns_client_fail_requests_test (regress_dns.c:2039)
==24733== by 0x48EA5D: testcase_run_bare_ (tinytest.c:105)
==24733== by 0x48ED3F: testcase_run_one (tinytest.c:252)
==24733== by 0x48F67E: tinytest_main (tinytest.c:434)
==24733== by 0x47C0DA: main (regress_main.c:461)
==24733== Address 0x61e6f70 is 448 bytes inside a block of size 456 free'd
==24733== at 0x4C29EAB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24733== by 0x4A8F4D: event_mm_free_ (event.c:3512)
==24733== by 0x4CC7A1: evdns_nameserver_free (evdns.c:4021)
==24733== by 0x4CC7DC: evdns_base_free_and_unlock (evdns.c:4037)
==24733== by 0x4CCAFD: evdns_base_free (evdns.c:4088)
==24733== by 0x458E95: dns_client_fail_requests_test (regress_dns.c:2039)
==24733== by 0x48EA5D: testcase_run_bare_ (tinytest.c:105)
==24733== by 0x48ED3F: testcase_run_one (tinytest.c:252)
==24733== by 0x48F67E: tinytest_main (tinytest.c:434)
==24733== by 0x47C0DA: main (regress_main.c:461)
==24733== Block was alloc'd at
==24733== at 0x4C28C4F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24733== by 0x4A8D5A: event_mm_malloc_ (event.c:3437)
==24733== by 0x4C8B96: evdns_nameserver_add_impl_ (evdns.c:2505)
==24733== by 0x4C916D: evdns_base_nameserver_ip_add (evdns.c:2629)
==24733== by 0x458DA3: dns_client_fail_requests_test (regress_dns.c:2031)
==24733== by 0x48EA5D: testcase_run_bare_ (tinytest.c:105)
==24733== by 0x48ED3F: testcase_run_one (tinytest.c:252)
==24733== by 0x48F67E: tinytest_main (tinytest.c:434)
==24733== by 0x47C0DA: main (regress_main.c:461)
==24733==
==24733== (action on error) vgdb me ...
Fixes: regress dns/client_fail_requests
Fixes: #269
Interesting that this wasn't found by regression tests since they respond with
that SoME-rAndDom-CaSe domains, and no case-insensitive mode is required during
comparing response from the server and request.
Fixes#288
Covered-by: regress dns/search_lower
In evdns_request_timeout_callback() in case we a giving up, we call
request_finished() which will free() req structure, however we ns from
it to fail it, so save pointer to ns to call nameserver_failed() on
them.
Founded with valgrind:
$ valgrind regress dns/retry
==10497== Memcheck, a memory error detector
==10497== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==10497== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==10497== Command: regress dns/retry
==10497==
dns/retry: [forking] ==10498== Invalid read of size 8
==10498== at 0x4C309D: evdns_request_timeout_callback (evdns.c:2179)
==10498== by 0x49EA95: event_process_active_single_queue (event.c:1576)
==10498== by 0x49EFDD: event_process_active (event.c:1668)
==10498== by 0x49F6DD: event_base_loop (event.c:1891)
==10498== by 0x49F063: event_base_dispatch (event.c:1702)
==10498== by 0x44C7F1: dns_retry_test_impl (regress_dns.c:724)
==10498== by 0x44CF60: dns_retry_test (regress_dns.c:749)
==10498== by 0x48A8A1: testcase_run_bare_ (tinytest.c:105)
==10498== by 0x48A94E: testcase_run_forked_ (tinytest.c:189)
==10498== by 0x48AB73: testcase_run_one (tinytest.c:247)
==10498== by 0x48B4C2: tinytest_main (tinytest.c:434)
==10498== by 0x477FC7: main (regress_main.c:459)
==10498== Address 0x6176ef8 is 40 bytes inside a block of size 342 free'd
==10498== at 0x4C29E90: free (vg_replace_malloc.c:473)
==10498== by 0x4A4411: event_mm_free_ (event.c:3443)
==10498== by 0x4BE8C5: request_finished (evdns.c:702)
==10498== by 0x4C3098: evdns_request_timeout_callback (evdns.c:2178)
==10498== by 0x49EA95: event_process_active_single_queue (event.c:1576)
==10498== by 0x49EFDD: event_process_active (event.c:1668)
==10498== by 0x49F6DD: event_base_loop (event.c:1891)
==10498== by 0x49F063: event_base_dispatch (event.c:1702)
==10498== by 0x44C7F1: dns_retry_test_impl (regress_dns.c:724)
==10498== by 0x44CF60: dns_retry_test (regress_dns.c:749)
==10498== by 0x48A8A1: testcase_run_bare_ (tinytest.c:105)
==10498== by 0x48A94E: testcase_run_forked_ (tinytest.c:189)
==10498==
==10498==
==10498== HEAP SUMMARY:
==10498== in use at exit: 0 bytes in 0 blocks
==10498== total heap usage: 83 allocs, 83 frees, 10,020 bytes allocated
==10498==
==10498== All heap blocks were freed -- no leaks are possible
==10498==
==10498== For counts of detected and suppressed errors, rerun with: -v
==10498== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
OK
1 tests ok. (0 skipped)
==10497==
==10497== HEAP SUMMARY:
==10497== in use at exit: 0 bytes in 0 blocks
==10497== total heap usage: 3 allocs, 3 frees, 96 bytes allocated
==10497==
==10497== All heap blocks were freed -- no leaks are possible
==10497==
==10497== For counts of detected and suppressed errors, rerun with: -v
==10497== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Bug was introduced in 97c750d6602517f22a1100f16592b421c38f2a45 ("evdns:
fail ns after we are failing/retrasmitting request").
In case we are failing request (evdns_request_timeout_callback()), we
delete timeout_event in request_finished(), while just before calling
request_finished() (for failing request) there was a call to
nameserver_failed(), that add event for timeout_event, IOW we must fail
ns after request because otherwise we will not have timeout_event
actived, and we will waiting forever.
Before this patch the dns/retry_disable_when_inactive will wait forever,
after - OK.
When user install EVDNS_BASE_DISABLE_WHEN_INACTIVE flag for evdns base,
we must remove the timer that is used for probing, if current dns server
failed, otherwise it won't break the loop.
Since evutil_snprintf() (actually evutil_vsnprintf() called by it) will
make sure the buffer is null-terminated by placing a null byte at
len_out - 1, we need to pass the full length of the buffer; otherwise
the path will end in "\\host" instead of "\\hosts".
Hosts files are not loaded in evdns_base_config_windows_nameservers() if
load_nameservers_with_getnetworkparams() succeeds on Windows. Parse and
load it first before setting up nameservers.
"flush" can imply writing something out to a file or connection before
clearing it; "clear" always means "remove". It's also potentially
misleading to say "outdated" here, since the function removes _all_
addresses regardless, not just certain outdated ones.
Also, don't free the lock in this function. Also reindent the function.
As mentioned at https://sourceforge.net/p/levent/bugs/293/
created a small function "evdns_base_flush_outdated_host_addresses" which removes all the previous requests of hosts , if user wants to clean up the list of hosts can call and use this function.
Requires function declaration to be added in include/event2/dns.h
Adding it in another patch for the same bug.
If there is no nameservers installed, using
evdns_base_nameserver_ip_add(), than evdns_base_resume() will SEGFAULT,
because of NULL dereference in evdns_requests_pump_waiting_queue()
Conflicts:
evdns.c
If there is no nameservers installed, using
evdns_base_nameserver_ip_add(), than evdns_base_resume() will SEGFAULT,
because of NULL dereference in evdns_requests_pump_waiting_queue()
Here is the brief description of problem:
When you are use evdns to resolve domains to IP adresses (see
./sample/dns-example) you loop never returns from event_base_dispatch(),
and because of this the program will never terminated.
Because existing programs may be depending on the old behavior, we
only apply the fix when evdns_base_new() is created with a new flag -
EVDNS_BASE_DISABLE_WHEN_INACTIVE.
(Commit message edited by Nick while squashing the branch.)
If an evdns_getaddrinfo timeout happens while pending_cb is set, and
a callback is about to run, but we get a call to
evdns_getaddrinfo_gotresolve before it finishes.
Github issue #60. Thanks to Greg Hazel for patch and patience.
Back when deferred_cb stuff had its own queue, the queue was always
executed, but we never ran more than 16 callbacks per iteration.
That made for two problems:
1: Because deferred_cb stuff would always run, and had no priority,
it could cause priority inversion.
2: It doesn't respect the max_dispatch_interval code.
Then, when I refactored deferred_cb to be a special case of
event_callback, that solved the above issues, but made for two more
issues:
3: Because deferred_cb stuff would always get the default priority,
it could could low-priority bufferevents to get too much priority.
4: With code like bufferevent_pair, it's easy to get into a
situation where two deferreds keep adding one another, preventing
the event loop from ever actually scanning for more events.
This commit fixes the above by giving deferreds a better notion of
priorities, and by limiting the number of deferreds that can be
added to the _current_ loop iteration's active queues. (Extra
deferreds are put into the active_later state.)
That isn't an all-purpose priority inversion solution, of course: for
that, you may need to mess around with max_dispatch_interval.
Otherwise, requests initially sent to a failing nameserver would
stay there indefinitely, even if other nameservers would work.
Fix for sourceforge bug 3518439
