Opened 6 years ago

Closed 5 years ago

#10268 closed enhancement (implemented)

tor's dnsserv returns NOTIMPL for queries of type ALL and MX

Reported by: epoch Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-client, dns, dnsport, 025-triaged, andrea-review-0254
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

While trying to get qmail working over tor I found that it will work if ALL and MX records don't return NOTIMPL. In the attached patch I have ALL records doing whatever A and AAAA records do and MX just returning NXDOMAIN. (Which should be fine since MX records aren't required and MTAs should fall back to A records if they don't exist, right?)

Child Tickets

Attachments (1)

tor-dnsserv.patch (1.7 KB) - added by epoch 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by epoch

Hopefully this patch won't affect what was wanted with https://trac.torproject.org/projects/tor/ticket/3369

comment:2 Changed 6 years ago by epoch

Milestone: Tor: unspecified
Type: defectenhancement

comment:3 Changed 6 years ago by nickm

Keywords: tor-client dns dnsport added
Milestone: Tor: 0.2.5.x-final
Status: newneeds_review

It does seem to conflict with #3369, at least as I read it, but I might misunderstand.

The real long-term solution here would be to implement proposal 219 so that we can support more DNS types, but that's obviously not going to be simple. If this gets qmail working, we should maybe see whether it's workable for as a stopgap option or something.

comment:4 in reply to:  description Changed 6 years ago by epoch

Replying to epoch:

While trying to get qmail working over tor I found that it will work if ALL and MX records don't return NOTIMPL. In the attached patch I have ALL records doing whatever A and AAAA records do and MX just returning NXDOMAIN. (Which should be fine since MX records aren't required and MTAs should fall back to A records if they don't exist, right?)

Didn't know that descriptions weren't changeable... Right now the patch is returning NO ERROR for queries for MX records with 0 answers. Which is what seems to be correct according to what bind does when a domain doesn't have an MX record and should satisfy the process in http://tools.ietf.org/html/rfc5321#section-5.1 describing how mailers should figure out where to send mail which says NX returned for an MX lookup is supposed to be reported as an error.

comment:5 Changed 6 years ago by epoch

Patch has been tested with postfix too. Can send and receive mail to and from hidden services.

Changed 6 years ago by epoch

Attachment: tor-dnsserv.patch added

comment:6 Changed 5 years ago by nickm

This looks plausible to me... but maybe it should return nothing for all other types, not just for MX?

comment:7 Changed 5 years ago by nickm

Keywords: 025-triaged added

comment:8 Changed 5 years ago by nickm

(We chatted about this a little on IRC; it sounds plausible. Maybe it should be controlled by an option. Marked as 025-triaged)

comment:9 Changed 5 years ago by nickm

I tweaked it as a new branch "bug10268" in my public repository. It now takes _all_ previously disallowed qtypes and responds by sending an empty reply. In theory. I tested it a little and it doesn't seem to have anything wrong with it, though more attention would be good.

comment:10 Changed 5 years ago by nickm

Keywords: andrea-review-0254 added

Drop owners from needs_review tickets in tor 0.2.5

comment:11 Changed 5 years ago by andrea

Review:

  • The if (!q) case to send a NOTIMPL can only happen if req->nquestions == 0 or if all of the questions either have req->questions[i] == NULL or req->questions[i]->dns_question_class != EVDNS_CLASS_INET. I suppose the last case is the one that can actually happen if a client still cares about CHAOS or HESIOD for some reason, and so the if (!q) { NOTIMPL } bit isn't dead code?
  • Aside from the above inquiry, this all looks okay to me in lieu of actually doing proposal 219 properly.

comment:12 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

wrt that question: I believe that's right. I've gotten zero requests for CHAOS or HESIOD stuff, thank goodness.

Merging to 0.2.5 then.

Note: See TracTickets for help on using tickets.