dnsserv.c asserts when no supported questions are requested

The patch for #10268 (moved) has a simple crasher that is easily exploited from the network if DNSPort is open to a LAN (e.g., if you are transparent proxying).

As [ticket:10268 #comment:11 andrea] hinted at, the added "if (!q) q = req->questions[i];" to the for loop ensures that "q" is always set to the first question, even if it's unsupported. In which case, the "if (!q)" check for NOTIMPL is dead code. Ultimately, you will eventually hit the "tor_assert" that was added to the "else" branch. Additionally, the "switch" block switches on "req->questions[i]->type", but the assignment to "supported_q" is "q" (which is always the first question) instead of "req->questions[i]", so it doesn't actually pick the first supported question -- it always picks the first question.

Trac:
Username: geekmug

changed milestone to %Tor: 0.2.5.x-final

added 027-backport actualpoints::small component::core tor/tor dns dnsport milestone::Tor: 0.2.5.x-final points::very small priority::medium reporter::geekmug resolution::fixed reviewer::nickm severity::major status::closed tor-client type::defect version::tor 0.2.7.6 labels

Trac:
Username: geekmug

dnsserv.patch

Trac:
Username: geekmug

Trac:
Milestone: N/A to Tor: 0.2.8.x-final
Keywords: dns dnsport tor-client deleted, dns dnsport tor-client 027-backport added

Trac:
Keywords: dns dnsport tor-client 027-backport deleted, dns dnsport tor-client 027-backport TorCoreTeam201604 added
Reviewer: N/A to nickm

Trac:
Status: new to needs_review

It looks like qtype_all is broken by this patch, assuming that qtype_all worked?

It looks like this code, currently in dnsserv.c, prevents us from reaching the assert?

  if (err != DNS_ERR_NONE || !supported_q) {
    /* We got an error?  There's no question we're willing to answer? Then
     * send back an answer immediately; we're done. */
    evdns_server_request_respond(req, err);
    return;
  }

The code as it stands does look wrong, though. And it looks like qtype_all would also have hit this test.

I've tested Tor 0.2.5 and forward with MX queries to verify that they don't actually crash. It appears they don't. I'm testing by only sending a single MX query in the request.

(Please correct me if I'm wrong!)

Assuming that there isn't really a crash bug, this is just ugly code, nothing more. So it can be a cleanup fix for 0.2.9.

Trac:
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.???
Points: N/A to very small
Priority: Very High to Medium
Status: needs_review to needs_revision
Keywords: dns dnsport tor-client 027-backport TorCoreTeam201604 deleted, dns dnsport tor-client 029-proposed added

Replying to nickm:

I've tested Tor 0.2.5 and forward with MX queries to verify that they don't actually crash. It appears they don't. I'm testing by only sending a single MX query in the request.

I'm sorry, I must have done a poor job of explaining the bug. Your test is invalid because the crash is only presented with multiple queries wherein the first query is a bad type and contains at least one good type. The key part of my commit message is: "it doesn't actually pick the first supported question -- it always picks the first question." All you need is a query with an unsupported type for the first query and at least one supported query.

For instance, if I send a SRV and A record query together (in that order), then the "if (!q) q = req->questions[i];" makes q a SRV record. Then, the switch block falls through to the default. On the next loop, q is already set, so it is still the SRV record, but the switch is on "req->questions[i]->type" that is now an A record. The case block then assigns "supported_q = q" (which is not req->questions[i]).

I stumbled on this by putting the DNSPort as 5353, but then mDNS was hitting the tor server. For instance, one of my devices sends out a request for [SRV, A, SRV], which matches the criteria I describe above, and crashes tor.

(gdb) frame 1 #1 0x00000000004cef69 in evdns_server_callback (req=0xeab638, data_=0x7ff550) at src/or/dnsserv.c:139 139 tor_assert(q->type == EVDNS_TYPE_PTR); (gdb) print *q $2 = {type = 33, class = 1, name = "A"}

I can capture a packet for you to reply or perhaps generate some code to cause the issue, but I don't readily know how to generate a DNS query with multiple records using commonly available tools (e.g., "dig" doesn't support it).

Trac:
Username: geekmug

Ah; thanks. That makes more sense!

Trac:
Status: needs_revision to needs_review
Keywords: dns dnsport tor-client 029-proposed deleted, dns dnsport tor-client must-fix-before-028-rc added
Milestone: Tor: 0.2.??? to Tor: 0.2.8.x-final
Severity: Normal to Major

Calling all non-needs_information tickets for May.

Trac:
Keywords: dns dnsport tor-client must-fix-before-028-rc deleted, must-fix-before-028-rc, tor-client, TorCoreTeam201605, dns, dnsport added

okay, the analysis does look right. Merging your fix to 0.2.8 and forward; marking this ticket for possible backport. (If we backport, I've got the fix with a changes file in my branch bug18710_025 .)

Trac:
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.7.x-final
Actualpoints: N/A to small
Keywords: tor-client, TorCoreTeam201605, must-fix-before-028-rc deleted, tor-client 027-backport added

Available for backport to 0.2.5...

Trac:
Status: needs_review to merge_ready
Milestone: Tor: 0.2.7.x-final to Tor: 0.2.5.x-final

backported.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #20512 (moved)

mentioned in issue #20512 (moved)

moved to tpo/core/tor#18710 (closed)