[squid-dev] [PATCH] TLS: Add support for EECDH

[Amos Jeffries]

Actually two more and you may want to test these after changing.


1) The 'dhfile' pointer must now never be freed. Since it is either a
pointer into tls_dh or eecdhCurve allocated memory.
 - It should simply be set to dhfile=NULL where it was free()'d, and now
also when the tls_dh and/or eecdhCurve memory is released.
 - I spotted the destructor safe_free(dhfile), maybe elsewhere as well.


2) In src/anyp/PortCfg.cc:
> @@ -227,8 +233,27 @@
>      contextMethod = SSLv23_server_method();
>  #endif
>  
> -    if (dhfile)
> -        dhParams.reset(Ssl::readDHParams(dhfile));
> +    const char *dhFile = dhfile; // backward compatibility for dhparams= configuration
> +
> +    if (tls_dh && tls_dh[0]) {
> +        safe_free(eecdhCurve);
> +        eecdhCurve = xstrdup(tls_dh);
> +        char *p = strchr(eecdhCurve, ':');
> +        if (p) {  // tls-dh=eecdhCurve:dhfile
> +            *p = '\0';
> +            dhFile = p+1;
> +        } else {  // tls-dh=dhfile is equivalent to tls-dh=:dhfile
> +            dhFile = tls_dh;
> +            eecdhCurve[0] = '\0';
> +        }
> +    }
> +
> +    if (dhFile && dhFile[0])
> +        dhParams.reset(Ssl::readDHParams(dhFile));
> +
> +    // an empty eecdhCurve means "do not use EECDH"
> +    if (eecdhCurve && !eecdhCurve[0])
> +        safe_free(eecdhCurve);
>  

The eecdhCurve is always left a nil-pointer unless its assigned, which
only happens inside that if() block.

So it seems that could be better written as just:

> +    const char *dhFile = dhfile; // backward compatibility for
dhparams= configuration
> +    safe_free(eecdhCurve); // clear any previous EECDH configuration
> +    if (tls_dh && *tls_dh) {
> +        eecdhCurve = xstrdup(tls_dh);
> +        char *p = strchr(eecdhCurve, ':');
> +        if (p) {  // tls-dh=eecdhCurve:dhfile
> +            *p = '\0';
> +            dhFile = p+1;
> +        } else {  // tls-dh=dhfile is equivalent to tls-dh=:dhfile
> +            dhFile = tls_dh;
> +            // an empty eecdhCurve means "do not use EECDH"
> +            safe_free(eecdhCurve);
> +        }
> +    }
> +
> +    if (dhFile && *dhFile)
> +        dhParams.reset(Ssl::readDHParams(dhFile));



Amos