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

Additional IP range validations #1152

Merged
merged 4 commits into from
Nov 26, 2022

Conversation

daenney
Copy link
Member

@daenney daenney commented Nov 26, 2022

Commits are individually reviewable.

Since I was looking at this code yesterday I decided to take another pass through it:

  • Added a check on the network to ensure it's always TCP
  • Added IPv4-mapped range to v6 blocklist and add test for it. The behaviour is different between net and netip so it's useful to test and ensure we don't break this at some point
  • I've aligned the reserved blocks with the respective IANA Special Purpose registries for v4 and v6
  • Added some tests to ensure things are blocked especially for all the magical /32's on v4

It's possible for the network to be udp4 or udp6. This is rather
unlikely to occur, but since we're given the network anyway as part of
the Sanitize function getting called we might as well check for it.
The net and netip packages diverge in that net.ParseIP will consider an
IPv4-mapped address to be an IPv4 address and as such it would get
caught by the IPv4Reserved list. However, netip considers it an IPv6
address, so we need to ensure the mapped range is in IPv6Reserved.
This includes a number of tests for /32's explicitly called out in the
registry to ensure we always consider those invalid.
@daenney
Copy link
Member Author

daenney commented Nov 26, 2022

This test feels a little redundant now:

func TestHTTPClientPrivateIP(t *testing.T) {

But it's also a little underspecified given what it tests for:

var privateIPs = []string{
"http://127.0.0.1:80",
"http://0.0.0.0:80",
"http://192.168.0.1:80",
"http://192.168.1.0:80",
"http://10.0.0.0:80",
"http://172.16.0.0:80",
"http://10.255.255.255:80",
"http://172.31.255.255:80",
"http://255.255.255.255:80",
}

I'm not sure what to do about that one, because retaining at least one test to ensure the Sanitizer is properly hooked up is valuable.

@NyaaaWhatsUpDoc
Copy link
Member

This test feels a little redundant now:

func TestHTTPClientPrivateIP(t *testing.T) {

But it's also a little underspecified given what it tests for:

var privateIPs = []string{
"http://127.0.0.1:80",
"http://0.0.0.0:80",
"http://192.168.0.1:80",
"http://192.168.1.0:80",
"http://10.0.0.0:80",
"http://172.16.0.0:80",
"http://10.255.255.255:80",
"http://172.31.255.255:80",
"http://255.255.255.255:80",
}

I'm not sure what to do about that one, because retaining at least one test to ensure the Sanitizer is properly hooked up is valuable.

Leave it for now. As you say it checks the sanitizer is at least hooked up correctly :)

@NyaaaWhatsUpDoc
Copy link
Member

If you're happy with everything here @daenney I'll merge this. Looks good to me, and thank you for the contribution!

@daenney
Copy link
Member Author

daenney commented Nov 26, 2022

Good to go from my side!

(also wow you're fast, I was not expecting a review at this time on a weekend)

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc merged commit 746f3fa into superseriousbusiness:main Nov 26, 2022
@daenney daenney deleted the more-ssrf branch November 26, 2022 11:10
@NyaaaWhatsUpDoc
Copy link
Member

Good to go from my side!

(also wow you're fast, I was not expecting a review at this time on a weekend)

I work on this project outside my main job so more often than not you'll probably find me working at odd hours :'). Also your code is well written and easily reviewable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants