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

url: improve private IP protection #2285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

half-duplex
Copy link
Member

@half-duplex half-duplex commented May 25, 2022

Description

Atop #2282
Fixes #2284

Draft implementation should resolve #2348.

  • Document TOCTOU

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@half-duplex half-duplex added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label May 25, 2022
@half-duplex half-duplex added this to the 8.0.0 milestone May 25, 2022
@half-duplex
Copy link
Member Author

  • Passing bot.memory to find_title feels weird, but it's needed for the safety checks, which need to be done each redirect
  • Both methods of getting the IPs for a hostname ignore whether a system has working IPv6. This is a really annoying problem I didn't foresee.
    • Check for a global IPv6 address? (But some working v6 doesn't have that)
    • Check for IPv6 routing? (Feels dirty
    • If TTL is over a second or two, assume it'll be cached and accept the edge case TOCTOUs?
      • Needs fallback. Fall back to only supporting IPv4? Gross, but only needed when TTL<2, so would work 99.999% of the time.

@dgw
Copy link
Member

dgw commented Jun 4, 2022

With #2282 merged, presumably this can proceed toward Ready status after a rebase.

Any idea why this affects VCR tests? Somehow it is causing the example request for google.com to appear as a direct IP instead.

@half-duplex
Copy link
Member Author

half-duplex commented Jun 4, 2022

If the test checks for the address used in the requests.get call, it'll be getting something different from what it used to in some cases. I figured I'd look into that after seeing if I'm even continuing with this redirect handling strategy because of the above questions.

@dgw
Copy link
Member

dgw commented Jun 10, 2022

Assuming this PR graduates from draft status, please remove dnspython from requirements. These changes remove its use from url.py (🥳), and nothing else uses it.

@half-duplex half-duplex force-pushed the url-private-safety branch 5 times, most recently from f103f82 to 2743253 Compare July 10, 2022 20:32
@half-duplex
Copy link
Member Author

Rebased, but still looking for feedback on the above questions.

sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor

Exirel commented Jul 13, 2022

accept the edge case TOCTOUs

Yes. With your work in particular, Sopel tries a lot of thing to mitigate the attack surface, which is good enough in my opinion. If someone has a server where you can mess it up with a GET request, or that leaks sensible data if you access a URL without authentication, then it's bound to happen, Sopel or not.

And you are doing a pretty good job at trying to prevent that, so kuddo!

@dgw
Copy link
Member

dgw commented Jan 2, 2023

@half-duplex Did you still want to get this in? Haven't seen any activity for a while.

@dgw dgw added the Stale Mostly used for PRs that no longer work and need updating before re-review/merge. label Mar 7, 2023
@dgw
Copy link
Member

dgw commented Mar 23, 2023

Revisited this a bit today on IRC, especially as regards the unresolved TOCTOU problem:

17:56:31 <+mal> the toctou issue cannot be fixed without some invasive stuff, iirc
17:56:50 <+mal> so if anyone cares to actually try to bypass the local IP protection, they will
17:57:20 <+mal> the feature, unless I do the invasive stuff, can only protect against lazy attackers and
                unintended attempts
17:58:26 <+dgw> I think xrl gave a good perspective in
                https://github.com/sopel-irc/sopel/pull/2285#issuecomment-1183750062
17:58:52 <+mal> yeah, fair
17:59:23 <+dgw> This kinda falls under the idea that "Sopel is a toy; don't use it to guard your nuclear
                launch codes"
17:59:27 <+xrl> That's also why no one should ever have a internal system not locked behind some sort
                of authentication.
18:00:02 <+mal> dgw: in my mind, either the feature should Work Properly™ or it should have clear
                documentation about its threat model
18:00:36 <+dgw> clear documentation would be fine; we can totally push some of the netsec stuff
                onto the bot owner, if they're running it someplace it can access LAN resources
18:00:43 <+mal> yeah

@dgw dgw modified the milestones: 8.0.0, 8.1.0 Jun 15, 2023
@half-duplex
Copy link
Member Author

For when I look at this next: requests/toolbelt#293 was merged

@half-duplex half-duplex force-pushed the url-private-safety branch 2 times, most recently from 5e56e07 to a5c9234 Compare November 12, 2023 20:16
@half-duplex half-duplex marked this pull request as ready for review November 12, 2023 20:16
@half-duplex half-duplex added Security Issues or PRs with security or privacy implications and removed Stale Mostly used for PRs that no longer work and need updating before re-review/merge. labels Nov 12, 2023
@half-duplex half-duplex changed the title url: fix private IP protection url: improve private IP protection Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Security Issues or PRs with security or privacy implications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

url: private_resolution/dns_resolution useless url: Unexpected error on nonexistent domains
3 participants