Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refuse to resolve the .onion TLD. #10705

Closed
wants to merge 1 commit into from
Closed

Refuse to resolve the .onion TLD. #10705

wants to merge 1 commit into from

Conversation

Kangie
Copy link
Contributor

@Kangie Kangie commented Mar 7, 2023

RFC 7686 states that:

Applications that do not implement the Tor
protocol SHOULD generate an error upon the use of .onion and
SHOULD NOT perform a DNS lookup.

Let's do that.

See #543
https://www.rfc-editor.org/rfc/rfc7686#section-2


I'm certain that this will inconvenience some people; I feel that any inconvenience is far outweighed by the benefits for those using (or trying to use) Tor.

I initially made this change into a default-on feature but decided against it. We're not Tor-aware, we should just refuse to resolve the .onion TLD.

If there's interest in making this a feature I'm willing to go back and do that so that anyone with a valid use case will be able to disable RFC 7686 protections while those that need to be sure that we won't accidentally leak information can clearly see that their binary has this 'feature'.

@Kangie
Copy link
Contributor Author

Kangie commented Mar 7, 2023

I'll fix up that CI after some sleep. 😄

lib/hostip.c Outdated Show resolved Hide resolved
@bagder bagder added the name lookup DNS and related tech label Mar 7, 2023
@danielgustafsson
Copy link
Member

I haven't had time to read this one yet, but we've been down this rabbithole before so it's worth making sure we're not repeating issues discovered in #5159

@Kangie Kangie force-pushed the rfc7686 branch 2 times, most recently from 55e60e6 to 8b20956 Compare March 7, 2023 17:11
@Kangie
Copy link
Contributor Author

Kangie commented Mar 7, 2023

The following is still technically an issue here:

The main problem with this approach is that asking for a host foo.onion doesn't necessary equal a DNS resolve of that name, but could be a local host name that gets the domain appended in the resolve so it would ask for foo.onion.domain.tld

While that's strictly correct, how many people are actually using two subdomains and relying on DNS search suffixes to fill in the rest? 😛 On a more serious note, to actually be RFC compliant we should never request a DNS lookup for a request ending in .onion; it's a bit of an edge case but I'd make the argument that it's irrelevant in the real world.

A bug report from a user who reported that they could sometimes make curl resolve .onion TLDs in their somewhat kluged solution brought the original note in the KNOWN_BUGS and its associated issue to my attention. I figure that it's safer for everyone if curl isn't accidentally leaking .onion requests until someone decides to implement a proper Tor backend.

Edit: Can't see any other obvious issues when compared to the other PR; this is a far simpler implementation. I'm a terrible C (and particularly C89) coder though, so any feedback is welcome.

Edit the second: Bedtime thought, .onion. is a case that I suspect should to be handled as well, thoughts?

@Kangie
Copy link
Contributor Author

Kangie commented Mar 7, 2023

Results of my investigation from Gentoo Bugzilla:

... the adns/c-ares backend was updated at some point to be RFC 7686-compliant. A quick google confirms this happened in 2018:

c-ares/c-ares#196

As this is the default in Gentoo, curl/libcurl will send a request to c-ares and get back "NXDOMAIN" as per the RFC:

Name Resolution APIs and Libraries (...) MUST either respond to requests for
.onion names by resolving them according to [tor-rendezvous] or by responding
with NXDOMAIN.

When you disable that, (as you have here) curl simply sends the request directly using system DNS.

@bagder
Copy link
Member

bagder commented Mar 7, 2023

Bedtime thought, .onion. is a case that I suspect should to be handled as well, thoughts?

Yes!

@Kangie Kangie force-pushed the rfc7686 branch 6 times, most recently from cf2600d to 9522cbd Compare March 8, 2023 05:10
@Kangie
Copy link
Contributor Author

Kangie commented Mar 8, 2023

If I understand test syntax correctly this is looking pretty good now. I'm not sure about checking the strdup for an OOM; we're at about the edge of my C knowledge at this point 😄

Happy to address any feedback you may have!

lib/hostip.c Outdated Show resolved Hide resolved
lib/hostip.c Outdated Show resolved Hide resolved
lib/hostip.c Outdated Show resolved Hide resolved
lib/hostip.c Outdated Show resolved Hide resolved
lib/hostip.c Outdated Show resolved Hide resolved
@bagder bagder added the feature-window A merge of this requires an open feature window label Mar 8, 2023
@Kangie
Copy link
Contributor Author

Kangie commented Mar 9, 2023

I'll see what I can do about that over the weekend!

RFC 7686 states that:

> Applications that do not implement the Tor
> protocol SHOULD generate an error upon the use of .onion and
> SHOULD NOT perform a DNS lookup.

Let's do that.

See curl#543
https://www.rfc-editor.org/rfc/rfc7686#section-2
@bagder bagder removed the feature-window A merge of this requires an open feature window label Mar 30, 2023
@bagder bagder closed this in 0ae0abb Mar 30, 2023
@bagder
Copy link
Member

bagder commented Mar 30, 2023

Thanks!

@Kangie Kangie deleted the rfc7686 branch March 30, 2023 23:47
dfandrich added a commit that referenced this pull request Mar 31, 2023
curl bails out early with a different error message if http support is
compiled out.

Ref: #10705
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
RFC 7686 states that:

> Applications that do not implement the Tor
> protocol SHOULD generate an error upon the use of .onion and
> SHOULD NOT perform a DNS lookup.

Let's do that.

https://www.rfc-editor.org/rfc/rfc7686#section-2

Add test 1471 and 1472 to verify

Fixes curl#543
Closes curl#10705
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
curl bails out early with a different error message if http support is
compiled out.

Ref: curl#10705
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
name lookup DNS and related tech tests
Development

Successfully merging this pull request may close these issues.

None yet

3 participants