Schannel: Properly handle request for certificate

Certain servers, like smtp.live.com, will send a request for a
certificate even though they don't require one. In Schannel this
manifests as a warning/info status (SEC_I_INCOMPLETE_CREDENTIALS).
In the cases where it's not needed we should suppress the warning and
try to connect anyway, which is done by calling
InitializeSecurityContext again when we get the status.

Pick-to: 5.15
Change-Id: I3c48140f2949d8557251a49a2b66946da9395736
Reviewed-by: Joshua GPBeta <studiocghibli@gmail.com>
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

src/network/ssl/qsslsocket_schannel.cpp

@@ -923,56 +923,71 @@ bool QSslSocketBackendPrivate::performHandshake()
     if (intermediateBuffer.isEmpty())
         return true; // no data, will fail
 
-    SecBuffer inputBuffers[2];
+    SecBuffer outBuffers[3] = {};
-    inputBuffers[0] = createSecBuffer(intermediateBuffer, SECBUFFER_TOKEN);
+    const auto freeOutBuffers = [&outBuffers]() {
-    inputBuffers[1] = createSecBuffer(nullptr, 0, SECBUFFER_EMPTY);
-    SecBufferDesc inputBufferDesc{
-        SECBUFFER_VERSION,
-        ARRAYSIZE(inputBuffers),
-        inputBuffers
-    };
-
-    SecBuffer outBuffers[3];
-    outBuffers[0] = createSecBuffer(nullptr, 0, SECBUFFER_TOKEN);
-    outBuffers[1] = createSecBuffer(nullptr, 0, SECBUFFER_ALERT);
-    outBuffers[2] = createSecBuffer(nullptr, 0, SECBUFFER_EMPTY);
-    auto freeBuffers = qScopeGuard([&outBuffers]() {
         for (auto i = 0ull; i < ARRAYSIZE(outBuffers); i++) {
             if (outBuffers[i].pvBuffer)
                 FreeContextBuffer(outBuffers[i].pvBuffer);
         }
-    });
-    SecBufferDesc outputBufferDesc{
-        SECBUFFER_VERSION,
-        ARRAYSIZE(outBuffers),
-        outBuffers
     };
+    const auto outBuffersGuard = qScopeGuard(freeOutBuffers);
+    // For this call to InitializeSecurityContext we may need to call it twice.
+    // In some cases us not having a certificate isn't actually an error, but just a request.
+    // With Schannel, to ignore this warning, we need to call InitializeSecurityContext again
+    // when we get SEC_I_INCOMPLETE_CREDENTIALS! As far as I can tell it's not documented anywhere.
+    // https://stackoverflow.com/a/47479968/2493610
+    SECURITY_STATUS status;
+    short attempts = 2;
+    do {
+        SecBuffer inputBuffers[2];
+        inputBuffers[0] = createSecBuffer(intermediateBuffer, SECBUFFER_TOKEN);
+        inputBuffers[1] = createSecBuffer(nullptr, 0, SECBUFFER_EMPTY);
+        SecBufferDesc inputBufferDesc{
+            SECBUFFER_VERSION,
+            ARRAYSIZE(inputBuffers),
+            inputBuffers
+        };
 
-    ULONG contextReq = getContextRequirements();
+        freeOutBuffers(); // free buffers from any previous attempt
-    TimeStamp expiry;
+        outBuffers[0] = createSecBuffer(nullptr, 0, SECBUFFER_TOKEN);
-    auto status = InitializeSecurityContext(&credentialHandle, // phCredential
+        outBuffers[1] = createSecBuffer(nullptr, 0, SECBUFFER_ALERT);
-                                            &contextHandle, // phContext
+        outBuffers[2] = createSecBuffer(nullptr, 0, SECBUFFER_EMPTY);
-                                            const_reinterpret_cast<SEC_WCHAR *>(targetName().utf16()), // pszTargetName
+        SecBufferDesc outputBufferDesc{
-                                            contextReq, // fContextReq
+            SECBUFFER_VERSION,
-                                            0, // Reserved1
+            ARRAYSIZE(outBuffers),
-                                            0, // TargetDataRep (unused)
+            outBuffers
-                                            &inputBufferDesc, // pInput
+        };
-                                            0, // Reserved2
+
-                                            nullptr, // phNewContext (we already have one)
+        ULONG contextReq = getContextRequirements();
-                                            &outputBufferDesc, // pOutput
+        TimeStamp expiry;
-                                            &contextAttributes, // pfContextAttr
+        status = InitializeSecurityContext(
-                                            &expiry // ptsExpiry
+                &credentialHandle, // phCredential
-    );
+                &contextHandle, // phContext
+                const_reinterpret_cast<SEC_WCHAR *>(targetName().utf16()), // pszTargetName
+                contextReq, // fContextReq
+                0, // Reserved1
+                0, // TargetDataRep (unused)
+                &inputBufferDesc, // pInput
+                0, // Reserved2
+                nullptr, // phNewContext (we already have one)
+                &outputBufferDesc, // pOutput
+                &contextAttributes, // pfContextAttr
+                &expiry // ptsExpiry
+        );
+
+        if (inputBuffers[1].BufferType == SECBUFFER_EXTRA) {
+            // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel
+            // inputBuffers[1].cbBuffer indicates the amount of bytes _NOT_ processed, the rest need
+            // to be stored.
+            retainExtraData(intermediateBuffer, inputBuffers[1]);
+        } else if (status != SEC_E_INCOMPLETE_MESSAGE) {
+            // Clear the buffer if we weren't asked for more data
+            intermediateBuffer.resize(0);
+        }
+
+        --attempts;
+    } while (status == SEC_I_INCOMPLETE_CREDENTIALS && attempts > 0);
 
-    if (inputBuffers[1].BufferType == SECBUFFER_EXTRA) {
-        // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel
-        // inputBuffers[1].cbBuffer indicates the amount of bytes _NOT_ processed, the rest need to
-        // be stored.
-        retainExtraData(intermediateBuffer, inputBuffers[1]);
-    } else if (status != SEC_E_INCOMPLETE_MESSAGE) {
-        // Clear the buffer if we weren't asked for more data
-        intermediateBuffer.resize(0);
-    }
     switch (status) {
     case SEC_E_OK:
         // Need to transmit a final token in the handshake if 'cbBuffer' is non-zero.

tests/auto/network/ssl/qsslsocket/tst_qsslsocket.cpp

@@ -3391,14 +3391,6 @@ void tst_QSslSocket::verifyClientCertificate()
     // check server socket
     QVERIFY(server.socket);
 
-#if QT_CONFIG(schannel)
-    // As additional info to the QEXPECT_FAIL below:
-    // This is because schannel treats it as an error (client side) if you don't have a certificate
-    // when asked for one.
-    QEXPECT_FAIL("NoCert:VerifyPeer",
-                 "The client disconnects first, which causes the event "
-                 "loop to quit before the server disconnects.", Continue);
-#endif
     QCOMPARE(server.socket->state(), expectedState);
     QCOMPARE(server.socket->isEncrypted(), works);