New EOF detection breaks session resumption

@kleest

We want to do session resumption.

If we follow the documentation of SSL_shutdown:

In case the application wants to be able to resume the session, it is recommended to do a complete shutdown procedure (bidirectional close_notify alerts).
[...]
The shutdown is not yet finished: the close_notify was sent but the peer did not send it back yet. Call SSL_read() to do a bidirectional shutdown.

Consider the following pseudocode:

// obtain SSL_SESSION via callback during SSL_connect
...

// bidirectional shutdown
if (SSL_shutdown(ssl) == 0)
    SSL_read(ssl, NULL, 0);

// try to resume session via new ssl object
...

Due to new EOF detection (db943f4), the call to SSL_read fails here and invalidates the session (SSLfatal) breaking resumption.

Version: OpenSSL 1.1.1e and current master (7e06a67)
Proposed fix: If bidirectional shutdown is supposed to work correctly, a zero-length EOF in SSL_read should not be a fatal error.

/cc @kleest

Both peers should always send the close notify (call SSL_shutdown), but then have 2 options: 1) Wait to receive the peer's close notify 2) Because of the protocol, know that the only thing that can come now is the close notify and not wait for it. My understanding is that you try to do 1), but the server is failing to do it's part and is not sending the close notify. At the SSL layer, we don't know that the communication is done or not, so if we don't receice the close notify, but do detect an EOF, we need to return an error. If we do not return an error, you might be vulnerable to a truncation attack. Because you don't know that the other side is going to do 1) or 2), you should always send the close notify. So I think you now have those options: - Fix the server to send the close notify - Don't wait for the close notify

It's worth reminding that openssl s_client/s_server do not provide a relevant example. I agree that the broken one in this pair is s_server, but it's worth fixing it.

@beldmit

@beldmit This is not just a s_server problem. We found broken resumption using nginx as well.

@kroeckx While you are right that the server should send a close notify, many servers don't. So "fix the server" is not something we can do. I did a quick test against the Alexa Top 50 and the rate of successful resumptions using bidirectional shutdown went from 70% using 1.1.1d to 30% using 1.1.1e. If SSL_read is going to signal an error, at least don't invalidate the session while doing so.

As a general note, this kind of breaking change is not something I would have expected in a bugfix release.

I did not expect software to ignore that we already returned an error. Looking at some code, they seem to have special cased that error and decided to just ignore the error, possibly opening themselves for a truncation attack. I think it was actually well known that HTTPS is broken in this regard, that many servers do not properly close the connection while they should. The only recommendation I have for that is to not call SSL_read() when you know that you have already received everything, or that you ignore it in that case you have received everything. If you don't know that everything has been received, and you don't get a close notify, you really should get an error. I'm currently unsure if we should revert this or not. There probably is code where because of the protocol it's clear that everything was send or not, and such code worked properly before if they had a special case for that error. But all other code that just ignores it should get fixed instead. As I understand it, HTTP 1.1 is not always such a protocol. So I guess I'm waiting to see examples of non-broken code that is affected by the change.

We can consider revering it in 1.1.1 but keeping it in master/3.0. But I'm currently still unsure what to do, we want to encourage people to fix this. And it seems non-trivial for code that actually knows all data was received and can't avoid calling SSL_read() to ignore the error.

I'm currently unsure if we should revert this or not.

This seems like the best solution for the near future until we can find a better way to handle the EOF issue.

And it seems non-trivial for code that actually knows all data was received and can't avoid calling SSL_read() to ignore the error.

We would love to ignore the error, except that SSL_read now calls SSLfatal, which invalidates the session attached to it and makes it not resumable. We have no direct control over the validity of the session and cannot predict if a server will send a close notify or not.

If this problem persists, we can only switch to unidirectional shutdown and hope that the issue gets fixed in a future release.

I'm currently unsure if we should revert this or not.

We effectively changed an error from SSL_ERROR_SYSCALL to SSL_ERROR_SSL. This is correct because our own documentation says to go check errno if you get the former because there's been a system level IO error, or go check the OpenSSL error stack with the latter because there's been some TLS level problem. The problem really is at the TLS level, and errno is 0 in this case so returning SSL_ERROR_SYSCALL is just incorrect behaviour. Since it was a bug fix it met the requirements for backport to 1.1.1

Since we are swapping one type of error for another, one might have expected it to not be too big a deal. Nonetheless, I had a suspicion when I opened the 1.1.1 backport PR this that some code might find this change unexpected. For that reason I requested multiple approvals but still the consensus seemed to be at that time that we should still backport.

As always with bug fixes, one person's bug fix is another person's breaking change, if they were relying on the buggy behaviour. The problem now is having made the decision to backport does it just confuse things further to revert it...only to reintroduce it again in master? I'm also unsure as to the correct answer to this.

I think, at first, it should be fixed in openssl code itself - at least s_client/s_server pair should be a relevant example.

A compromise could be to not break the session resumption if unexpected EOF is obtained. Is there any security relevant reason why the session should be invalidated in this case?

I've got some private comments from the Nginx team. They are very unhappy, especially speaking about broken session resumption. If they provide some more details, I'll resend them to the project list.

http://mailman.nginx.org/pipermail/nginx/2020-March/059175.html

A compromise could be to not break the session resumption if unexpected EOF is obtained. Is there any security relevant reason why the session should be invalidated in this case?

I think we should do that.

http://mailman.nginx.org/pipermail/nginx/2020-March/059175.html

Note that #11381 talks about that.

A compromise could be to not break the session resumption if unexpected EOF is obtained

That would fix at least this issue, but the other EOF issues persist.

Why are multiple tickets assigned to 1 session, and not just a new session pet ticket?

The API for clients to perform resumption is to resume the session associated with an SSL object. As tickets arrive, the session is mutated to hold the most recent ticket.

TLS 1.3 supports multiple tickets. It suggests sending as many tickets as parallel connections are expected.

TLS 1.3 supports multiple tickets. It suggests sending as many tickets as parallel connections are expected.

Yes, I know this. However many OpenSSL client applications that reuse sessions, just save the final session object at the conclusion of a connection, and use it to resume future sessions.

A more sophisticated application can implement the new session callback, and this IIRC is called for each received ticket, and the application may then be able to save away multiple tickets (one per "snapshot" of the session) for resumption. Quoting SSL_CTX_sess_set_new_cb(3):

    Note that in TLSv1.3, sessions are established after the main handshake
    has completed. The server decides when to send the client the session
    information and this may occur some time after the end of the handshake
    (or not at all). This means that applications should expect the
    new_session_cb() function to be invoked during the handshake (for <=
    TLSv1.2) or after the handshake (for TLSv1.3). It is also possible in
    TLSv1.3 for multiple sessions to be established with a single
    connection. In these case the new_session_cb() function will be invoked
    multiple times.

Of course the application then needs a sophisticated means of storing multiple single-use sessions for the same peer, instead of the traditional (with TLS 1.0–1.2) single multi-use session.

The Postfix SMTP server overrides default OpenSSL session ticket properties to issues only one ticket per full handshake, and to allow session ticket reuse. These are a better fit for SMTP, than the browser-oriented single-use model.

As tickets arrive, the session is mutated to hold the most recent ticket.

Sorry, I've not been following this discussion, so I've not really looked at the context. But we don't mutate the existing session when a new ticket arrives. We duplicate the existing one and mutate the duplicate, then update the SSL object to reference the newly duplicated session. See:

openssl/ssl/statem/statem_clnt.c

Lines 2717 to 2745 in 1977c00

    
               /* 
        
                * Sessions must be immutable once they go into the session cache. Otherwise 
        
                * we can get multi-thread problems. Therefore we don't "update" sessions, 
        
                * we replace them with a duplicate. In TLSv1.3 we need to do this every 
        
                * time a NewSessionTicket arrives because those messages arrive 
        
                * post-handshake and the session may have already gone into the session 
        
                * cache. 
        
                */ 
        
               if (SSL_CONNECTION_IS_TLS13(s) || s->session->session_id_length > 0) { 
        
                   SSL_SESSION *new_sess; 
        
                   /* 
        
                    * We reused an existing session, so we need to replace it with a new 
        
                    * one 
        
                    */ 
        
                   if ((new_sess = ssl_session_dup(s->session, 0)) == 0) { 
        
                       SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_SSL_LIB); 
        
                       goto err; 
        
                   } 
        
                   if ((s->session_ctx->session_cache_mode & SSL_SESS_CACHE_CLIENT) != 0 
        
                           && !SSL_CONNECTION_IS_TLS13(s)) { 
        
                       /* 
        
                        * In TLSv1.2 and below the arrival of a new tickets signals that 
        
                        * any old ticket we were using is now out of date, so we remove the 
        
                        * old session from the cache. We carry on if this fails 
        
                        */ 
        
                       SSL_CTX_remove_session(s->session_ctx, s->session); 
        
                   }

Yes, a new session is constructed for each new ticket, but in the end the SSL handle references just the most recently created session, so applications that save the session at the end of a connection end up using only the last ticket to arrive. Also the same with applications with only one slot in their cache per logical remote server. It does non-trivial sophistication to consume multiple sessions, but that's really a tangential discussion. The main thing to note is that we should not invalidate the (current) session on EOF sans close_notify from the peer (and may not be able to do that for sessions from earlier tickets if any, assuming the application has a suitable multi-slot cache via the new session cb).

[XrdTls] Enable SSL_OP_IGNORE_UNEXPECTED_EOF option if available

fix: Redirects from the RDAP server were causing an SSL EOF error

[XrdTls] Enable SSL_OP_IGNORE_UNEXPECTED_EOF option if available

openssl: revert EOF detection change in 1.1.1

p5-IO-Socket-SSL: update to 2.068.

openssl: revert EOF detection change in 1.1.1

p5-IO-Socket-SSL: update to 2.068.

Navigation Menu

Repository navigation

New EOF detection breaks session resumption #11378

98 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	/*
	* Sessions must be immutable once they go into the session cache. Otherwise
	* we can get multi-thread problems. Therefore we don't "update" sessions,
	* we replace them with a duplicate. In TLSv1.3 we need to do this every
	* time a NewSessionTicket arrives because those messages arrive
	* post-handshake and the session may have already gone into the session
	* cache.
	*/
	if (SSL_CONNECTION_IS_TLS13(s) \|\| s->session->session_id_length > 0) {
	SSL_SESSION *new_sess;

	/*
	* We reused an existing session, so we need to replace it with a new
	* one
	*/
	if ((new_sess = ssl_session_dup(s->session, 0)) == 0) {
	SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_SSL_LIB);
	goto err;
	}

	if ((s->session_ctx->session_cache_mode & SSL_SESS_CACHE_CLIENT) != 0
	&& !SSL_CONNECTION_IS_TLS13(s)) {
	/*
	* In TLSv1.2 and below the arrival of a new tickets signals that
	* any old ticket we were using is now out of date, so we remove the
	* old session from the cache. We carry on if this fails
	*/
	SSL_CTX_remove_session(s->session_ctx, s->session);
	}

New EOF detection breaks session resumption #11378

Description

Activity

kroeckx commented on Mar 22, 2020

beldmit commented on Mar 22, 2020

roussosalex commented on Mar 22, 2020

kroeckx commented on Mar 22, 2020

kroeckx commented on Mar 23, 2020

roussosalex commented on Mar 23, 2020

mattcaswell commented on Mar 23, 2020

beldmit commented on Mar 23, 2020

t8m commented on Mar 23, 2020

beldmit commented on Mar 23, 2020

beldmit commented on Mar 23, 2020

kroeckx commented on Mar 23, 2020

kroeckx commented on Mar 23, 2020

roussosalex commented on Mar 23, 2020

98 remaining items

vdukhovni commented on Jun 10, 2024

kroeckx commented on Jun 10, 2024

vdukhovni commented on Jun 10, 2024

mattcaswell commented on Jun 14, 2024

vdukhovni commented on Jun 14, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions