Opened 4 years ago

Closed 3 years ago

#18710 closed defect (fixed)

dnsserv.c asserts when no supported questions are requested

Reported by: geekmug Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.6
Severity: Major Keywords: dns, dnsport, tor-client 027-backport
Cc: Actual Points: small
Parent ID: Points: very small
Reviewer: nickm Sponsor:

Description

The patch for #10268 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 #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.

Child Tickets

Attachments (1)

dnsserv.patch (1.2 KB) - added by geekmug 4 years ago.

Download all attachments as: .zip

Change History (13)

Changed 4 years ago by geekmug

Attachment: dnsserv.patch added

comment:1 Changed 4 years ago by nickm

Keywords: 027-backport added
Milestone: Tor: 0.2.8.x-final

comment:2 Changed 4 years ago by nickm

Keywords: TorCoreTeam201604 added
Reviewer: nickm

comment:3 Changed 4 years ago by nickm

Status: newneeds_review

comment:4 Changed 4 years ago by nickm

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

comment:5 Changed 4 years ago by nickm

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.

comment:6 Changed 4 years ago by nickm

Keywords: 029-proposed added; 027-backport TorCoreTeam201604 removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.???
Points: very small
Priority: Very HighMedium
Status: needs_reviewneeds_revision

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.

comment:7 in reply to:  6 Changed 4 years ago by geekmug

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).

comment:8 Changed 4 years ago by nickm

Keywords: must-fix-before-028-rc added; 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Severity: NormalMajor
Status: needs_revisionneeds_review

Ah; thanks. That makes more sense!

comment:9 Changed 4 years ago by nickm

Keywords: TorCoreTeam201605 added

Calling all non-needs_information tickets for May.

comment:10 Changed 4 years ago by nickm

Actual Points: small
Keywords: 027-backport added; must-fix-before-028-rc TorCoreTeam201605 removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.7.x-final

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 .)

comment:11 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.5.x-final
Status: needs_reviewmerge_ready

Available for backport to 0.2.5...

comment:12 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

backported.

Note: See TracTickets for help on using tickets.