Regression: certificate host validation to strict if SAN present #875

Activity

1. bagder

   added

   TLS

   on Jun 14, 2016
2. bagder commented on Jun 14, 2016

   

   bagder

   on Jun 14, 2016

   Member

   More actions

   Yeah, that change clearly didn't take other SAN field types into account properly. My bad. I'll write up a suggested fix.
3. bagder commented on Jun 15, 2016

   

   bagder

   on Jun 15, 2016

   Member

   More actions

   Reverting would be one option, but i decided to instead step forward and try to adhere to the RFC2818 better which says that a dNSName field MUST match if present. Not entirely what curl did previously but should be what it does with this patch and it should work for your case @databus23 . Would be great if you could verify before I merge this.

   From c07990025984ec7bb58f9e91a14d197f3e71f5c2 Mon Sep 17 00:00:00 2001
   From: Daniel Stenberg <daniel@haxx.se>
   Date: Wed, 15 Jun 2016 15:36:40 +0200
   Subject: [PATCH] openssl: fix cert check with non-DNS name fields present
   
   Regression introduced in 5f5b62635 (released in 7.48.0)
   
   Reported-by: Fabian Ruff
   Fixes #875
   ---
    lib/vtls/openssl.c | 27 ++++++++++++++++++++-------
    1 file changed, 20 insertions(+), 7 deletions(-)
   
   diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
   index f702653..2f69790 100644
   --- a/lib/vtls/openssl.c
   +++ b/lib/vtls/openssl.c
   @@ -1080,10 +1080,11 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
      struct in6_addr addr;
    #else
      struct in_addr addr;
    #endif
      CURLcode result = CURLE_OK;
   +  bool dNSName = FALSE; /* if a dNSName field exists in the cert */
   
    #ifdef ENABLE_IPV6
      if(conn->bits.ipv6_ip &&
         Curl_inet_pton(AF_INET6, conn->host.name, &addr)) {
        target = GEN_IPADD;
   @@ -1100,20 +1101,27 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
      altnames = X509_get_ext_d2i(server_cert, NID_subject_alt_name, NULL, NULL);
   
      if(altnames) {
        int numalts;
        int i;
   +    bool dnsmatched = FALSE;
   +    bool ipmatched = FALSE;
   
        /* get amount of alternatives, RFC2459 claims there MUST be at least
           one, but we don't depend on it... */
        numalts = sk_GENERAL_NAME_num(altnames);
   
   -    /* loop through all alternatives while none has matched */
   -    for(i=0; (i<numalts) && !matched; i++) {
   +    /* loop through all alternatives - until a dnsmatch */
   +    for(i=0; (i < numalts) && !dnsmatched; i++) {
          /* get a handle to alternative name number i */
          const GENERAL_NAME *check = sk_GENERAL_NAME_value(altnames, i);
   
   +      /* If a subjectAltName extension of type dNSName is present, that MUST
   +         be used as the identity. / RFC2818 section 3.1 */
   +      if(check->type == GEN_DNS)
   +        dNSName = TRUE;
   +
          /* only check alternatives of the same type the target is */
          if(check->type == target) {
            /* get data and length */
            const char *altptr = (char *)ASN1_STRING_data(check->d.ia5);
            size_t altlen = (size_t) ASN1_STRING_length(check->d.ia5);
   @@ -1132,39 +1140,44 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
              */
              if((altlen == strlen(altptr)) &&
                 /* if this isn't true, there was an embedded zero in the name
                    string and we cannot match it. */
                 Curl_cert_hostcheck(altptr, conn->host.name)) {
   -            matched = TRUE;
   +            dnsmatched = TRUE;
                infof(data,
                      " subjectAltName: host \"%s\" matched cert's \"%s\"\n",
                      conn->host.dispname, altptr);
              }
              break;
   
            case GEN_IPADD: /* IP address comparison */
              /* compare alternative IP address if the data chunk is the same size
                 our server IP address is */
              if((altlen == addrlen) && !memcmp(altptr, &addr, altlen)) {
   -            matched = TRUE;
   +            ipmatched = TRUE;
                infof(data,
                      " subjectAltName: host \"%s\" matched cert's IP address!\n",
                      conn->host.dispname);
              }
              break;
            }
          }
        }
        GENERAL_NAMES_free(altnames);
   +
   +    if(dnsmatched || (!dNSName && ipmatched)) {
   +      /* count as a match if the dnsname matched or if there was no dnsname
   +         fields at all AND there was an IP field match */
   +      matched = TRUE;
   +    }
      }
   
      if(matched)
        /* an alternative name matched */
        ;
   -  else if(altnames) {
   -    /* an alternative name field existed, but didn't match and then we MUST
   -       fail */
   +  else if(dNSName) {
   +    /* an dNSName field existed, but didn't match and then we MUST fail */
        infof(data, " subjectAltName does not match %s\n", conn->host.dispname);
        failf(data, "SSL: no alternative certificate subject name matches "
              "target host name '%s'", conn->host.dispname);
        result = CURLE_PEER_FAILED_VERIFICATION;
      }
   -- 
   2.8.1
   
4. bagder

   self-assigned this

   on Jun 15, 2016
5. databus23 commented on Jun 15, 2016

   

   databus23

   on Jun 15, 2016

   Last edited by databus23

   Author

   More actions

   I verified against latest master that the patch indeed fixes the reported issue. 👍
6. bagder

   closed this as completedin d4643d6on Jun 16, 2016
7. databus23 commented on Jun 16, 2016

   

   databus23

   on Jun 16, 2016

   Author

   More actions

   @bagder Thanks! Whats the current ETA for 7.50?
8. bagder commented on Jun 16, 2016

   

   bagder

   on Jun 16, 2016

   Member

   More actions

   The release calendar currently says July 13th, but I need to change that for personal reasons. So I don't know yet...
9. wmsch commented on Aug 11, 2016

   

   wmsch

   on Aug 11, 2016

   More actions

   Regarding the code with comment:
   /* an dNSName field existed, but didn't match and then we MUST fail */
   If an IP address is specified in the request instead of a name, and it matches an address in the iPAddress field, and a dNSName field exists, shouldn't this still work? It does not work for me in 7.50.1 if both iPAddress and dNSName are provided. If dNSName does not exist, it works.
10. jay commented on Aug 12, 2016

    

    jay

    on Aug 12, 2016

    Member

    More actions

    @wmsch that might be a bug, I've filed it as #959, please take the conversation there thanks
11. wmsch commented on Oct 6, 2016

    

    wmsch

    on Oct 6, 2016

    More actions

    Regarding the initial problem statement from @databus23, Common Name is still not checked for match on hostname for server certificates. Although this is no longer a requirement, it is allowed by RFC6125 Section 6.4.4, as databus23 observed, and this is still a problem for users with such server certificates.
12. bagder commented on Oct 6, 2016

    

    bagder

    on Oct 6, 2016

    Member

    More actions

    CN is checked, but only if there's no SAN field used. That's also what that the spec says we should do. I suppose you're saying there's some case when this doesn't work? If so, that's a bug and you should probably file an issue with all the details in it. This issue you're commenting in here has already been fixed.

    👍React with 👍1Reacted by Jay Satiro
13. lock

    locked as resolved and limited conversation to collaborators on May 7, 2018