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

Broken code in lib/vtls/x509asn1.c Curl_verifyhost #10163

Closed
piru opened this issue Dec 26, 2022 · 7 comments
Closed

Broken code in lib/vtls/x509asn1.c Curl_verifyhost #10163

piru opened this issue Dec 26, 2022 · 7 comments
Labels

Comments

@piru
Copy link

piru commented Dec 26, 2022

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
Copy link
Member

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.

@bagder bagder added the TLS label Dec 26, 2022
@piru
Copy link
Author

piru commented Dec 26, 2022

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
Copy link
Member

bagder commented Dec 30, 2022

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
Copy link
Member

danielgustafsson commented Dec 30, 2022 via email

@bagder
Copy link
Member

bagder commented Jan 2, 2023

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.

bagder added a commit that referenced this issue Jan 2, 2023
bagder added a commit that referenced this issue Jan 2, 2023
@bagder bagder closed this as completed in 4fc7737 Jan 3, 2023
bagder added a commit that referenced this issue Jan 3, 2023
Ref: #10163

- This is a niche TLS library, only running on some IBM systems
- no regular curl contributors use this backend
- no CI builds use or verify this backend
- gskit, or the curl adaption for it, lacks many modern TLS features
  making it an inferior solution
- build breakages in this code take weeks or more to get detected
- fixing gskit code is mostly done "flying blind"

Closes #10201
@jonrumsey
Copy link
Contributor

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
Copy link
Member

bagder commented Jan 3, 2023

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.

bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
Ref: curl#10163

- This is a niche TLS library, only running on some IBM systems
- no regular curl contributors use this backend
- no CI builds use or verify this backend
- gskit, or the curl adaption for it, lacks many modern TLS features
  making it an inferior solution
- build breakages in this code take weeks or more to get detected
- fixing gskit code is mostly done "flying blind"

Closes curl#10201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

4 participants