Re: [exim-dev] [Bug 1479] hostname check missing when verify…

Góra strony
Delete this message
Reply to this message
Autor: Viktor Dukhovni
Data:  
Dla: exim-dev
Temat: Re: [exim-dev] [Bug 1479] hostname check missing when verifying X509 certificate
On Sat, May 17, 2014 at 11:08:11AM +0100, Jeremy Harris wrote:

> Created an attachment (id=729)
> --> (http://bugs.exim.org/attachment.cgi?id=729)
> patch version 2


I see you implemented multiple hostname support, thanks! One
comment on the OpenSSL verify_callback. You're accessing the
current_cert structure element of X509_STORE_CTX directly.
OpenSSL provides an accessor method:

    X509 *X509_STORE_CTX_get_current_cert(X509_STORE_CTX *ctx);


you should probably use that, to avoid potential binary compatility
issues if the structure changes.

If (likely when) my patch for X509_check_host() is adopted you'll want
to specify a non-zero value for the final "flags" argument:

    X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS


I'll drop you a note when this happens. With this flag wildcards
can only take the form "*.example.com", not "mx*.example.com" or
"*mx.example.com". The latter are not applicable to SMTP per
RFC 6125, the DANE draft or the upcoming MTA-to-MTA namechecks
draft from the UTA working group.

> Made the option into a list. Renamed option "tls_verify_cert_hostnames".
> Ignored SANs with embedded NULs.


Thanks. I believe OpenSSL always NUL terminates the ASN1 string data
that represents a DNS altname. So you don't need to check for lack of
NUL termination.

+  if (ele[len])    /* not nul-terminated */
+    ele = string_copyn(ele, len);
+
+  if (strnlen(CS ele, len) == len)    /* ignore any with embedded nul */
+    list = string_append_listele(list, sep,


The copy code looks like a potential memory leak to me, but since the
check is not needed, you can just delete that code.

The payload of a DNS alternative name is always parsed as IA5String,
which is always nul-terminated when decoded. Postfix is paranoid, so
I check the ASN.1 string type:

#define TRIM0(s, l) do { while ((l) > 0 && (s)[(l)-1] == 0) --(l); } while (0)

    if (ASN1_STRING_type(gn->d.ia5) != V_ASN1_IA5STRING) {
        msg_warn("%s: %s: invalid ASN1 value type in subjectAltName",
                 myname, TLScontext->namaddr);
        return (0);
    }


    /*
     * Safe to treat as an ASCII string possibly holding a DNS name
     */
    dnsname = (char *) ASN1_STRING_data(gn->d.ia5);
    len = ASN1_STRING_length(gn->d.ia5);
    TRIM0(dnsname, len);


    /*
     * Per Dr. Steven Henson of the OpenSSL development team, ASN1_IA5STRING
     * values can have internal ASCII NUL values in this context because
     * their length is taken from the decoded ASN1 buffer, a trailing NUL is
     * always appended to make sure that the string is terminated, but the
     * ASN.1 length may differ from strlen().
     */
    if (len != strlen(dnsname)) {
        msg_warn("%s: %s: internal NUL in subjectAltName",
                 myname, TLScontext->namaddr);
        return 0;
    }


The TRIM0 macro deals with names that are NUL-padded (observed in
some oddly constructed certs, where there are are trailing NUL
octets in the ASN.1 encoded name).

-- 
    Viktor.