[PATCH 0/1] MR10916: secur32: Return SEC_E_INVALID_TOKEN for invalid TLS record headers

[Ivan Lyugaev (＠valy)]

I encountered an issue with establishing an encrypted connection to the mail server in Outlook. I managed to find out that Outlook tries various connection setup options — in particular, it sends a ClientHello to server port 587, and in response receives an SMTP banner, which in the establish_context function (https://gitlab.winehq.org/wine/wine/-/blame/master/dlls/secur32/schannel.c?r...) is mistakenly interpreted as TLS and returns a SEC_E_INCOMPLETE_MESSAGE error. This causes further reading of data from the server → an error and connection break.

I managed to fix it by adding validation of the first bytes of the buffer to match the TLS header.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10916

[PATCH 1/1] secur32: Return SEC_E_INVALID_TOKEN for invalid TLS record headers

[Ivan Lyugaev]

From: Ivan Lyugaev <[e-mail]>

---
 dlls/secur32/schannel.c       | 16 ++++++++++++++++
 dlls/secur32/tests/schannel.c |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
index 261d0606244..89e4a861887 100644
--- a/dlls/secur32/schannel.c
+++ b/dlls/secur32/schannel.c
@@ -841,6 +841,10 @@ static void dump_buffer_desc(SecBufferDesc *desc)
 
 #define HEADER_SIZE_TLS  5
 #define HEADER_SIZE_DTLS 13
+#define TLS_CONTENT_TYPE_CHANGE_CIPHER_SPEC 20
+#define TLS_CONTENT_TYPE_APPLICATION_DATA   23
+#define TLS_RECORD_TYPE_OFFSET          0
+#define TLS_RECORD_VERSION_MAJOR_OFFSET 1
 
 static inline SIZE_T read_record_size(const BYTE *buf, SIZE_T header_size)
 {
@@ -852,6 +856,11 @@ static inline BOOL is_dtls_context(const struct schan_context *ctx)
     return ctx->header_size == HEADER_SIZE_DTLS;
 }
 
+static inline BOOL is_tls_record_header(BYTE type, BYTE version)
+{
+    return type >= TLS_CONTENT_TYPE_CHANGE_CIPHER_SPEC && type <= TLS_CONTENT_TYPE_APPLICATION_DATA && version == 3;
+}
+
 static void fill_missing_sec_buffer(SecBufferDesc *input, DWORD size)
 {
     int idx = schan_find_sec_buffer_idx(input, 0, SECBUFFER_EMPTY);
@@ -1017,6 +1026,13 @@ static SECURITY_STATUS establish_context(
                 return SEC_E_INCOMPLETE_MESSAGE;
             }
 
+            if (!is_dtls_context(ctx) && !is_tls_record_header(ptr[TLS_RECORD_TYPE_OFFSET], ptr[TLS_RECORD_VERSION_MAJOR_OFFSET]))
+            {
+                WARN("Invalid TLS record header: %02x %02x %02x %02x %02x.\n",
+                     ptr[0], ptr[1], ptr[2], ptr[3], ptr[4]);
+                return SEC_E_INVALID_TOKEN;
+            }
+
             while (buffer->cbBuffer >= expected_size + ctx->header_size)
             {
                 record_size = ctx->header_size + read_record_size(ptr, ctx->header_size);
diff --git a/dlls/secur32/tests/schannel.c b/dlls/secur32/tests/schannel.c
index 3c4f938f556..92dcfe618c7 100644
--- a/dlls/secur32/tests/schannel.c
+++ b/dlls/secur32/tests/schannel.c
@@ -1040,7 +1040,7 @@ static void test_communication(void)
     status = InitializeSecurityContextA(&cred_handle, &context, (SEC_CHAR *)"localhost",
             ISC_REQ_CONFIDENTIALITY|ISC_REQ_STREAM,
             0, 0, &buffers[1], 0, NULL, &buffers[0], &attrs, NULL);
-    todo_wine
+
     ok(status == SEC_E_INVALID_TOKEN, "Expected SEC_E_INVALID_TOKEN, got %08lx\n", status);
     todo_wine
     ok(buffers[0].pBuffers[0].cbBuffer == 0, "Output buffer size was not set to 0.\n");
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/10916

[Alfred Agrell (＠Alcaro)]

Third time I hit that draft button by accident... sorry about that, still not used to that recent gitlab redesign.

Looks good to me.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10916#note_140048

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/secur32/schannel.c:
> return SEC_E_INCOMPLETE_MESSAGE;
>              }
>
> +            if (!is_dtls_context(ctx) && !is_tls_record_header(ptr[TLS_RECORD_TYPE_OFFSET], ptr[TLS_RECORD_VERSION_MAJOR_OFFSET]))

Passing the ptr to is_tls_record_header() and handling the offsets there would be cleaner IMO.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10916#note_140051