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

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@smuehlst
Contributor

smuehlst commented Apr 13, 2018

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.

Verify host name against subjectAltName on non-ASCII platforms
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

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Apr 16, 2018

Member

The travis red is a failed make checksrc:

./vtls/openssl.c:1430:75: warning: badly placed open brace (BRACEPOS)
                                                                           {
                                                                           ^
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 SSL/TLS label Apr 16, 2018

@smuehlst

This comment has been minimized.

Show comment Hide comment
@smuehlst

smuehlst Apr 17, 2018

Contributor

Fixed "make checksrc" problem.

Contributor

smuehlst commented Apr 17, 2018

Fixed "make checksrc" problem.

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Apr 19, 2018

Member

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
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

This comment has been minimized.

Show comment Hide comment
@smuehlst

smuehlst Apr 20, 2018

Contributor

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.

Contributor

smuehlst commented Apr 20, 2018

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

This comment has been minimized.

Show comment Hide comment
@smuehlst

smuehlst Apr 20, 2018

Contributor

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,

Contributor

smuehlst commented Apr 20, 2018

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,

Refactored subjectAltName check into separate function
For readability reasons the matching of the subjectAltName entries
against the hostname was moved to a function of its own.
@smuehlst

This comment has been minimized.

Show comment Hide comment
@smuehlst

smuehlst Apr 20, 2018

Contributor

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.

Contributor

smuehlst commented Apr 20, 2018

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

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Apr 20, 2018

Member

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment