Control: tags -1 + patch fixed-upstream
On Wed, 02 May 2018 at 17:19:20 +0100, Simon McVittie wrote:
> * https://github.com/openSUSE/osc/issues/398
>
> """
> - m2crypto 0.29 does no SSL_free(...) (which is fixed in 0.30)
> (that's why this bug is not triggered with m2crypto 0.29)
> - there's a "bug" in ssl_update_cache cache in openssl 1.1.0h (in
> short: the session is not put in the session cache...)
> - osc currently relies on the fact that the session is in the session
> cache (or more precisely, that there are at least two references to
> the SSL_SESSION), which is, of course, a bug in osc
> """
The osc crash appears to be fixable by the patch that was applied
upstream (see attached Disable-ssl-session-resumption.patch). If I'm
understanding the commit message correctly, strictly speaking this was
already an osc bug (it made bad assumptions about session caching),
but the regression in openssl changed its impact from "might crash in
rare cases" to "crashes every time".
> * https://github.com/openssl/openssl/pull/5967
>
> """
> Commit d316cdc introduced some extra
> checks into the session-cache update procedure, intended to prevent
> the caching of sessions whose resumption would lead to a handshake
> failure, since if the server is authenticating the client, there needs to
> be an application-set "session id context" to match up to the authentication
> context. While that change is effective for its stated purpose, there
> was also some collatoral damage introduced along with the fix -- clients
> that set SSL_VERIFY_PEER are not expected to set an sid_ctx, and so
> their usage of session caching was erroneously denied.
>
> Fix the scope of the original commit by limiting it to only acting
> when the SSL is a server SSL.
> """
Sorry for the delay in testing this.
Applying openssl commit c84f7d9251446bf76c179bf5da31f25944f4b975
(and reverting osc to the version that crashes) seems
to be another way to address this. See attached
c84f7d9251446bf76c179bf5da31f25944f4b975.patch.
Regards,
smcv
From: Marcus Huewe <suse-...@gmx.de>
Date: Tue, 8 May 2018 14:23:08 +0200
Subject: Disable ssl session resumption
The old code could potentially yield to a use-after-free situation,
which results in UB. For this, consider the following scenario, where
osc performs several HTTPS requests (assumption: the server supports
ssl session resumption):
- HTTPS Request 1:
* a new SSL *s connection is established, which also creates a new
SSL_SESSION *ss => ss->references == 1
* once the handshake is done, the ss is put into the session cache
(see ssl_update_cache) => ss->references == 2
- osc saves the session ss in a class variable
- s is SSL_free()d, which calls SSL_SESSION_free => ss->references == 1
- HTTPS Request 2:
* setup a new SSL *s connection that reuses the saved session ss
=> ss->references == 2
* once the handshake is done, ssl_update_cache is called, which is a
NOP, because s->hit == 1 (that is, the session was resumed)
* osc saves the session ss in a class variable
* s is SSL_free()d, which calls SSL_SESSION_free => ss->references == 1
...
> 2 hours later (see tls1_default_timeout)
...
- HTTPS Request 256:
* setup a new SSL *s connection that reuses the saved session ss
=> ss->references == 2
* once the handshake is done, ssl_update_cache is called, but is
_no_ NOP anymore
* ssl_update_cache flushes the session cache (this is done every
255/256 (depending on the way we count) connections) => ss is
SSL_SESSION_free()d => ss->references == 1
* osc saves the session ss in a class variable
* s is SSL_free()d, which calls SSL_SESSION_free:
since ss->references == 1, ss is eventually free()d
- HTTPS Request 257:
* setup a new SSL *s connection that reuses the saved session ss
Since ss does not exist anymore, the remaining program execution is UB.
(Note: SSL_free(...) is _NOT_ called, if M2Crypto 0.29 is used.
M2Crypto 0.30 calls SSL_free(...) again.)
Due to a bug in OpenSSL_1_1_0h (see openssl commit 8e405776858) the
scenario from above can be triggered with exactly 2 HTTPS requests (the
SSL_SESSION is not cached, because we configured SSL_VERIFY_PEER, but
no sid_ctx was set). This is fixed in openssl commit c4fa1f7fc01.
In order to reliably reuse a session, we probably need to listen to the
session cache changes. Such callbacks could be registered via
SSL_CTX_sess_set_new_cb and/or SSL_CTX_sess_set_remove_cb, but both
functions are not provided by M2Crypto. Another idea is to directly utilize
the session cache, but this also has to be implemented in M2Crypto first.
Yet another approach is to retrieve the session via SSL_get1_session, which
increases the session's refcnt, but this also needs to be implemented in
M2Crypto first (if we choose to use this approach, we also have to make
sure that we eventually free the session manually...).
Fixes: #398 ("SIGSEGV on \"osc commit\"")
Origin: upstream, 0.162.2, commit:https://github.com/openSUSE/osc/commit/b730f880cfe85a8547f569355a21706f27ebfa78
Bug: https://github.com/openSUSE/osc/issues/398
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=895035
---
osc/oscssl.py | 6 ------
1 file changed, 6 deletions(-)
diff --git a/osc/oscssl.py b/osc/oscssl.py
index 7aa5a0d..186c98d 100644
--- a/osc/oscssl.py
+++ b/osc/oscssl.py
@@ -174,7 +174,6 @@ class mySSLContext(SSL.Context):
class myHTTPSHandler(M2Crypto.m2urllib2.HTTPSHandler):
handler_order = 499
- saved_session = None
def __init__(self, *args, **kwargs):
self.appname = kwargs.pop('appname', 'generic')
@@ -204,8 +203,6 @@ class myHTTPSHandler(M2Crypto.m2urllib2.HTTPSHandler):
selector = req.get_selector()
# End our change
h.set_debuglevel(self._debuglevel)
- if self.saved_session:
- h.set_session(self.saved_session)
headers = dict(req.headers)
headers.update(req.unredirected_hdrs)
@@ -218,9 +215,6 @@ class myHTTPSHandler(M2Crypto.m2urllib2.HTTPSHandler):
headers["Connection"] = "close"
try:
h.request(req.get_method(), selector, req.data, headers)
- s = h.get_session()
- if s:
- self.saved_session = s
r = h.getresponse()
except socket.error as err: # XXX what error?
err.filename = full_url
>From c84f7d9251446bf76c179bf5da31f25944f4b975 Mon Sep 17 00:00:00 2001
From: Benjamin Kaduk <bka...@akamai.com>
Date: Mon, 16 Apr 2018 07:32:02 -0500
Subject: [PATCH] Fix regression with session cache use by clients
Commit d316cdcf6d8d6934663278145fe0a8191e14a8c5 introduced some extra
checks into the session-cache update procedure, intended to prevent
the caching of sessions whose resumption would lead to a handshake
failure, since if the server is authenticating the client, there needs to
be an application-set "session id context" to match up to the authentication
context. While that change is effective for its stated purpose, there
was also some collatoral damage introduced along with the fix -- clients
that set SSL_VERIFY_PEER are not expected to set an sid_ctx, and so
their usage of session caching was erroneously denied.
Fix the scope of the original commit by limiting it to only acting
when the SSL is a server SSL.
---
ssl/ssl_lib.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index b1d78dc35ef..63a7f57025a 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3336,12 +3336,13 @@ void ssl_update_cache(SSL *s, int mode)
/*
* If sid_ctx_length is 0 there is no specific application context
* associated with this session, so when we try to resume it and
- * SSL_VERIFY_PEER is requested, we have no indication that this is
- * actually a session for the proper application context, and the
- * *handshake* will fail, not just the resumption attempt.
- * Do not cache these sessions that are not resumable.
+ * SSL_VERIFY_PEER is requested to verify the client identity, we have no
+ * indication that this is actually a session for the proper application
+ * context, and the *handshake* will fail, not just the resumption attempt.
+ * Do not cache (on the server) these sessions that are not resumable
+ * (clients can set SSL_VERIFY_PEER without needing a sid_ctx set).
*/
- if (s->session->sid_ctx_length == 0
+ if (s->server && s->session->sid_ctx_length == 0
&& (s->verify_mode & SSL_VERIFY_PEER) != 0)
return;
