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

Verify host name against subjectAltName on non-ASCII platforms #2493

Closed
wants to merge 3 commits into from

Conversation

smuehlst
Copy link
Contributor

Curl_cert_hostcheck operates with the host character set, therefore the ASCII subjectAltName string retrieved with OpenSSL must be converted to the host encoding before comparison.

This was discovered on a z/OS machine with EBCDIC encoding, where HTTPS connections to servers with subjectAltName in the server certificate failed.

Curl_cert_hostcheck operates with the host character set, therefore
the ASCII subjectAltName string retrieved with OpenSSL must be
converted to the host encoding before comparison.
@bagder
Copy link
Member

bagder commented Apr 16, 2018

The travis red is a failed make checksrc:

./vtls/openssl.c:1430:75: warning: badly placed open brace (BRACEPOS)
                                                                           {
                                                                           ^

@bagder bagder added the TLS label Apr 16, 2018
@smuehlst
Copy link
Contributor Author

Fixed "make checksrc" problem.

@bagder
Copy link
Member

bagder commented Apr 19, 2018

There's a second place in the file where Curl_cert_hostcheck() is called. Doesn't that require the same procedure?

And regarding the code, I would prefer an approach that uses less #ifdefs in place. Perhaps you can just provide a wrapper function that does the conversions in this style:

#ifdef CURL_DOES_CONVERSIONS
static int hostcheck_convert(const char *match_pattern, const char *hostname)
{
  char *altptr2 = strdup(altptr);
  if(altptr2) {
    if(!Curl_convert_from_network(data, altptr2, altlen))
        rc = Curl_cert_hostcheck(altptr2, hostname);
    else
        rc = failcode;
    free(altptr2);
  }
  return rc;
}

#define Curl_cert_hostcheck(x,y) hostcheck_convert(x,y)
#endif

@smuehlst
Copy link
Contributor Author

There's a second place in the file where Curl_cert_hostcheck() is called. Doesn't that require the same procedure?

You are right, I need to check this. If I understand it correctly the second place is triggered when a certificate does not have the subjectAltName extension. I'm not sure at the moment that a conversion is needed there as well, because our previous test environment had a server certificate without subjectAltName extension, and then it worked as expected, albeit with an older version of curl. The current problem on z/OS only surfaced when we created a new server certificate with subjectAltName extension.

And regarding the code, I would prefer an approach that uses less #ifdefs in place. Perhaps you can just provide a wrapper function that does the conversions in this style: ...

I will refactor the code accordingly.

@smuehlst
Copy link
Contributor Author

There's a second place in the file where Curl_cert_hostcheck() is called. Doesn't that require the same procedure?

The second place within verifyhost() where Curl_cert_hostcheck()is called does not need to be modified, because there the host name in peer_CN is already converted to host encoding by Curl_convert_from_utf8():

CURLcode rc = Curl_convert_from_utf8(data, (char *)peer_CN,

For readability reasons the matching of the subjectAltName entries
against the hostname was moved to a function of its own.
@smuehlst
Copy link
Contributor Author

I moved the check against subjectAltName into a separate function. The proposed macro trick to redefine the function Curl_cert_hostcheck() was not possible because the function needs additional parameters.

@bagder bagder closed this in b0a5022 Apr 20, 2018
@bagder
Copy link
Member

bagder commented Apr 20, 2018

Thanks, the red travis failure is unrelated.

I took the liberty to modify your function slightly before I merged, please have a look and shout if you think I massacred something.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants