Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
On Fri, Jan 15, 2021 at 9:13 PM James McCoy wrote: > On Fri, Jan 15, 2021 at 08:36:22AM -0500, Justin Erenkrantz wrote: > > Sadly, my Debian sid box ran into other issues and is currently > inaccessible. > > > > I *think* that this would address the 1.3.x test issues, but 1.3.x > doesn't > > build on Mac OS for me for other reasons...so, let me know how it goes? > =) -- > > Success! > Great! I'd recommend just picking that up as a local patch for now and we'll work towards releasing a 1.4.0 with this and a bunch of other goodness in the coming weeks/months. >From a quick look at debian-devel-announce, it looks like the Bullseye freeze has just started ; so, probably by the time the unstable window opens again post-Bullseye, we'll be able to land a proper 1.4.0. Cheers. -- justin
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
On Fri, Jan 15, 2021 at 08:36:22AM -0500, Justin Erenkrantz wrote: > Sadly, my Debian sid box ran into other issues and is currently inaccessible. > > I *think* that this would address the 1.3.x test issues, but 1.3.x doesn't > build on Mac OS for me for other reasons...so, let me know how it goes? =) > -- Success! Thanks, -- James GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
Sadly, my Debian sid box ran into other issues and is currently
inaccessible.
I *think* that this would address the 1.3.x test issues, but 1.3.x doesn't
build on Mac OS for me for other reasons...so, let me know how it goes?
=) -- justin
Index: test/test_context.c
===
--- test/test_context.c (revision 1885525)
+++ test/test_context.c (working copy)
@@ -1138,7 +1138,7 @@
/* We expect an error from the certificate validation function. */
if (failures & expected_failures)
-return APR_SUCCESS;
+return APR_EGENERAL;
else
return SERF_ERROR_ISSUE_IN_TESTSUITE;
}
@@ -1206,8 +1206,8 @@
create_new_request(tb, _ctx[0], "GET", "/", 1);
-test_helper_run_requests_expect_ok(tc, tb, num_requests, handler_ctx,
- test_pool);
+test_helper_run_requests_expect_fail(tc, tb, num_requests, handler_ctx,
+ test_pool);
}
/* Set up the ssl context with the CA and root CA certificates needed for
@@ -1774,8 +1774,8 @@
create_new_request(tb, _ctx[0], "GET", "/", 1);
-test_helper_run_requests_expect_ok(tc, tb, num_requests, handler_ctx,
- test_pool);
+test_helper_run_requests_expect_fail(tc, tb, num_requests, handler_ctx,
+ test_pool);
}
/* Validate that the expired certificate is reported as failure in the
@@ -1820,8 +1820,8 @@
create_new_request(tb, _ctx[0], "GET", "/", 1);
-test_helper_run_requests_expect_ok(tc, tb, num_requests, handler_ctx,
- test_pool);
+test_helper_run_requests_expect_fail(tc, tb, num_requests, handler_ctx,
+ test_pool);
}
Index: test/test_serf.h
===
--- test/test_serf.h (revision 1885525)
+++ test/test_serf.h (working copy)
@@ -239,6 +239,12 @@
int num_requests,
handler_baton_t handler_ctx[],
apr_pool_t *pool);
+void
+test_helper_run_requests_expect_fail(CuTest *tc, test_baton_t *tb,
+ int num_requests,
+ handler_baton_t handler_ctx[],
+ apr_pool_t *pool);
+
serf_bucket_t* accept_response(serf_request_t *request,
serf_bucket_t *stream,
void *acceptor_baton,
Index: test/test_util.c
===
--- test/test_util.c (revision 1885525)
+++ test/test_util.c (working copy)
@@ -461,6 +461,19 @@
CuAssertIntEquals(tc, num_requests, tb->handled_requests->nelts);
}
+void
+test_helper_run_requests_expect_fail(CuTest *tc, test_baton_t *tb,
+ int num_requests,
+ handler_baton_t handler_ctx[],
+ apr_pool_t *pool)
+{
+apr_status_t status;
+
+status = test_helper_run_requests_no_check(tc, tb, num_requests,
+ handler_ctx, pool);
+CuAssertIntEquals(tc, APR_EGENERAL, status);
+}
+
serf_bucket_t* accept_response(serf_request_t *request,
serf_bucket_t *stream,
void *acceptor_baton,
On Thu, Jan 14, 2021 at 11:35 PM James McCoy wrote:
> Happy New Year!
>
> On Wed, Dec 30, 2020 at 10:39:28PM -0500, James McCoy wrote:
> > On Tue, Dec 29, 2020 at 02:35:11PM -0500, Justin Erenkrantz wrote:
> > > The OpenSSL devs intended this to be a breaking change - but it's not
> > > documented anywhere. Sigh.
> > >
> > > I've got a WIP patch against trunk that causes test_ssl to pass - see
> below.
> > > It also seems to work with OpenSSL 1.1.1h as well as OpenSSL 1.1.1i /
> > > 1.1.1-stable, AFAICT.
> > >
> > > James: can you please give it a try as well?
> >
> > Yes, I can confirm this fixes test_ssl_handshake on trunk. There's
> > enough difference between trunk and branches/1.3.x that it doesn't apply
> > cleanly there.
>
> Any chance you would be able to make a patch for 1.3.x? Although a 1.4
> release would be nice, it's a bit late in the Debian release cycle to
> upload a major new version.
>
> A targeted fix for the test suite would address the immediate issue,
> though.
>
> Cheers,
> --
> James
> GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
>
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
Happy New Year! On Wed, Dec 30, 2020 at 10:39:28PM -0500, James McCoy wrote: > On Tue, Dec 29, 2020 at 02:35:11PM -0500, Justin Erenkrantz wrote: > > The OpenSSL devs intended this to be a breaking change - but it's not > > documented anywhere. Sigh. > > > > I've got a WIP patch against trunk that causes test_ssl to pass - see > > below. > > It also seems to work with OpenSSL 1.1.1h as well as OpenSSL 1.1.1i / > > 1.1.1-stable, AFAICT. > > > > James: can you please give it a try as well? > > Yes, I can confirm this fixes test_ssl_handshake on trunk. There's > enough difference between trunk and branches/1.3.x that it doesn't apply > cleanly there. Any chance you would be able to make a patch for 1.3.x? Although a 1.4 release would be nice, it's a bit late in the Debian release cycle to upload a major new version. A targeted fix for the test suite would address the immediate issue, though. Cheers, -- James GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
On Tue, Dec 29, 2020 at 02:35:11PM -0500, Justin Erenkrantz wrote: > The OpenSSL devs intended this to be a breaking change - but it's not > documented anywhere. Sigh. > > I've got a WIP patch against trunk that causes test_ssl to pass - see below. > It also seems to work with OpenSSL 1.1.1h as well as OpenSSL 1.1.1i / > 1.1.1-stable, AFAICT. > > James: can you please give it a try as well? Yes, I can confirm this fixes test_ssl_handshake on trunk. There's enough difference between trunk and branches/1.3.x that it doesn't apply cleanly there. Cheers, -- James GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
The OpenSSL devs intended this to be a breaking change - but it's not documented anywhere. Sigh. I've got a WIP patch against trunk that causes test_ssl to pass - see below. It also seems to work with OpenSSL 1.1.1h as well as OpenSSL 1.1.1i / 1.1.1-stable, AFAICT. James: can you please give it a try as well? We've been on the threshold of releasing serf 1.4 for quite some time now...maybe we should just do that... If this looks reasonable, I'll try to clean this up and get it into trunk and 1.4.x. Cheers. -- justin Index: test/test_serf.h === --- test/test_serf.h (revision 1884847) +++ test/test_serf.h (working copy) @@ -296,6 +296,14 @@ handler_baton_t handler_ctx[], apr_pool_t *pool); +/* Helper function, runs the client and server context loops and validates + that errors were encountered. */ +void +run_client_and_mock_servers_loops_expect_fail(CuTest *tc, test_baton_t *tb, + int num_requests, + handler_baton_t handler_ctx[], + apr_pool_t *pool); + /* Logs a test suite error with its code location, and return status SERF_ERROR_ISSUE_IN_TESTSUITE. */ #define REPORT_TEST_SUITE_ERROR()\ Index: test/test_ssl.c === --- test/test_ssl.c (revision 1884847) +++ test/test_ssl.c (working copy) @@ -465,9 +465,10 @@ tb->result_flags |= TEST_RESULT_SERVERCERTCB_CALLED; +test__log(TEST_VERBOSE, __FILE__, "Cert failure received: %d ; expected failure mask: %d\n", failures, expected_failures); /* We expect an error from the certificate validation function. */ if (failures & expected_failures) -return APR_SUCCESS; +return APR_EGENERAL; else return REPORT_TEST_SUITE_ERROR(); } @@ -532,8 +533,8 @@ create_new_request(tb, _ctx[0], "GET", "/", 1); -run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests, -handler_ctx, tb->pool); +run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests, + handler_ctx, tb->pool); } /* Validate that connecting to a SSLv2 only server fails. */ @@ -1121,8 +1122,8 @@ create_new_request(tb, _ctx[0], "GET", "/", 1); -run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests, -handler_ctx, tb->pool); +run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests, + handler_ctx, tb->pool); CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED); } @@ -1165,8 +1166,8 @@ create_new_request(tb, _ctx[0], "GET", "/", 1); -run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests, -handler_ctx, tb->pool); +run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests, + handler_ctx, tb->pool); CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED); } @@ -2095,8 +2096,8 @@ create_new_request(tb, _ctx[0], "GET", "/", 1); -run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests, -handler_ctx, tb->pool); +run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests, + handler_ctx, tb->pool); CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED); } @@ -2138,8 +2139,8 @@ create_new_request(tb, _ctx[0], "GET", "/", 1); -run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests, -handler_ctx, tb->pool); +run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests, + handler_ctx, tb->pool); CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED); } @@ -2181,8 +2182,8 @@ create_new_request(tb, _ctx[0], "GET", "/", 1); -run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests, -handler_ctx, tb->pool); +run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests, + handler_ctx, tb->pool); CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED); } @@ -2310,8 +2311,8 @@ create_new_request(tb, _ctx[0], "GET", "/", 1); -run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests, -handler_ctx, tb->pool); +run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests, +
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
On Mon, Dec 28, 2020 at 5:00 PM Justin Erenkrantz wrote: > It's not clear to me if OpenSSL authors intended to make this breaking > change. On the serf side, we would need to think through what it would > mean to have our app callback return false upon failure in order to > short-circuit the check. > > I probably won't get a chance to open an upstream OpenSSL issue today (or > even tomorrow)... > I found the original issue where they changed the behavior and added a comment there: https://github.com/openssl/openssl/issues/11297 Cheers. -- justin
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
As an update, I've been able to triage this a bit further.
It's definitely that last noted change (erroring out on expired self-signed
root) that broke it. OpenSSL 1.1.1{g,h} are fine, but {i,-stable} are
not. Reverting just x509_vfy.c to what is in 1.1.1h causes the test to
pass.
In this test case, Serf receives 2 verify callbacks in test_ssl_handshake.
The first failing test case is to not have a known CA - so, we are
intentionally trying to trigger a verify failure. One of the app
callback received has the expected failure, the other doesn't. Serf
basically flags the second (success) as an unexpected pass.
2020-12-28T16:51:34.045142-05 test/test_ssl.c: Cert failure received: 4 ;
expected failure mask: 4
2020-12-28T16:51:34.045159-05 test/test_ssl.c: Cert failure received: 0 ;
expected failure mask: 4
The upstream issues/commits appear to be:
https://github.com/openssl/openssl/issues/13427
https://github.com/openssl/openssl/commit/3bed88a3970605a2ff817065f93b08e965d89e5f#diff-2a76d0a7ddc5ae2646a6c183270a7b4d5302d8491acb0af0dfbd70643efdf431
The key difference is almost certainly this change:
https://github.com/openssl/openssl/blob/OpenSSL_1_1_1h/crypto/x509/x509_vfy.c#L1754
---
return verify_cb_cert(ctx, xi, 0,
X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE);
---
https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/x509/x509_vfy.c#L1755
---
if (!verify_cb_cert(ctx, xi, 0,
X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE))
return 0;
xs = xi;
goto check_cert_time;
---
Up to 1.1.1h, OpenSSL would stop processing the certificate after sending
along the leaf error to the app callback. In -stable (1.1.1i+ and master),
if the app's callback doesn't return a failure, it will then proceed to the
date check portion (check_cert_time) - which then receives a successful
verification callback.
It's not clear to me if OpenSSL authors intended to make this breaking
change. On the serf side, we would need to think through what it would
mean to have our app callback return false upon failure in order to
short-circuit the check.
I probably won't get a chance to open an upstream OpenSSL issue today (or
even tomorrow)...
Cheers. -- justin
Index: test/test_ssl.c
===
--- test/test_ssl.c (revision 1884847)
+++ test/test_ssl.c (working copy)
@@ -465,6 +465,7 @@
tb->result_flags |= TEST_RESULT_SERVERCERTCB_CALLED;
+test__log(TEST_VERBOSE, __FILE__, "Cert failure received: %d ;
expected failure mask: %d\n", failures, expected_failures);
/* We expect an error from the certificate validation function. */
if (failures & expected_failures)
return APR_SUCCESS;
On Sun, Dec 27, 2020 at 11:22 AM James McCoy wrote:
> On Sun, Dec 27, 2020 at 10:46:24AM -0500, Justin Erenkrantz wrote:
> > Thanks. I expect that this might be due to the last change - erroring
> out on
> > an expired self-signed root cert. Though I thought we didn’t check in a
> root
> > cert for our test chain...could Debian’s packaging be including a cert
> for
> > testing?
>
> I use create_certs.py from trunk to re-generate the test certificates
> every build, otherwise I was running into time bombs with the certs
> expiring. Other than that, I don't do anything different than the
> normal test process.
>
> The Debian packaging does have some local patches[0] applied to address
> issues that have been fixed upstream but not yet released.
>
> [0]: https://sources.debian.org/patches/serf/1.3.9-8/
>
> > I will try to take a look this week with Debian sid...I assume it has
> 1.1.1i
> > already? — justin
>
> Yes, it does.
>
> Cheers,
> --
> James
> GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
>
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
On Sun, Dec 27, 2020 at 10:46:24AM -0500, Justin Erenkrantz wrote: > Thanks. I expect that this might be due to the last change - erroring out on > an expired self-signed root cert. Though I thought we didn’t check in a root > cert for our test chain...could Debian’s packaging be including a cert for > testing? I use create_certs.py from trunk to re-generate the test certificates every build, otherwise I was running into time bombs with the certs expiring. Other than that, I don't do anything different than the normal test process. The Debian packaging does have some local patches[0] applied to address issues that have been fixed upstream but not yet released. [0]: https://sources.debian.org/patches/serf/1.3.9-8/ > I will try to take a look this week with Debian sid...I assume it has 1.1.1i > already? — justin Yes, it does. Cheers, -- James GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
Thanks. I expect that this might be due to the last change - erroring out on an expired self-signed root cert. Though I thought we didn’t check in a root cert for our test chain...could Debian’s packaging be including a cert for testing? I will try to take a look this week with Debian sid...I assume it has 1.1.1i already? — justin On Sun, Dec 27, 2020 at 10:39 AM James McCoy wrote: > On Sat, Dec 26, 2020 at 11:09:41PM +0100, Lucas Nussbaum wrote: > > Source: serf > > Version: 1.3.9-8 > > [...] > > > Trailer-Test: f > > > ...F.. > > > > > > There was 1 failure: > > > 1) test_ssl_handshake: test/test_util.c:456: expected <0> but was > <120199> > > It looks like the change from libssl1.1 version 1.1.1h to 1.1.1i > regressed this test. > > The documented changes between these two releases are: > > Changes between 1.1.1h and 1.1.1i [8 Dec 2020] > > *) Fixed NULL pointer deref in the GENERAL_NAME_cmp function > This function could crash if both GENERAL_NAMEs contain an > EDIPARTYNAME. > If an attacker can control both items being compared then this could > lead > to a possible denial of service attack. OpenSSL itself uses the > GENERAL_NAME_cmp function for two purposes: > 1) Comparing CRL distribution point names between an available CRL > and a > CRL distribution point embedded in an X509 certificate > 2) When verifying that a timestamp response token signer matches the > timestamp authority name (exposed via the API functions > TS_RESP_verify_response and TS_RESP_verify_token) > (CVE-2020-1971) > [Matt Caswell] > > *) Add support for Apple Silicon M1 Macs with the darwin64-arm64-cc > target. > [Stuart Carnie] > > *) The security callback, which can be customised by application code, > supports > the security operation SSL_SECOP_TMP_DH. This is defined to take an > EVP_PKEY > in the "other" parameter. In most places this is what is passed. All > these > places occur server side. However there was one client side call of > this > security operation and it passed a DH object instead. This is > incorrect > according to the definition of SSL_SECOP_TMP_DH, and is inconsistent > with all > of the other locations. Therefore this client side call has been > changed to > pass an EVP_PKEY instead. > [Matt Caswell] > > *) In 1.1.1h, an expired trusted (root) certificate was not anymore > rejected > when validating a certificate path. This check is restored in 1.1.1i. > [David von Oheimb] > > The full diff is at > https://github.com/openssl/openssl/compare/OpenSSL_1_1_1h...OpenSSL_1_1_1i > > Cheers, > -- > James > GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB >
Bug#978353: serf: FTBFS: test_ssl_handshake fails with OpenSSL 1.1.1i
On Sat, Dec 26, 2020 at 11:09:41PM +0100, Lucas Nussbaum wrote: > Source: serf > Version: 1.3.9-8 > [...] > > Trailer-Test: f > > ...F.. > > > > There was 1 failure: > > 1) test_ssl_handshake: test/test_util.c:456: expected <0> but was <120199> It looks like the change from libssl1.1 version 1.1.1h to 1.1.1i regressed this test. The documented changes between these two releases are: Changes between 1.1.1h and 1.1.1i [8 Dec 2020] *) Fixed NULL pointer deref in the GENERAL_NAME_cmp function This function could crash if both GENERAL_NAMEs contain an EDIPARTYNAME. If an attacker can control both items being compared then this could lead to a possible denial of service attack. OpenSSL itself uses the GENERAL_NAME_cmp function for two purposes: 1) Comparing CRL distribution point names between an available CRL and a CRL distribution point embedded in an X509 certificate 2) When verifying that a timestamp response token signer matches the timestamp authority name (exposed via the API functions TS_RESP_verify_response and TS_RESP_verify_token) (CVE-2020-1971) [Matt Caswell] *) Add support for Apple Silicon M1 Macs with the darwin64-arm64-cc target. [Stuart Carnie] *) The security callback, which can be customised by application code, supports the security operation SSL_SECOP_TMP_DH. This is defined to take an EVP_PKEY in the "other" parameter. In most places this is what is passed. All these places occur server side. However there was one client side call of this security operation and it passed a DH object instead. This is incorrect according to the definition of SSL_SECOP_TMP_DH, and is inconsistent with all of the other locations. Therefore this client side call has been changed to pass an EVP_PKEY instead. [Matt Caswell] *) In 1.1.1h, an expired trusted (root) certificate was not anymore rejected when validating a certificate path. This check is restored in 1.1.1i. [David von Oheimb] The full diff is at https://github.com/openssl/openssl/compare/OpenSSL_1_1_1h...OpenSSL_1_1_1i Cheers, -- James GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
