ECH-draft-13c: s_server ctx switch (switch to inner) bug

[Neimhin]

Fix here (but I don't know how to create a PR across repos): https://github.com/Neimhin/openssl/pull/4/files

In s_server we should switch to ctx2 when ECH is successful and when the inner SNI matches the host of the cert in ctx2. I believe the logic currently switches context whenever ECH is successful and an inner SNI is provided (regardless of whether the inner SNI matches the cert for ctx2).

This if condition in [1] is, I think, always true.

I haven't yet run any code to test this, let me know if you want me to do a demo of this bug.

The comment preceding the servername callback function says to use X509_check_host [2], which actually checks whether a hostname matches the CN of the cert (but perhaps does not verify it?). It seems to me it would be okay to not verify the hostname since the server owns the cert.

[1] https://github.com/sftcd/openssl/blob/61451686e836f20c50b92d4ed4fa10ac7d38a0de/apps/s_server.c#L682

[2] https://github.com/sftcd/openssl/blob/61451686e836f20c50b92d4ed4fa10ac7d38a0de/apps/s_server.c#L522

[Neimhin]

Relevant documentation [1]:

X509_VERIFY_PARAM_set1_host() sets the expected DNS hostname to name clearing any previously specified hostname. If name is NULL, or empty the list of hostnames is cleared, and name checks are not performed on the peer certificate. If name is NUL-terminated, namelen may be zero, otherwise namelen must be set to the length of name. When a hostname is specified, certificate verification automatically invokes X509_check_host(3) with flags equal to the flags argument given to X509_VERIFY_PARAM_set_hostflags() (default zero). Applications are strongly advised to use this interface in preference to explicitly calling X509_check_host(3), hostname checks may be out of scope with the DANE-EE(3) certificate usage, and the internal check will be suppressed as appropriate when DANE verification is enabled.

The documentation strongly recommends not directly using X509_check_host, but I think that is for the case of verifying peer certificates. In our case we are checking our own certificate, not a peer certificate, but with a peer-supplied inner SNI.

[1] https://www.openssl.org/docs/manmaster/man3/X509_VERIFY_PARAM_set1_host.html

[Neimhin]

Relevant code:

Here is what I think is the erroneous code. I believe the intention in to check that servername matches the hostname for ctx2, but notice that ctx2 is never reverenced in this snippet. This code is just setting some fields on the X509_VERIFY_PARAM object, but not actually running any certificate verification.

            int mrv;
            X509_VERIFY_PARAM *vpm = NULL;

            BIO_printf(p->biodebug,
                       "ssl_ech_servername_cb: TLS servername: %s.\n",
                       servername);
            BIO_printf(p->biodebug,
                       "ssl_ech_servername_cb: Cert servername: %s.\n",
                       p->servername);
            vpm = X509_VERIFY_PARAM_new();
            if (vpm == NULL)
                return SSL_TLSEXT_ERR_NOACK;
            mrv = X509_VERIFY_PARAM_set1_host(vpm, servername,
                                              strlen(servername));
            X509_VERIFY_PARAM_free(vpm);
            if (mrv == 1) {

Here are the definitions that gives our value for mrv:

int X509_VERIFY_PARAM_set1_host(X509_VERIFY_PARAM *param,
                                const char *name, size_t namelen)
{
    return int_x509_param_set_hosts(param, SET_HOST, name, namelen);
}

I think this is just creating a stack of hosts to be checked at some point, but not checking them now: (In our case this will pretty much always return 1 (unless we run out of memory).

static int int_x509_param_set_hosts(X509_VERIFY_PARAM *vpm, int mode,
                                    const char *name, size_t namelen)
{
    char *copy;

    /*
     * Refuse names with embedded NUL bytes, except perhaps as final byte.
     * XXX: Do we need to push an error onto the error stack?
     */
    if (namelen == 0 || name == NULL)
        namelen = name ? strlen(name) : 0;
    else if (name != NULL
             && memchr(name, '\0', namelen > 1 ? namelen - 1 : namelen) != NULL)
        return 0;
    if (namelen > 0 && name[namelen - 1] == '\0')
        --namelen;

    if (mode == SET_HOST) {
        sk_OPENSSL_STRING_pop_free(vpm->hosts, str_free);
        vpm->hosts = NULL;
    }
    if (name == NULL || namelen == 0)
        return 1;

    copy = OPENSSL_strndup(name, namelen);
    if (copy == NULL)
        return 0;

    if (vpm->hosts == NULL &&
        (vpm->hosts = sk_OPENSSL_STRING_new_null()) == NULL) {
        OPENSSL_free(copy);
        return 0;
    }

    if (!sk_OPENSSL_STRING_push(vpm->hosts, copy)) {
        OPENSSL_free(copy);
        if (sk_OPENSSL_STRING_num(vpm->hosts) == 0) {
            sk_OPENSSL_STRING_free(vpm->hosts);
            vpm->hosts = NULL;
        }
        return 0;
    }

    return 1;
}

[Neimhin]

While we're here, is it necessary to cryptographically verify the inner SNI against the ctx2 certificate? Isn't the ctx2 certificate trusted, so can't we do a cheaper check (e.g. strcmp)?

[Neimhin]

Reproduction:

Follow instructions for localhost ECH server/client in https://github.com/defo-project/ech-dev-utils/blob/main/howtos/localhost-tests.md, (I created ~/code/openssl/cadir and ~/code/openssl/echconfig.pem), where example.com is the outer server and foo.example.com is the inner server.

Server:

$ bash ../ech-dev-utils/scripts/echsvr.sh
Running ../ech-dev-utils/scripts/echsvr.sh at 20240609-003726
Not forcing HRR
Using key pair from /home/neimhin/code/openssl/echconfig.pem
GET /index.html HTTP/1.1
Connection: keep-alive
Host: foo.example.com

GET /index.html HTTP/1.1
Connection: keep-alive
Host: wrong.example.com

Clients (first with inner SNI as foo.example.com then with wrong.example.com) both return certificate for the inner server (foo.example.com), which is wrong, we should see the main server certificate (example.com) when we pass -H wrong.example.com).

~/code/openssl$ LD_LIBRARY_PATH=. $HOME/code/ech-dev-utils/scripts/echcli.sh -s localhost -H foo.example.com -p 8443 -P echconfig.pem -f index.html -d --wait 0 | grep DNS
                DNS:*.foo.example.com, DNS:foo.example.com
~/code/openssl$ LD_LIBRARY_PATH=. $HOME/code/ech-dev-utils/scripts/echcli.sh -s localhost -H wrong.example.com -p 8443 -P echconfig.pem -f index.html -d --wait 0 | grep DNS
                DNS:*.foo.example.com, DNS:foo.example.com

Other details:

~/code/openssl$ git rev-parse HEAD
b163c13441ae99c02a7094f305d2d4076b113669
~/code/openssl$ git status
On branch ECH-draft-13c
Your branch is up to date with 'origin/ECH-draft-13c'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        .github/workflows/test-ech.yml
        bin/
        cadir/
        commit-info/
        echconfig.pem
        go/
        scratch
        tmp/

nothing added to commit but untracked files present (use "git add" to track)