Bug 5363: Handle IP-based SANs in bumped connections #1793

walkert · 2024-04-25T13:28:10Z

PR Context

We are deploying squid in SSL-bump mode as a MITM proxy between clients and a 3rd party API server. The 3rd party server is known to us by IP only and is also multi-homed such that its SSL cert has multiple SANs of type IPADD.

First problem we encountered

During our testing, when connecting to the remote host, we received Unable to connect to the server: tls: failed to verify certificate: x509: cannot validate certificate for 10.1.1.2 because it doesn't contain any IP SANs. When inspecting the real cert and the generated cert, we discovered that the real cert had numerous SANs with DNS: and IP Address: prefixes but the generated cert contained a single SAN referencing the IP but with a DNS: prefix i.e. DNS: 10.1.1.2.

Commit d7abe92 introduces a helper function and logic change in addAltNameWithSubjectCn to use the correct prefix (DNS or IP) depending on the cn_data. With this fix in place, the real and generated certs for our 3rd party host match as expected.

Second problem we encountered

With the above fix in place, we then received a new TLS error when connecting to the host with IP 10.1.1.2: Certificate does match domainname: /CN=33.124.184.33. Looking through the code in matchX509CommonNames we discovered that if the CN doesn't match, only DNS-based SANs are used for comparison. Since the remote host is multi-homed, the IP we're using to connect to it and the IP it has as its CN do not match, so we must rely on SAN comparison.

Commit 06d33bb now performs a switch on the SAN type. For DNS names, it performs the original comparison. For IP addresses, it uses a newly introduced function which performs a byte comparison of the IP addresses. With this fix in place, we are able to communicate with the 3rd party host via squid successfully.

Happy to provide more info/context. Thanks!

squid-prbot · 2024-04-25T13:28:13Z

Can one of the admins verify this patch?

rousskov · 2024-04-26T13:22:14Z

@walkert, thank you for sharing these changes and detailing the logic behind them! I have not reviewed PR code changes yet, but I do have one higher-level question: In your opinion, does this PR address (or at least relates to) Bug 5363? If this PR addresses the same problem, then which solution is better, the patch posted for that bug report or this PR?

walkert · 2024-04-26T16:24:27Z

Thanks @rousskov. So, as far as I can see, the first commit in this PR fixes the bug you're referring to. That bug however does not resolve the second problem we saw which is fixed in the most-recent commit. In terms of which solution is better I would say it may depend on your appetite for changing existing code. My commit modifies the behaviour of the existing addAltNameWithSubjectCn to deal appropriately with IPs and DNS names. The patch from that bug introduces a new function addAltNameWithIpCn. Further, my first commit (which fixes the bug) introduces a helper function which is relied upon by the most recent commit. Looking again at the patch, the new addAltNameWithIpCn contains a fair amount of duplicative code from addAltNameWithSubjectCn and I'm not sure whether that's really necessary. Needless to say I'm open to whatever your preference is.

rousskov · 2024-04-28T03:05:01Z

@walkert, thank you for that analysis. We will proceed with your PR as the primary solution, at least for now. I hope to be able to review it, at least superficially next week.

walkert · 2024-04-28T14:34:27Z

Thanks @rousskov. I just noticed a failure due to my forgetting to declare one of the functions as static. Just fixed and pushed that change (then force-pushed a rebase of origin/master).

rousskov · 2024-04-28T15:09:17Z

(then force-pushed a rebase of origin/master).

FYI: There is no need to rebase (and force push) unless there are merge conflicts or other rare complications. Anubis, the merge bot used by Squid repository, will squash-merge all the changes into master (using PR title/description as the commit message) at the end of the merge process.

rousskov

I have left a few high-level change requests. Once those are addressed, I can help polish this code to meet various Squid code style guidelines.

rousskov · 2024-05-01T18:09:07Z

src/ssl/gadgets.cc

@@ -484,8 +505,16 @@ addAltNameWithSubjectCn(Security::CertPointer &cert)
 if (!cn_data)
 return false;

+ sockaddr_storage socket_info;
+ const char* altname_type;
+ const int is_ip = Ssl::is_ip_address(cn_data->data, &socket_info);


Please see if you can use Ip::Address::operator "=" instead of calling is_ip_address() here. Ip::Address::operator "=" API is terrible, but it is existing code that we do not want to semi-duplicate unless absolutely necessary (and we would have to semi-duplicate it differently if we have to do that at all).

N.B. This PR adds is_ip_address() that uses inet_pton(). Latest Squid code does not use inet_pton(). It uses Ip::Address::lookupHostIP(...,true) callers. That private Ip::Address method currently uses getaddrinfo().

Hi @rousskov. I wonder if you could help please. I'm attempting to use Ip::Address::operator by adding #include ip/Address.h to gadgets.cc. I then do something like this:

Ip::Address ipAddress; if (ipAddress = address_str) { type = "IP" } else { type = "DNS" }

After running make I get this error in the output:

/usr/bin/ld: ../../../../src/ssl/.libs/libsslutil.a(gadgets.o): in function `Ip::Address::Address()': /tmp/squid-6.9/src/ssl/../../src/ip/Address.h:46: undefined reference to `Ip::Address::setEmpty()' /usr/bin/ld: ../../../../src/ssl/.libs/libsslutil.a(gadgets.o): in function `addAltNameWithSubjectCn(Security::LockingPointer<x509_st, &Security::X509_free_cpp, HardFun<int, x509_st*, &X509_up_ref> >&)': /tmp/squid-6.9/src/ssl/gadgets.cc:490: undefined reference to `Ip::Address::operator=(char const*)' collect2: error: ld returned 1 exit status

I've tried to use a couple of the Ip::Address methods which resulted in the same error. Would you be able to point me in the right direction to fix this please?

Thanks

Shared output does not detail which executable fails to link, but I am guessing it is security_file_certgen. See the difference between two comments describing two libraries in src/ssl/Makefile.am. Compared to the primary squid executable, security_file_certgen helper is build using a (smaller but naturally growing) subset of libraries; we split src/ssl code into two libraries to minimize the number of (indirect) security_file_certgen dependencies¹.

If my guess is correct, then try adding $(top_builddir)/src/ip/libip.la to security_file_certgen_LDADD in src/security/cert_generators/file/Makefile.am. It should probably go on the first or second line, above or below libsslutil.la. You may also need to add more libraries if libip.la brings unresolved dependencies of its own¹... Do not forget to re-bootstrap after modifying an Automake file!

~~If the suggestion above works, then the original two-library split described above will no longer be necessary (but removing it should be done in a dedicated PR so do not worry about it for now).~~

Footnotes

We plan to migrate away from that painful minimization approach, but the code you have to deal with has not been upgraded, so you have to link security_file_certgen (and other binaries) with the necessary convenience libraries (often discovered by looking at linker errors like the one you have shared). ↩ ↩²

You were right. However, I've made the change you suggested to src/security/cert_generators/file/Makefile.am then I ran ./bootstrap.sh followed by ./configure from $(top_builddir) but I see no change in the error reported. Here it is with a bit more context:

make[4]: Entering directory '/portal/squid-6.9/src/security/cert_generators' Making all in file make[5]: Entering directory '/portal/squid-6.9/src/security/cert_generators/file' /bin/bash ../../../../libtool --tag=CXX --mode=link g++ -std=c++17 -Wall -Wextra -Wimplicit-fallthrough=5 -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow -Wmissing-declarations -Woverloaded-virtual -Werror -pipe -D_REENTRANT -I/usr/include/p11-kit-1 -g -O2 -march=native -g -o security_file_certgen certificate_db.o security_file_certgen.o ../../../../src/ip/libip.la ../../../../src/ssl/libsslutil.la ../../../../src/sbuf/libsbuf.la ../../../../src/debug/libdebug.la ../../../../src/error/liberror.la ../../../../src/comm/libminimal.la ../../../../src/mem/libminimal.la ../../../../src/base/libbase.la ../../../../src/time/libtime.la -lssl -lcrypto -lgnutls ../../../../compat/libcompatsquid.la libtool: link: g++ -std=c++17 -Wall -Wextra -Wimplicit-fallthrough=5 -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow -Wmissing-declarations -Woverloaded-virtual -Werror -pipe -D_REENTRANT -I/usr/include/p11-kit-1 -g -O2 -march=native -g -o security_file_certgen certificate_db.o security_file_certgen.o ../../../../src/ip/.libs/libip.a ../../../../src/ssl/.libs/libsslutil.a ../../../../src/sbuf/.libs/libsbuf.a ../../../../src/debug/.libs/libdebug.a ../../../../src/error/.libs/liberror.a ../../../../src/comm/.libs/libminimal.a ../../../../src/mem/.libs/libminimal.a ../../../../src/base/.libs/libbase.a ../../../../src/time/.libs/libtime.a -lssl -lcrypto -lgnutls ../../../../compat/.libs/libcompatsquid.a /usr/bin/ld: ../../../../src/ssl/.libs/libsslutil.a(gadgets.o): in function `Ip::Address::Address()': /portal/squid-6.9/src/ssl/../../src/ip/Address.h:46: undefined reference to `Ip::Address::setEmpty()' /usr/bin/ld: ../../../../src/ssl/.libs/libsslutil.a(gadgets.o): in function `addAltNameWithSubjectCn(Security::LockingPointer<x509_st, &Security::X509_free_cpp, HardFun<int, x509_st*, &X509_up_ref> >&)': /portal/squid-6.9/src/ssl/gadgets.cc:490: undefined reference to `Ip::Address::operator=(char const*)' collect2: error: ld returned 1 exit status

It seems like libsslutil is not seeing the link but I'm afraid I'm not sure how to proceed from here either if you can suggest please. Appreciate your help/patience.

... security_file_certgen.o ../../../../src/ip/libip.la ../../../../src/ssl/libsslutil.la ...

Try reordering the first two libraries. In other words, place libip after libsslutil in Makefile.am.

Rule of thumb: Libraries that provide symbol X should be listed after libraries that use symbol X.

Ah thanks. I had tried it below with a different source checkout and was sure I'd seen errors but doing as you suggest I was able to get make to complete. I'll now see about making some of these changes. Thanks.

rousskov · 2024-05-01T18:12:46Z

src/ssl/support.cc

+ // Cast check_data to const char
+ const unsigned char* check_ip = reinterpret_cast<const unsigned char*>(check_data);
+ sockaddr_storage socket_info;
+ if (!Ssl::is_ip_address(check_ip, &socket_info)) {


Please see if you can use Ip::Address::operator "=" instead of calling is_ip_address() here. If the assignment returns true, the resulting Ip::Address object should be usable for comparison.

Updated commits now use the = for comparison.

src/ssl/support.cc

rousskov · 2024-05-01T18:39:56Z

src/ssl/support.cc

+ // If it's an IP address, attempt to compare it with the check_data
+ if (compare_ip_addresses(check_data, check->d.iPAddress) == 0) {
+ sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free);
+ return 1;


This GEN_IPADD case addition feels like a Ssl::matchX509CommonNames() API violation to me. The function promises to (and did) outsource "yes, there is a match" conclusion to check_func. This PR-added code ignores that promise.

Ssl::matchX509CommonNames() API is used by several code paths. I suspect that many (if not all) of those paths would benefit from being able to handle GEN_IPADD entries.

I suspect that we have to adjust Ssl::matchX509CommonNames() API to replace check_func function with a C++ class object that has two methods: matchDomainName() and matchIp().

So, I've added 2 new commits. The most recent adds a VerifyAddress class. However, so that I don't have to mess with the calling semantics for matchX509CommonNames in other locations, I've retained the API to expect a check_func (or equivalent) but modified the body of the original check_func to use the new VerifyAddress::match method.

FWIW - I found it more simple to retain the switch and then ensure the correct ASN1_STRING is passed to match. So we're still outsourcing the check/conclusion to check_func but being purposeful about which data we send to it for comparison.

src/ssl/support.cc

walkert · 2024-05-09T08:55:05Z

src/security/cert_generators/file/Makefile.am

@@ -24,6 +24,7 @@ security_file_certgen_SOURCES = \

 security_file_certgen_LDADD = \
 $(top_builddir)/src/ssl/libsslutil.la \
+ $(top_builddir)/src/ip/libip.la \


Required to allow gadgets.cc to refer to ip/Address.h.

walkert · 2024-05-09T08:55:56Z

src/ssl/support.cc

+}
+
+int VerifyAddress::matchDomainNameData(ASN1_STRING *cn_data) {
+ char cn[1024];


Note that all logic from this method is copied from the original check_domain function.

rousskov

You have made significant progress, thank you! I left a few change requests for the next iteration. I continue to ignore most of the secondary issues that we will handle later.

rousskov · 2024-05-14T16:26:24Z

src/ssl/support.cc

+ debugs(83, 4, "Tried to get an IP from ASN1_OCTET_STRING but failed");
+ return 1;
+ }
+ server = strdup(buffer);


The duped memory will leak. I hope this code is refactored (for other reasons) and that refactoring removes this memory leak (so I am not suggesting a specific fix), but I am flagging it in case that does not happen.

rousskov · 2024-05-14T16:33:36Z

src/ssl/support.cc

+ char buffer[INET6_ADDRSTRLEN];
+ if (ASN1_STRING_length(asn1_data) == 4) {
+ // IPv4
+ inet_ntop(AF_INET, ip, buffer, sizeof(buffer));


inet_ntop(AF_INET) takes "struct in_addr (in network byte order)". If possible, please use Ip::Address constructor that takes in_addr to convert ASN1_STRING_get0_data() output to Ip::Address. Otherwise, please discuss why that is not possible.

Same for the other inet_ntop() call, but there the input bytes are "struct in6_addr (in network byte order)", of course.

Once you get an Ip::Address object here, do not fall through to the "Attempt to convert server into an IP and use matchIP if it's valid" code below -- call matchIp() (and return its result) ASAP! That "Attempt to convert..." code at the bottom of this method will be devoted to conversions from textual address representations...

rousskov · 2024-05-14T16:41:49Z

src/ssl/support.cc

+ } else {
+ // Otherwise, assume we have the cn_data from a typical ASN1_STRING
+ // This could be a domainname or IP address
+ server = (const char *)asn1_data->data;


Please add a C++ comment documenting what guarantees zero-termination of this ASN1_STRING::data buffer (assumed by this code)? If there is no such guarantee, then this code will need to be revised accordingly.

src/ssl/support.cc

rousskov · 2024-05-14T17:27:55Z

src/ssl/support.cc

+ case GEN_IPADD:
+ debugs(83, 4, "Check type is GEN_IPADD, verifying...");
+ // If it's an IP address, call check_func with the iPAddress data
+ if ( (*check_func)(check_data, check->d.iPAddress) == 0) {


N.B. The type of the second check_func() argument is (a pointer to) an ASN1_STRING. "Almost all ASN1 types in OpenSSL are represented as an ASN1_STRING structure. ASN1_STRING is also used for some CHOICE types." Roughly speaking, ASN1_STRING can store nearly anything.

I am concerned that this code loses "this ASN1_STRING is a GEN_IPADD SAN (rather than, say, GEN_DNS SAN)" knowledge and, hence, forces check_func() to guess what information its second parameter actually contains. That guess is currently based on ASN1_STRING_type(asn1_data) not being V_ASN1_OCTET_STRING. I assume that other CN and SAN types may pass that rather weak "type check", in current or future code, resulting in silently incorrect casting and incorrect matching in check_func() calls that are not as well protected as this one.

I do understand that you would prefer not to fiddle with other check_func callers. However, this information loss and associated dangerous guesses is a very large red flag for me. If I am missing some built-in protections that make this loss and guesses safe, please do let me know. If this concern is not addressed (one way or the other), I will find the time to start addressing it myself after the next iteration, after you address other change requests (that do not require changing other check_func() callers).

squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Apr 25, 2024

rousskov self-requested a review April 26, 2024 13:22

rousskov added the S-waiting-for-author author action is expected (and usually required) label Apr 26, 2024

rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Apr 28, 2024

rousskov changed the title ~~Introduce logic to deal with hosts that use IP-based SANs in SSL-bump mode~~ Bug 5363: Handle IP-based SANs in bumped connections Apr 28, 2024

walkert force-pushed the walkert/ssl-updates branch from d3c64da to 650db1b Compare April 28, 2024 14:32

rousskov requested changes May 1, 2024

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels May 2, 2024

Tony Walker and others added 5 commits May 9, 2024 09:44

Ensure bumped SSL certs get IP SANs if available

9724436

Enable matchX509CommonNames to match on IP

e24d680

Declare compare_ip_addresses as static

7b2541e

Switch to Ip::Address for IP verification

d387570

Add VerifyAddress class for matching IP/Domainname

e1d60a9

walkert force-pushed the walkert/ssl-updates branch from 650db1b to e1d60a9 Compare May 9, 2024 08:50

walkert commented May 9, 2024

View reviewed changes

rousskov requested changes May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 5363: Handle IP-based SANs in bumped connections #1793

Bug 5363: Handle IP-based SANs in bumped connections #1793

walkert commented Apr 25, 2024

squid-prbot commented Apr 25, 2024

rousskov commented Apr 26, 2024

walkert commented Apr 26, 2024

rousskov commented Apr 28, 2024

walkert commented Apr 28, 2024

rousskov commented Apr 28, 2024

rousskov left a comment

rousskov May 1, 2024

walkert May 2, 2024

rousskov May 2, 2024 •

edited

walkert May 3, 2024

rousskov May 3, 2024

walkert May 3, 2024

rousskov May 1, 2024

walkert May 9, 2024

rousskov May 1, 2024

walkert May 9, 2024

walkert May 9, 2024

walkert May 9, 2024

walkert May 9, 2024

rousskov left a comment

rousskov May 14, 2024

rousskov May 14, 2024

rousskov May 14, 2024

rousskov May 14, 2024

Bug 5363: Handle IP-based SANs in bumped connections #1793

Are you sure you want to change the base?

Bug 5363: Handle IP-based SANs in bumped connections #1793

Conversation

walkert commented Apr 25, 2024

PR Context

First problem we encountered

Second problem we encountered

squid-prbot commented Apr 25, 2024

rousskov commented Apr 26, 2024

walkert commented Apr 26, 2024

rousskov commented Apr 28, 2024

walkert commented Apr 28, 2024

rousskov commented Apr 28, 2024

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov May 2, 2024 • edited

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov May 2, 2024 •

edited