Bug#828236: Bug#844160: marked as done (apache2-dev should depend on libssl1.0-dev)

[Stefan Fritsch]

On Monday, 14 November 2016 05:03:45 CET 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.

The dependency on  libssl1.0-dev has now been moved from apache2-dev to the 
new apache2-ssl-dev package. Therefore the openssl build-dependency of php 
should now be decoupled again from apache2.

Cheers,
Stefan

Bug#828236: 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, , pubkey);
> +nid = OBJ_obj2nid(algor->algorithm);

I'm not sure what you're saying?


Kurt

Bug#828236: Bug#844160: marked as done (apache2-dev should depend on libssl1.0-dev)

[Adrian Bunk]

On Mon, Nov 14, 2016 at 08:44:35AM +0100, Ondřej Surý wrote:
> On Mon, Nov 14, 2016, at 08:21, Adrian Bunk wrote:
> > 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 reviewed the patch Kurt has provided and I don't see any strong reason
> > > why anything should break.
> > >...
> > 
> > Can you guarantee that rdeps of Apache can use 1.0.2 in stretch when 
> > Apache itself uses 1.1?
> 
> Why?

How do you fix rdeps of Apache that are not compiling or working
stable with 1.1?

We are already inside the stretch freeze.

Many packages are not even compiling with 1.1

And run-time testing with 1.1 has not even begun for most packages.
Did you confirm that the Apache code you reviewed actually works
flawlessly? Runtime bugs like #843988 are possible.

> > That is the most important question here.
> 
> No, I think the question is:
> 
> Can we migrate (or drop) all rdeps to 1.0.2?

The answer is "yes, we can migrate all".

All OpenSSL-using packages in unstable were compiled with and using 
1.0.2 until libssl-dev switched from 1.0.2 to 1.1 just 2 weeks ago.

Even today 80% of all libssl dependencies in unstable are still
against libssl1.0.2 (1.1 binNMUs have not yet been done).

> > This is what my "mod_ssl_openssl.h and the comment in #828330"
> > was referring to.
> > 
> > The dual 1.0.2/1.1 setup for stretch can only work when any set of 
> > packages in the archive that needs the same OpenSSL version stays
> > at 1.0.2 unless *all* packages in this set are compiling and working
> > fine with 1.1
> 
> The *set* you are talking probably cover whole archive, since the
> Build-Depends of PHP are quite vast and here are the Build-Depends
> of Build-Depends:
> 
> (This is from stretch, not from unstable)
> apache2-dev libssl-dev (>= 0.9.8m)
> libc-client2007e-dev libssl-dev
> libcurl4-openssl-dev libssl-dev
> libevent-dev libssl-dev
> libkrb5-dev libssl-dev
> libpq-dev libssl-dev
> libsasl2-dev libssl-dev
> libsnmp-dev libssl-dev (>> 0.9.8)
> 
> Greping just Depends: on -dev packages is slightly more optimistic:
> 
> apache2-dev libssl-dev (<< 1.1)
> libc-client2007e-dev libssl-dev
> libpq-dev libssl-dev
> libsnmp-dev libssl-dev
> 
> But ultimately I am afraid that libssl dependencies are so entagled
> that it will cover all archive.

"all archive" is not correct, but more packages in stretch using
1.0.2 than 1.1 sounds realistic.

> > And since the OpenSSL version used is part of the libcurl3 ABI
> > (see #844018 for details), using 1.1 in stretch is anyway not
> > really an option for Apache/PHP in stretch.
> 
> What you are really saying is that using OpenSSL 1.1 is generally
> not an option for stretch.

It is not a realistic option for larger groups of packages that have to 
use the same OpenSSL version.

It was clear before the OpenSSL default changed that using only 1.1 in 
stretch would not be realistic.

It was known before the OpenSSL default changed that problems like with 
libcurl3 could happen.

Considering that we are already in the freeze, for any OpenSSL 1.1 
related problem without a simple and fast solution the only option
within the stretch release schedule is to move packages back to 1.0.2

> Cheers,

cu
Adrian

-- 

   "Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
   "Only a promise," Lao Er said.
   Pearl S. Buck - Dragon Seed

Bug#828236: Bug#844160: marked as done (apache2-dev should depend on libssl1.0-dev)

[Ondřej Surý]

On Mon, Nov 14, 2016, at 08:21, Adrian Bunk wrote:
> 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 reviewed the patch Kurt has provided and I don't see any strong reason
> > why anything should break.
> >...
> 
> Can you guarantee that rdeps of Apache can use 1.0.2 in stretch when 
> Apache itself uses 1.1?

Why?

> That is the most important question here.

No, I think the question is:

Can we migrate (or drop) all rdeps to 1.0.2?

> This is what my "mod_ssl_openssl.h and the comment in #828330"
> was referring to.
> 
> The dual 1.0.2/1.1 setup for stretch can only work when any set of 
> packages in the archive that needs the same OpenSSL version stays
> at 1.0.2 unless *all* packages in this set are compiling and working
> fine with 1.1

The *set* you are talking probably cover whole archive, since the
Build-Depends of PHP are quite vast and here are the Build-Depends
of Build-Depends:

(This is from stretch, not from unstable)
apache2-dev libssl-dev (>= 0.9.8m)
libc-client2007e-dev libssl-dev
libcurl4-openssl-dev libssl-dev
libevent-dev libssl-dev
libkrb5-dev libssl-dev
libpq-dev libssl-dev
libsasl2-dev libssl-dev
libsnmp-dev libssl-dev (>> 0.9.8)

Greping just Depends: on -dev packages is slightly more optimistic:

apache2-dev libssl-dev (<< 1.1)
libc-client2007e-dev libssl-dev
libpq-dev libssl-dev
libsnmp-dev libssl-dev

But ultimately I am afraid that libssl dependencies are so entagled
that it will cover all archive.

> And since the OpenSSL version used is part of the libcurl3 ABI
> (see #844018 for details), using 1.1 in stretch is anyway not
> really an option for Apache/PHP in stretch.

What you are really saying is that using OpenSSL 1.1 is generally
not an option for stretch.

Cheers,
-- 
Ondřej Surý 
Knot DNS (https://www.knot-dns.cz/) – a high-performance DNS server
Knot Resolver (https://www.knot-resolver.cz/) – secure, privacy-aware,
fast DNS(SEC) resolver
Vše pro chleba (https://vseprochleba.cz) – Mouky ze mlýna a potřeby pro
pečení chleba všeho druhu

Bug#828236: Bug#844160: marked as done (apache2-dev should depend on libssl1.0-dev)

[Ondřej Surý]

> 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 reviewed the patch Kurt has provided and I don't see any strong reason
why anything should break.

~~~

ssl_engine_io_init() is called but return value is not checked although
it might obviously fail as the function can return 0 on failure.

~~~

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...

~~~

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);

or adding:
+SRP_user_pwd_free(u);

I think this should be in separate patch.

~~~

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) {

~~~

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, , pubkey);
+nid = OBJ_obj2nid(algor->algorithm);

and here:

@@ -398,7 +403,9 @@ static int stapling_check_response(serve
 
 if (bio) {
 int n;
-if ((i2a_ASN1_INTEGER(bio, cinf->cid->serialNumber) !=
-1) &&
+ASN1_INTEGER *serial;
+   OCSP_id_get0_info(NULL, NULL, NULL, , cinf->cid);
+if ((i2a_ASN1_INTEGER(bio, serial) != -1) &&
 ((n = BIO_read(bio, snum, sizeof snum - 1)) > 0))
 snum[n] = '\0';
 BIO_free(bio);

Cheers,
-- 
Ondřej Surý 
Knot DNS (https://www.knot-dns.cz/) – a high-performance DNS server
Knot Resolver (https://www.knot-resolver.cz/) – secure, privacy-aware,
fast DNS(SEC) resolver
Vše pro chleba (https://vseprochleba.cz) – Mouky ze mlýna a potřeby pro
pečení chleba všeho druhu

On Sun, Nov 13, 2016, at 13:36, Debian Bug Tracking System wrote:
> Your message dated Sun, 13 Nov 2016 12:34:03 +
> with message-id 
> and subject line Bug#844160: fixed in apache2 2.4.23-7
> has caused the Debian Bug report #844160,
> regarding apache2-dev should depend on libssl1.0-dev
> to be marked as done.
> 
> This means that you claim that the problem has been dealt with.
> If this is not the case it is now your responsibility to reopen the
> Bug report if necessary, and/or fix the problem forthwith.
> 
> (NB: If you are a system administrator and have no idea what this
> message is talking about, this may indicate a serious mail system
> misconfiguration somewhere. Please contact ow...@bugs.debian.org
> immediately.)
> 
> 
> -- 
> 844160: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=844160
> Debian Bug Tracking System
> Contact ow...@bugs.debian.org with problems
> Email had 2 attachments:
> + apache2-dev should depend on libssl1.0-dev
>   3k (message/rfc822)
> + Bug#844160: fixed in apache2 2.4.23-7
>   8k (message/rfc822)