Topic: Imprecise error handling of ProcessReply()
Hi all,
I am using wolfSSL embedded ssl to provide SSL capabilities for a custom HTTP server using nonblocking i/o.
When doing some fuzzing tests against my server I noticed that CySSL_accept() always returns with -1 and the SSL error set to WANT_READ if I send random garbage (in my tests actually a plain text HTTP request message) to the socket.
I would've expected a return value of -1 but an SSL error similar to libopenssl's "SSL_R_HTTP_REQUEST".
When tracing the code in wolfSSL I pinpointed the problem to the fallback code in ProcessReply() which attempts to interpret the client data as SSLv2 client-hello record if the first 5 bytes do not match a SSLv3/TLS signature.
If a client sends e.g. a plaintext GET request to a wolfSSL server socket, the b0 and b1 variables will contain the chars 'G' and 'E' which results in a ssl->curSize of 18245 bytes. With such a high curSize the subsequent GetInputData() in the "runProcessOldClientHello" case will likely starve the socket and return with "WANT_READ".
Of course the application code calling wolfSSL_accept() can handle this accordingly and treat the failure as retry and eventual rely on the client socket returning EOF but it would be nice if it where possible to back out early when such a condition occurs.
Below is a patch which is meant as discussion starting point for improving the accept error handling in this case.
The change will make ProcessReply() read the 3rd byte of the alleged SSLv2 client hello record and check if it is indeed 0x01 (enum value client_hello). If the third byte is not 0x01, it will make ProcessReply() return with UNKNOWN_HANDSHAKE_TYPE which is propagated by wolfSSL_accept() - that will allow calling application code to properly detect the failure condition and spare one further unneeded retry.
Thanks for your attention and looking forward to any comments.
Regards, Jow
diff --git a/src/internal.c b/src/internal.c
index 58c42d1..9f35d6b 100644
--- a/src/internal.c
+++ b/src/internal.c
@@ -3094,7 +3094,7 @@ int ProcessReply(CYASSL* ssl)
int ret = 0, type, readSz;
word32 startIdx = 0;
#ifndef NO_CYASSL_SERVER
- byte b0, b1;
+ byte b0, b1, b2;
#endif
#ifdef CYASSL_DTLS
int used;
@@ -3144,6 +3144,15 @@ int ProcessReply(CYASSL* ssl)
b1 =
ssl->buffers.inputBuffer.buffer[ssl->buffers.inputBuffer.idx++];
ssl->curSize = ((b0 & 0x7f) << 8) | b1;
+
+ /* see if the message is of type client_hello - if not, reject */
+ b2 =
+ ssl->buffers.inputBuffer.buffer[ssl->buffers.inputBuffer.idx];
+
+ if ( b2 != client_hello ) {
+ ssl->curSize = 0;
+ return UNKNOWN_HANDSHAKE_TYPE;
+ }
}
else {
ssl->options.processReply = getRecordLayerHeader;