Introduced STATE_CLOSING for connections to prevent getting stuck in endless loop (see discussion in #3 for details)

Author: fhessel

https/HTTPSConnection.cpp

@@ -25,6 +25,7 @@ HTTPSConnection::HTTPSConnection(ResourceResolver * resResolver):
 	_defaultHeaders = NULL;
 	_isKeepAlive = false;
 	_lastTransmissionTS = millis();
+	_shutdownTS = 0;
 }
 
 HTTPSConnection::~HTTPSConnection() {
@@ -115,29 +116,53 @@ bool HTTPSConnection::isError() {
 void HTTPSConnection::closeConnection() {
 	// TODO: Call an event handler here, maybe?
 
-	// Tear down SSL
-	if (_ssl) {
-		// wait here so that SSL_shutdown returns 1
-		while(SSL_shutdown(_ssl) == 0) delay(1);
-		SSL_free(_ssl);
-		_ssl = NULL;
-	}
 
-	// Tear down the socket
-	if (_socket >= 0) {
-		HTTPS_DLOGHEX("[<--] Connection has been closed. fid = ", _socket);
-		close(_socket);
-		_socket = -1;
-		_addrLen = 0;
-	}
+	if (_connectionState != STATE_ERROR && _connectionState != STATE_CLOSED) {
 
-	if (_connectionState != STATE_ERROR) {
-		_connectionState = STATE_CLOSED;
-	}
+		// First call to closeConnection - set the timestamp to calculate the timeout later on
+		if (_connectionState != STATE_CLOSING) {
+			_shutdownTS = millis();
+		}
 
-	if (_httpHeaders != NULL) {
-		delete _httpHeaders;
-		_httpHeaders = NULL;
+		// Set the connection state to closing. We stay in closing as long as SSL has not been shutdown
+		// correctly
+		_connectionState = STATE_CLOSING;
+
+		// Try to tear down SSL
+		if (_ssl) {
+			if(SSL_shutdown(_ssl) == 0) {
+				// SSL_shutdown will return 1 as soon as the client answered with close notify
+				// This means we are safe to close the socket
+				SSL_free(_ssl);
+				_ssl = NULL;
+			} else if (_shutdownTS + HTTPS_SHUTDOWN_TIMEOUT < millis()) {
+				// The timeout has been hit, we force SSL shutdown now by freeing the context
+				SSL_free(_ssl);
+				_ssl = NULL;
+				HTTPS_DLOG("[ERR] SSL_shutdown did not receive close notification from the client");
+				_connectionState = STATE_ERROR;
+			}
+		}
+
+		// If SSL has been brought down, close the socket
+		if (!_ssl) {
+			// Tear down the socket
+			if (_socket >= 0) {
+				HTTPS_DLOGHEX("[<--] Connection has been closed. fid = ", _socket);
+				close(_socket);
+				_socket = -1;
+				_addrLen = 0;
+			}
+
+			if (_connectionState != STATE_ERROR) {
+				_connectionState = STATE_CLOSED;
+			}
+
+			if (_httpHeaders != NULL) {
+				delete _httpHeaders;
+				_httpHeaders = NULL;
+			}
+		}
 	}
 }
 
@@ -489,6 +514,9 @@ void HTTPSConnection::loop() {
 		case STATE_BODY_FINISHED: // Request is complete
 			closeConnection();
 			break;
+		case STATE_CLOSING: // As long as we are in closing state, we call closeConnection() again and wait for it to finish or timeout
+			closeConnection();
+			break;
 		case STATE_WEBSOCKET: // Do handling of the websocket
 
 			break;

https/HTTPSConnection.hpp

@@ -98,15 +98,20 @@ class HTTPSConnection : private ConnectionContext {
 	// Timestamp of the last transmission action
 	unsigned long _lastTransmissionTS;
 
+	// Timestamp of when the shutdown was started
+	unsigned long _shutdownTS;
+
 	// Internal state machine of the connection:
 	//
 	// O --- > STATE_UNDEFINED -- initialize() --> STATE_INITIAL -- get / http/1.1 --> STATE_REQUEST_FINISHED --.
 	//                     |                          |                                       |                 |
 	//                     |                          |                                       |                 | Host: ...\r\n
 	// STATE_ERROR <- on error-----------------------<---------------------------------------<                  | Foo: bar\r\n
-	//                                                |                                       |                 | \r\n
-	//              close()                           |                                       |                 | \r\n
-	// STATE_CLOSED <----- STATE_WEBSOCKET <-.        |                                       |                 |
+	// ^                                              |                                       |                 | \r\n
+	// | shutdown   .--> STATE_CLOSED                 |                                       |                 | \r\n
+	// | fails     |                                  |                                       |                 |
+	// |           | close()                          |                                       |                 |
+	// STATE_CLOSING <---- STATE_WEBSOCKET <-.        |                                       |                 |
 	//  ^                                    |        |                                       |                 |
 	//  `---------- close() ---------- STATE_BODY_FINISHED <-- Body received or GET -- STATE_HEADERS_FINISHED <-´
 	//
@@ -125,6 +130,8 @@ class HTTPSConnection : private ConnectionContext {
 		STATE_BODY_FINISHED,
 		// The connection is in websocket mode
 		STATE_WEBSOCKET,
+		// The connection is about to close (and waiting for the client to send close notify)
+		STATE_CLOSING,
 		// The connection has been closed
 		STATE_CLOSED,
 		// An error has occured

https/HTTPSServer.cpp

@@ -83,11 +83,23 @@ void HTTPSServer::stop() {
 		_running = false;
 
 		// Clean up the connections
-		for(int i = 0; i < _maxConnections; i++) {
-			if (_connections[i] != NULL) {
-				_connections[i]->closeConnection();
-				delete _connections[i];
+		bool hasOpenConnections = true;
+		while(hasOpenConnections) {
+			hasOpenConnections = false;
+			for(int i = 0; i < _maxConnections; i++) {
+				if (_connections[i] != NULL) {
+					_connections[i]->closeConnection();
+
+					// Check if closing succeeded. If not, we need to call the close function multiple times
+					// and wait for the client
+					if (_connections[i]->isClosed()) {
+						delete _connections[i];
+					} else {
+						hasOpenConnections = true;
+					}
+				}
 			}
+			delay(1);
 		}
 
 		// Close the actual server socket

https/HTTPSServerConstants.hpp

@@ -35,4 +35,8 @@
 // Timeout for an HTTPS connection without any transmission
 #define HTTPS_CONNECTION_TIMEOUT               20000
 
+// Timeout used to wait for shutdown of SSL connection (ms)
+// (time for the client to return notify close flag) - without it, truncation attacks might be possible
+#define HTTPS_SHUTDOWN_TIMEOUT                 5000
+
 #endif /* HTTPS_HTTPSSERVERCONSTANTS_HPP_ */