Squid-Security-Audit

Buffer UnderRead in SSL CN Parsing

Squid does some parsing of domain names from SSL certificates when it is compiled with SSL. This parsing determines whether the certificate authenticates a response and the server it is arriving from.

The Issue

In order to match two domain names, Squid uses the function matchDomainName, which is able to match both FQDNs (Fully Qualified Domain Names), and subdomains. The important parts of this function can be summarized by:

matchDomainName(const char *h, const char *d, MatchDomainNameFlags flags)
{
    int dl;
    int hl;

    const bool hostIncludesSubdomains = (*h == '.');
    while ('.' == *h)
        ++h;

    hl = strlen(h);

    if (hl == 0)
        return -1;

    dl = strlen(d);

    /*
     * Start at the ends of the two strings and work towards the
     * beginning.
     */
    while (xtolower(h[--hl]) == xtolower(d[--dl])) {
        if (hl == 0 && dl == 0) {
[...]
}

The issue here is that the second domain’s length, const char *d, is not checked, before the while loop is run. More specifically, the issue is that if the strlen of d is zero, xtolower(d[--dl]) will read from d[-1]: i.e. a negative buffer, which is undefined behavior in C++.

There is one place within Squid’s code where this is possible. When Squid is compiled with SSL support (i.e. where it can encrypt/decrypt requests/responses rather than using a CONNECT tunnel), the function check_domain is used to check whether a request domain’s name is the same domain (or is equally valid) as in the certificate the server offers. Note: This code is only used when Squid is compiled with OpenSSL. This function is defined as the following:

static int check_domain( void *check_data, ASN1_STRING *cn_data)
{
    char cn[1024];
    const char *server = (const char *)check_data;

    if (cn_data->length == 0)
        return 1; // zero length cn, ignore

    if (cn_data->length > (int)sizeof(cn) - 1)
        return 1; //if does not fit our buffer just ignore

    char *s = reinterpret_cast<char*>(cn_data->data);
    char *d = cn;
    for (int i = 0; i < cn_data->length; ++i, ++d, ++s) {
        if (*s == '\0')
            return 1; // always a domain mismatch. contains 0x00
        *d = *s;
    }
    cn[cn_data->length] = '\0';
    debugs(83, 4, "Verifying server domain " << server << " to certificate name/subjectAltName " << cn);
    return matchDomainName(server, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains);
}

While zero-length domains are rejected before the matchDomainName function is ever called, if a certificate offers the CNAME as simply * (which would mean all domains), a pointer pointing to an empty/null character is passed to matchDomainName:

    return matchDomainName(server, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains);

since if cn is simply "*", cn+1 is "" (i.e. a null character).

The issue here is that reading from a negative buffer can either produce junk data, or can cause a crash; or is can do nothing. It is undefined behavior akin to a buffer overflow.