Broken code in lib/vtls/x509asn1.c Curl_verifyhost

[piru]

Description

Curl_verifyhost has some broken code due to recent code shuffling. The issue is visible here:
https://github.com/curl/curl/blob/curl-7_87_0/lib/vtls/x509asn1.c#L1355

The code doesn't compile and it also leaks some memory for empty ("") strings if parenthesis are added. The issue was introduced by commit 6b19247

The easiest solution is to revert the first section of the patch. Note how utf8asn1str always sets the pointer to NULL on error.

curl/libcurl version

curl-7_87_0 tag

[danielgustafsson]

Curl_verifyhost has some broken code due to recent code shuffling. The issue is visible here: https://github.com/curl/curl/blob/curl-7_87_0/lib/vtls/x509asn1.c#L1355

Meh, I was twiddling with codeformattting and ended up with the wrong patch =( This codepath only compiles and runs for GSKit which makes it quite rare. Thanks for spotting it.

The code doesn't compile and it also leaks some memory for empty ("") strings if parenthesis are added. The issue was introduced by commit 6b19247

Is there valid input which can produce an empty string be returned from utf8asn1str() here? We could add a check for zero-length hostnames but I'm not convinced it's a case that will happen.

The easiest solution is to revert the first section of the patch.

I think the easiest solution is to fix the issue by apply the following:

diff --git a/lib/vtls/x509asn1.c b/lib/vtls/x509asn1.c
index 4c1c9a8b7..2ab07eca7 100644
--- a/lib/vtls/x509asn1.c
+++ b/lib/vtls/x509asn1.c
@@ -1352,7 +1352,7 @@ CURLcode Curl_verifyhost(struct Curl_cfilter *cf,
           len = utf8asn1str(&dnsname, CURL_ASN1_IA5_STRING,
                             name.beg, name.end);
           if(len > 0) {
-            if(size_t)len == strlen(dnsname)
+            if((size_t)len == strlen(dnsname))
               matched = Curl_cert_hostcheck(dnsname, (size_t)len,
                                             connssl->hostname, hostlen);
             free(dnsname);

Note how utf8asn1str always sets the pointer to NULL on error.
Right, but the codepath here is not in executed in case of errors but when it returns true.

[piru]

Is there valid input which can produce an empty string be returned from utf8asn1str() here?

That's beside the point. The original fix was for scenario that didn't happen anyway, so fixing it with another actually buggy version (even if rarely if ever occurring) is suboptimal.

We could add a check for zero-length hostnames but I'm not convinced it's a case that will happen.

There is no need for it. I'd just make the code call free(dnsname); unconditionally. Like it used to originally.

The premise:

When utf8asn1str fails there is no allocation returned, so freeing
the return pointer in **to is at best a no-op and at worst a double-
free bug waiting to happen.

...is faulty in a way that it does return NULL on failures. There is no chance of double free.

I do agree that there was an obviously unnecessary free call in the 2nd part of the patch and I see no problem with removing that.

[bagder]

Maybe this is a hint that we should drop support for this platform as it clearly is not tested much. Not even a single user has reported this problem nine days after the release...

[danielgustafsson]

On 30 Dec 2022, at 16:02, Daniel Stenberg ***@***.***> wrote:  Maybe this is a hint that we should drop support for this platform as it clearly is not tested much. Not even a single users has reported this problem nine days after the release...

That’s my thinking as well, fix it for next and deprecate by 8 or just rip it for next?

[bagder]

As per our deprecation procedure, we add a mention to the DEPRECATE.md document with at least a 6 months notification period. So we need to fix this now anyway.

[jonrumsey]

For the last 5 years I have been building and testing new Curl releases on OS400 and usually raise and fix issues usually within a day (or two) of a release, unfortunately 7.87 dropped a couple of days after I'd already turned my work laptop off for the duration of the 2022 Christmas holidays. I missed this and I am only just picking up this new release today, I do appreciate that this issue has already been found/closed already so I can apply the patch :-)

The IBM GSKit runtime is available on many other non-IBM platforms other than OS400/AIX & z/OS, including various flavours of Linux, Windows, Mac, etc and it does provide support all modern TLS features - even if the lib/vtls/gskit.c code has not yet implemented them.

I do however acknowledge that the GSKit developer kit and header files that would be necessary for anyone to build with GSKit backend on other platforms are not made publicly available (only on OS400) and so that raises issues with maintaining GSKit on just the OS400 platform in this project.

I will need to plan to mitigate the impact of this deprecation. Would it be a safe assumption that CURLSSLBACKEND_GSKIT will continue be defined/reserved? This seems to have been true for previous deprecations of POLARSSL, AXTLS and MESALINK.

[bagder]

First: nothing has changed yet and nothing will change until earliest the date mentioned in DEPRECATE. This is also a call intended to trigger reactions and for reconsideration. This is a road we can decide to travel or not.

The concerns I express about the gskit support are real. If they would be addressed in suitable ways, I think we can discuss how the future should look like.

Would it be a safe assumption that CURLSSLBACKEND_GSKIT will continue be defined/reserved

Yes, we cannot change that without breaking the ABI so it is there to stay.