squid
squid copied to clipboard
squid-cache
Bug 5154: Reject IPv6 address when IPv6 is disabled
Address.cc:663 getAddrInfo() assertion failed: false
Commit fd9c47d added Ip::Address::fromHost(). That method relies on the old lookupHostIP() method for the final parsing steps. Unfortunately, lookupHostIP() could successfully return an IPv6 address despite disabled IPv6 support. Injecting an IPv6 address into IPv6-disabled Squid leads to an Ip::Address::getAddrInfo() assertion if Squid tries to open the connection to the corresponding destination.
This Ip::Address::lookupHostIP() adjustment eliminated that particular IPv6 injection path but broke two unit tests because those tests incorrectly expected Ip::Address and HttpRequest::FromUrl() APIs to successfully import IPv6 addresses even when IPv6 support was disabled.
While fixing those unit tests, we discovered that they never tested Squid with IPv6 support enabled! That bug was fixed by adding missing Ip::ProbeTransport() calls to those tests.
With these changes, Squid starts rejecting more non-CONNECT URLs containing IPv6 and IPv6-like addresses, including these two cases:
- all bracketed addresses when IPv6 support is disabled;
- bracket-less addresses with two colons that are not IPv6 addresses.
More rejections are expected in the foreseeable future as we tighten received request target validation.
Co-authored-by: Jeffrey Kintscher [email protected]
It looks like these two tests are failing:
2023-07-13T18:53:37.6960895Z FAIL: tests/testHttpRequest 2023-07-13T18:53:37.6960984Z FAIL: tests/testIpAddress
===================================================
Squid Web Proxy 7.0.0-VCS: src/test-suite.log
===================================================
# TOTAL: 34
# PASS: 32
# SKIP: 0
# XFAIL: 0
# FAIL: 2
# XPASS: 0
# ERROR: 0
.. contents:: :depth: 2
FAIL: tests/testHttpRequest
===========================
SKIP: mem/libmem.la MemPools (not implemented).
SKIP: security/libsecurity.la PeerOptions (not implemented).
SKIP: store/libstore.la Controller (not implemented).
.SKIP: mem/libmem.la Init (not implemented).
.SKIP: mem/libmem.la Init (not implemented).
F.SKIP: mem/libmem.la Init (not implemented).
.........
testHttpRequest.cc:131:Assertion
Test name: TestHttpRequest::testIPv6HostColonBug
equality assertion failed
- Expected: [2000:800::45]
- Actual : 2000:800::45
Failures !!!
Run: 12 Failure total: 1 Failures: 1 Errors: 0
SKIP: store/libstore.la ~Controller (not implemented).
FAIL tests/testHttpRequest (exit status: 1)
FAIL: tests/testIpAddress
=========================
SKIP: mem/libmem.la MemPools (not implemented).
.......F........stub time| ERROR: Unable to convert '192.168.100.12' to the rDNS type requested.
.F.
testIpAddress.cc:295:Assertion
Test name: TestIpAddress::testStringConstructor
assertion failed
- Expression: !bnIPA.isAnyAddr()
testIpAddress.cc:647:Assertion
Test name: TestIpAddress::testMasking
assertion failed
- Expression: !maskIPA.isNoAddr()
Failures !!!
Run: 17 Failure total: 2 Failures: 2 Errors: 0
FAIL tests/testIpAddress (exit status: 1)
Glad you are making progress with the triage. Most likely, we will need to modify primary Squid code and/or the test cases themselves. Please ping me if you need help addressing these failures.
@rousskov I'm not 100% sure what these tests are trying to do, I've manually tested this change using curl:
IPv6 enbled proxy host curl -v -k -x proxyhost:8080 https://google.com curl -v -k -x proxyhost:8080 https://142.250.187.206 curl -v -k -x proxyhost:8080 https://[2a00:1450:4001:828::200e]
They all work
IPv4 only proxy host
curl -v -k -x proxyhost:8080 https://google.com curl -v -k -x proxyhost:8080 https://142.250.187.206
Works
curl -v -k -x proxyhost:8080 https://[2a00:1450:4001:828::200e]
Fails with a 503 - which I guess is expected and happens without my change
I'll be putting 6.1 with this patch into production (the only place we saw the segfaults) next week some time
testHttpRequest.cc:131:Assertion
Test name: TestHttpRequest::testIPv6HostColonBug
equality assertion failed
- Expected: [2000:800::45]
- Actual : 2000:800::45
I'm not 100% sure what these tests are trying to do
The failing TestHttpRequest::testIPv6HostColonBug(), for example, has three test cases, each verifying FromUrl() outcome for certain inputs. Just to clarify, did you look at the corresponding test source code (e.g., src/tests/testHttpRequest.cc) and could not figure out why that test could start failing after the changes in this PR?
I think I'm understanding what the issue is....
http://2000:800::45/foo is being tested, and it isn't being detected at IPv6
+ else {
+ /*
+ * NP: =(sockaddr_*) may alter the port. we don't want that.
+ * all we have been given as input was an IPA.
+ */
+ short portSaved = port();
+ operator=(*res);
+ port(portSaved);
+
+ freeaddrinfo(resHead);
+ return false;
+ }
I tried changing my patch to this, and it passes "make check"
Though I imagine it would be better to change the "return true" rather than duplicating all that code?
The core problems here are very different AFAICT.
I fixed one of them in branch commit 6c9cb63, but that commit does not fix all the problems that should be fixed. Please give me a few hours to get through this mess.
I'll rebuilt 6.1 tomorrow and redeploy to our test proxies. I should be able to schedule production for Thursday & Friday
I've been running this patch in production since Thursday (again't 6.1 without the test changes) and there have been no issuess
@yadij, the changes in this PR will start blocking some requests, including some requests that may have been allowed and "working" (at least to a certain extent, like fetching cache hits) before. The most relevant PR description part is quoted below for your convenience, but it is easy to misinterpret outside of its context. If something looks like a red flag, I recommend reviewing this PR as a whole.
I currently plan to clear this PR for merging in about 10 calendar days.
With these changes, Squid starts rejecting more non-CONNECT URLs containing IPv6 and IPv6-like addresses, including these two cases:
- all bracketed addresses when IPv6 support is disabled;
- bracket-less addresses with two colons that are not IPv6 addresses.
More rejections are expected in the foreseeable future as we tighten received request target validation.
I've been running this patch in production since Thursday (again't 6.1 without the test changes) and there have been no issuess
@FireBurn, thank you for testing and sharing your test results! I assume that you were using an IPv6-disabled build or an IPv6-disabled environment, but please correct me if I am wrong.
Squid was IPv6 enabled, the environment was IPv4 only
Hey, are we any closer to getting this bug fixed? And by that I mean squid segfaulting
Hey, are we any closer to getting this bug fixed? And by that I mean squid segfaulting
Hi @FireBurn , from what I can see the PR is waiting for you to accept the review suggestions; have I missed anything?
Francesco: the PR is waiting for you to accept the review suggestions; have I missed anything?
Yes, you have:
- I am strongly against accepting those suggestions: https://github.com/squid-cache/squid/pull/1421#discussion_r1296146980
- There is another problem that must be fixed before this PR can be merged: https://github.com/squid-cache/squid/pull/1421#discussion_r1329158283
Hey, are we any closer to getting this bug fixed?
Not since September 18, 2023. The PR is currently stuck. AFAICT, at least two things are required to make progress:
- Resolving several change requests that I have addressed or objected to, starting with https://github.com/squid-cache/squid/pull/1421#discussion_r1278482454. This is on @yadij side since August 16, 2023. However, technically, you (i.e. the PR author) have not reacted either, so this thread can be seen as being on your side as well.
- Solving the
ipv6problem. That resolution is waiting for feedback from others (including you and @yadij, of course) since September 18, 2023. It is possible that solving that problem will result in a dedicated PR or significant changes in this PR. I do not want to proceed (with my preferred implementation) without feedback from others.
I know I sound like a broken record, but I would like to get this fixed upsteam, we've been using the original fix from https://github.com/squid-cache/squid/pull/947 for over a year to stop segfaults, that I guess could be used as a denial of service attack. I realise you want to get this fixed properly but is there a way to reduce this attack vector whilst you argue about how that should be implemented?
I would like to get this fixed upsteam
I assume that "this" is "bug 5154". I believe that bug has been fixed in master/v7 commit 97bbba6 a couple of weeks ago (FWIW, I have mentioned that effort in this PR three weeks ago). The fix has been earmarked for v6 and v5 backports a week ago.
we've been using the original fix from #947 for over a year to stop segfaults, that I guess could be used as a denial of service attack.
Yes, assertions can be used in attacks. Some folks prefer loud assertions to, say, silent http_access rules violations, but YMMV. IMHO, the underlying problem here is bigger and more nuanced than it may seem. See also: 51c518d.
I realise you want to get this fixed properly but is there a way to reduce this attack vector whilst you argue about how that should be implemented?
Yes, of course: I hope I have done that in 97bbba6.
FWIW, I believe that the problem with this PR is not endless arguments about how this bug should be "fixed properly"(sic), but lack of activity. I hate sounding like a broken record, but, IMHO, the primary responsibility for advancing a PR (including stimulating healthy activity, tracking progress, and flagging deadlocks) lies with PR author. Squid Project idiosyncrasies certainly complicate that task, and I am not implying that task is easy (or that a PR author has the power to ensure progress in all cases). I am implying (as gently as I can) that you are primarily responsible for this PR progress (especially since ~September 18, 2023 as detailed at https://github.com/squid-cache/squid/pull/1421#issuecomment-1768539537). I am happy to help.