[Pkg-openssl-devel] Bug#844160: marked as done (apache2-dev should depend on libssl1.0-dev)

[Kurt Roeckx]

On Mon, Nov 14, 2016 at 05:03:45AM +0100, Ondřej Surý wrote:
> > Looking at mod_ssl_openssl.h and the comment in #828330,
> > I'd suggest the change below to add a dependency on libssl1.0-dev
> > to apache2-dev.
> 
> And that exactly happens meaning that PHP 7.0 can no longer be built
> unless all it's build-depends (including PHP 7.0) and rdepends move to
> libssl1.0-dev as well.
> 
> So a nice deadlock, right? To be honest I would rather have a slightly
> less tested apache2 with OpenSSL 1.1.0 and iron out the bugs as we go
> than revert all the work I have done.

I think the most important users of SSL is actually HTTPS, so I
would really like to see the webservers move to it. So I would
like to do whatever is needed to get apache to use OpenSSL 1.1.0.

> This bit looks suspicious as it changes the existing behavior:
> 
> -            /* XXX: Should replace setting state with
> SSL_renegotiate(ssl);
> -             * However, this causes failures in perl-framework
> currently,
> -             * perhaps pre-test if we have already negotiated?
> -             */
> -#ifdef OPENSSL_NO_SSL_INTERN
> -            SSL_set_state(ssl, SSL_ST_ACCEPT);
> -#else
> -            ssl->state = SSL_ST_ACCEPT;
> -#endif
> +            /* XXX: Why is this done twice? */
> +            SSL_renegotiate(ssl);
> +            /* XXX: Return value ignored, uses SSL_get_state instead?
> */
> 
> but it might be correct...

I don't understand what the old code is trying to do. But
apache shouldn't be touching the state machine like that. If
SSL_renegotiate() is not enough they should talk to the openssl
maintainers instead.

> ~~~
> 
> There also seem to be some changes unrelated to OpenSSL 1.1.0 as:
> 
> -        RAND_pseudo_bytes(iv, EVP_MAX_IV_LENGTH);
> +        /* XXX: Return value not checked. */
> +        RAND_bytes(iv, EVP_MAX_IV_LENGTH);

RAND_pseudo_bytes is deprecated.

> or adding:
> +        SRP_user_pwd_free(u);
> 
> I think this should be in separate patch.

This fixes a memory leak (see CVE-2016-0798)

> Kurt, can you confirm this doesn't change behavior of the code?
> 
> -    else if (cert->valid && X509_check_issued(cert,cert) == X509_V_OK)
> {
> +    else if (X509_check_issued(cert, cert) == X509_V_OK) {

That cert->valid check doesn't even make sense. It's checking that
that this self signed certificate chains back to one of the root
CAs, and if so it skips the ocsp check. cert->valid obviously isn't
set in that case (unless it's the root CA). If it's self signed,
it's correct to just skip it.

This is about checking client authentication. And if you added the
client certificate directly to the root store, valid would have
been set and you'd skip the OCSP check. I'm not sure that you want
to do that cert check in that case.

It's use of X509_STORE_CTX_get_current_cert() also doesn't make
any sense. It's asking which certificate failed the validation,
which should be none in the normal case, in which case the OCSP
check is now skipped.

> 
> Wrong ws here:
> 
> -        nid = OBJ_obj2nid((ASN1_OBJECT
> *)(xs->cert_info->key->algor->algorithm));
> +        X509_PUBKEY *pubkey = X509_get_X509_PUBKEY(xs);
> +       X509_ALGOR *algor;
> +       X509_PUBKEY_get0_param(NULL, NULL, NULL, &algor, pubkey);
> +        nid = OBJ_obj2nid(algor->algorithm);

I'm not sure what you're saying?


Kurt