[PATCH v2 2/2] secur32: Validate output buffer size in schan_InitializeSecurityContextW().

[Hans Leidekker]

From: Connor McAdams <cmcadams(a)codeweavers.com>

v2: Remove redundant braces, merge with tests.

Signed-off-by: Connor McAdams <cmcadams(a)codeweavers.com>
Signed-off-by: Hans Leidekker <hans(a)codeweavers.com>
---
 dlls/secur32/schannel.c       | 12 +++++-
 dlls/secur32/tests/schannel.c | 73 ++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
index 6b699cccce1..141a191c7c6 100644
--- a/dlls/secur32/schannel.c
+++ b/dlls/secur32/schannel.c
@@ -709,7 +709,7 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextW(
     SecBuffer *buffer;
     SecBuffer alloc_buffer = { 0 };
     struct handshake_params params;
-    int idx;
+    int idx, i;
 
     TRACE("%p %p %s 0x%08x %d %d %p %d %p %p %p %p\n", phCredential, phContext,
      debugstr_w(pszTargetName), fContextReq, Reserved1, TargetDataRep, pInput,
@@ -724,6 +724,16 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextW(
         ptsExpiry->HighPart = 0;
     }
 
+    if (!pOutput || !pOutput->cBuffers) return SEC_E_INVALID_TOKEN;
+    for (i = 0; i < pOutput->cBuffers; i++)
+    {
+        ULONG type = pOutput->pBuffers[i].BufferType;
+
+        if (type != SECBUFFER_TOKEN && type != SECBUFFER_ALERT) continue;
+        if (!pOutput->pBuffers[i].cbBuffer && !(fContextReq & ISC_REQ_ALLOCATE_MEMORY))
+            return SEC_E_INSUFFICIENT_MEMORY;
+    }
+
     if (!phContext)
     {
         ULONG_PTR handle;
diff --git a/dlls/secur32/tests/schannel.c b/dlls/secur32/tests/schannel.c
index f72d71a3af3..6c15addf2fc 100644
--- a/dlls/secur32/tests/schannel.c
+++ b/dlls/secur32/tests/schannel.c
@@ -582,6 +582,12 @@ static void init_buffers(SecBufferDesc *desc, unsigned count, unsigned size)
     desc->pBuffers[0].pvBuffer = HeapAlloc(GetProcessHeap(), 0, size);
 }
 
+static void init_sec_buffer(SecBuffer *sec_buf, ULONG count, void *buf)
+{
+    sec_buf->cbBuffer = count;
+    sec_buf->pvBuffer = buf;
+}
+
 static void reset_buffers(SecBufferDesc *desc)
 {
     unsigned i;
@@ -642,6 +648,67 @@ static int receive_data(SOCKET sock, SecBuffer *buf)
     return received;
 }
 
+static void test_context_output_buffer_size(DWORD protocol, DWORD flags, ULONG ctxt_flags_req)
+{
+    SCHANNEL_CRED cred;
+    CredHandle cred_handle;
+    CtxtHandle context;
+    SECURITY_STATUS status;
+    SecBuffer in_buffer = {0, SECBUFFER_EMPTY, NULL};
+    SecBufferDesc in_buffers  = {SECBUFFER_VERSION, 1, &in_buffer};
+    SecBufferDesc out_buffers;
+    unsigned buf_size = 8192;
+    void *buf, *buf2;
+    ULONG attrs;
+    int i;
+
+    init_cred(&cred);
+    cred.grbitEnabledProtocols = protocol;
+    cred.dwFlags = flags;
+    status = AcquireCredentialsHandleA(NULL, (SEC_CHAR *)UNISP_NAME_A, SECPKG_CRED_OUTBOUND, NULL,
+        &cred, NULL, NULL, &cred_handle, NULL);
+    ok( status == SEC_E_OK, "got %08x\n", status );
+    if (status != SEC_E_OK) return;
+
+    init_buffers(&out_buffers, 4, buf_size);
+    out_buffers.pBuffers[0].BufferType = SECBUFFER_TOKEN;
+    buf = out_buffers.pBuffers[0].pvBuffer;
+    buf2 = out_buffers.pBuffers[1].pvBuffer = HeapAlloc(GetProcessHeap(), 0, buf_size);
+    for (i = 0; i < 2; i++)
+    {
+        SecBuffer *buffer = !i ? &out_buffers.pBuffers[0] : &out_buffers.pBuffers[1];
+
+        init_sec_buffer(&out_buffers.pBuffers[0], buf_size, buf);
+        if (i) buffer->BufferType = SECBUFFER_ALERT;
+
+        buffer->cbBuffer = 0;
+        status = InitializeSecurityContextA(&cred_handle, NULL, (SEC_CHAR *)"localhost", ctxt_flags_req,
+                0, 0, &in_buffers, 0, &context, &out_buffers, &attrs, NULL);
+        ok(status == SEC_E_INSUFFICIENT_MEMORY, "%d: Expected SEC_E_INSUFFICIENT_MEMORY, got %08x\n", i, status);
+
+        if (i) init_sec_buffer(&out_buffers.pBuffers[0], buf_size, NULL);
+        init_sec_buffer(buffer, 0, NULL);
+        status = InitializeSecurityContextA(&cred_handle, NULL, (SEC_CHAR *)"localhost",
+                ctxt_flags_req | ISC_REQ_ALLOCATE_MEMORY, 0, 0, &in_buffers, 0, &context, &out_buffers, &attrs, NULL);
+        ok(status == SEC_I_CONTINUE_NEEDED, "%d: Expected SEC_I_CONTINUE_NEEDED, got %08x\n", i, status);
+        if (i) FreeContextBuffer(out_buffers.pBuffers[0].pvBuffer);
+        FreeContextBuffer(buffer->pvBuffer);
+        DeleteSecurityContext(&context);
+
+        if (i) init_sec_buffer(&out_buffers.pBuffers[0], buf_size, buf);
+        init_sec_buffer(buffer, buf_size, !i ? buf : buf2);
+        status = InitializeSecurityContextA(&cred_handle, NULL, (SEC_CHAR *)"localhost", ctxt_flags_req,
+                0, 0, &in_buffers, 0, &context, &out_buffers, &attrs, NULL);
+        ok(status == SEC_I_CONTINUE_NEEDED, "%d: Expected SEC_I_CONTINUE_NEEDED, got %08x\n", i, status);
+        if (i) todo_wine ok(!buffer->cbBuffer, "Expected SECBUFFER_ALERT buffer to be empty\n");
+        DeleteSecurityContext(&context);
+    }
+
+    HeapFree(GetProcessHeap(), 0, buf2);
+    free_buffers(&out_buffers);
+    FreeCredentialsHandle(&cred_handle);
+}
+
 static void test_InitializeSecurityContext(void)
 {
     SCHANNEL_CRED cred;
@@ -974,6 +1041,9 @@ static void test_communication(void)
         return;
     }
 
+    test_context_output_buffer_size(SP_PROT_TLS1_CLIENT, SCH_CRED_NO_DEFAULT_CREDS|SCH_CRED_MANUAL_CRED_VALIDATION,
+            ISC_REQ_CONFIDENTIALITY|ISC_REQ_STREAM);
+
     /* Create a socket and connect to test.winehq.org */
     if ((sock = create_ssl_socket( "test.winehq.org" )) == -1) return;
 
@@ -1023,7 +1093,6 @@ todo_wine
     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_INSUFFICIENT_MEMORY || status == SEC_E_INVALID_TOKEN,
        "Expected SEC_E_INSUFFICIENT_MEMORY or SEC_E_INVALID_TOKEN, got %08x\n", status);
     ok(buffers[0].pBuffers[0].cbBuffer == 0, "Output buffer size was not set to 0.\n");
@@ -1031,7 +1100,6 @@ todo_wine
     status = InitializeSecurityContextA(&cred_handle, NULL, (SEC_CHAR *)"localhost",
             ISC_REQ_CONFIDENTIALITY|ISC_REQ_STREAM,
             0, 0, NULL, 0, &context, NULL, &attrs, NULL);
-todo_wine
     ok(status == SEC_E_INVALID_TOKEN, "Expected SEC_E_INVALID_TOKEN, got %08x\n", status);
 
     buffers[0].pBuffers[0].cbBuffer = buf_size;
@@ -1571,6 +1639,7 @@ static void test_dtls(void)
 
     flags_req = ISC_REQ_MANUAL_CRED_VALIDATION | ISC_REQ_EXTENDED_ERROR | ISC_REQ_DATAGRAM | ISC_REQ_USE_SUPPLIED_CREDS |
                 ISC_REQ_CONFIDENTIALITY | ISC_REQ_SEQUENCE_DETECT | ISC_REQ_REPLAY_DETECT;
+    test_context_output_buffer_size(SP_PROT_DTLS_CLIENT | SP_PROT_DTLS1_2_CLIENT, SCH_CRED_NO_DEFAULT_CREDS, flags_req);
 
     init_buffers( &buffers[0], 1, 128 );
     buffers[0].pBuffers[0].BufferType = SECBUFFER_DTLS_MTU;
-- 
2.30.2

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=106048

Your paranoid android.


=== w1064 (32 bit report) ===

secur32:
schannel.c:1442: Test failed: got 00090317
schannel.c:1452: Test failed: DecryptMessage failed: 80090317

=== w1064 (64 bit report) ===

secur32:
schannel.c:1442: Test failed: got 00090317
schannel.c:1452: Test failed: DecryptMessage failed: 80090317