From 97b9af1519ad3809a7900f0ac1f5710a439c87c2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Fri, 6 Sep 2019 13:51:37 +0200
Subject: Schannel: unbreak renegotiation (and likely gracious shutdown)

The reason it wasn't working before was a couple of things:
1. Due to an extra 'else' it would not process the SEC_I_RENEGOTIATE
or SEC_I_CONTEXT_EXPIRED branch.
2. The peerCertVerified boolean was not only wrong, but also
broke renegotiation even if the 'else' wasn't there.

My previous attempt to fix it ended up being a noop, so:
Reverts e21fa577dde32849fdaa744f30ad3b23d63b7214

Change-Id: Ifbad55d4bb066b7566bb88cead48e329cbd574f9
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>
---
 src/network/ssl/qsslsocket_schannel.cpp | 19 ++++---------------
 src/network/ssl/qsslsocket_schannel_p.h |  1 -
 2 files changed, 4 insertions(+), 16 deletions(-)

(limited to 'src/network/ssl')

diff --git a/src/network/ssl/qsslsocket_schannel.cpp b/src/network/ssl/qsslsocket_schannel.cpp
index 88f66ac4ea..37705b03a1 100644
--- a/src/network/ssl/qsslsocket_schannel.cpp
+++ b/src/network/ssl/qsslsocket_schannel.cpp
@@ -1069,7 +1069,6 @@ bool QSslSocketBackendPrivate::verifyHandshake()
     }
 
     schannelState = SchannelState::Done;
-    peerCertVerified = true;
     return true;
 }
 
@@ -1152,7 +1151,6 @@ void QSslSocketBackendPrivate::reset()
 
     connectionEncrypted = false;
     shutdown = false;
-    peerCertVerified = false;
     renegotiating = false;
 }
 
@@ -1315,7 +1313,9 @@ void QSslSocketBackendPrivate::transmit()
 #endif
                     intermediateBuffer = ciphertext.right(int(dataBuffer[3].cbBuffer));
                 }
-            } else if (status == SEC_E_INCOMPLETE_MESSAGE) {
+            }
+
+            if (status == SEC_E_INCOMPLETE_MESSAGE) {
                 // Need more data before we can decrypt.. to the buffer it goes!
 #ifdef QSSLSOCKET_DEBUG
                 qCDebug(lcSsl, "We didn't have enough data to decrypt anything, will try again!");
@@ -1361,17 +1361,6 @@ void QSslSocketBackendPrivate::transmit()
                 schannelState = SchannelState::Renegotiate;
                 renegotiating = true;
 
-                if (dataBuffer[3].BufferType == SECBUFFER_EXTRA) {
-                    // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel
-                    // dataBuffer[3].cbBuffer indicates the amount of bytes _NOT_ processed,
-                    // the rest need to be stored.
-#ifdef QSSLSOCKET_DEBUG
-                    qCDebug(lcSsl) << "We've got excess data, moving it to the intermediate buffer:"
-                                   << dataBuffer[3].cbBuffer << "bytes";
-#endif
-                    intermediateBuffer = ciphertext.right(int(dataBuffer[3].cbBuffer));
-                }
-
                 // We need to call 'continueHandshake' or else there's no guarantee it ever gets called
                 continueHandshake();
                 break;
@@ -1537,7 +1526,7 @@ void QSslSocketBackendPrivate::continueHandshake()
     case SchannelState::VerifyHandshake:
         // if we're in shutdown or renegotiating then we might not need to verify
         // (since we already did)
-        if (!peerCertVerified && !verifyHandshake()) {
+        if (!verifyHandshake()) {
             shutdown = true; // Skip sending shutdown alert
             q->abort(); // We don't want to send buffered data
             disconnectFromHost();
diff --git a/src/network/ssl/qsslsocket_schannel_p.h b/src/network/ssl/qsslsocket_schannel_p.h
index 9879e2fc60..6ab200e1f9 100644
--- a/src/network/ssl/qsslsocket_schannel_p.h
+++ b/src/network/ssl/qsslsocket_schannel_p.h
@@ -147,7 +147,6 @@ private:
     ULONG contextAttributes = 0;
 
     bool renegotiating = false;
-    bool peerCertVerified = false;
 };
 
 QT_END_NAMESPACE
-- 
cgit v1.2.3


From e5e8f1d67ce5f74ccb345086667933266543fd51 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Fri, 6 Sep 2019 13:01:43 +0200
Subject: Schannel: handle SEC_E_INCOMPLETE_DATA in acceptContext

It's not a failure state, we just need more data. It is handled properly
in other functions.

Change-Id: I9450a78c71a3f4fe9506a7a79de6efa2db08697c
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>
---
 src/network/ssl/qsslsocket_schannel.cpp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

(limited to 'src/network/ssl')

diff --git a/src/network/ssl/qsslsocket_schannel.cpp b/src/network/ssl/qsslsocket_schannel.cpp
index 37705b03a1..339ecf4da2 100644
--- a/src/network/ssl/qsslsocket_schannel.cpp
+++ b/src/network/ssl/qsslsocket_schannel.cpp
@@ -828,12 +828,17 @@ bool QSslSocketBackendPrivate::acceptContext()
             &expiry // ptsTimeStamp
     );
 
+    if (status == SEC_E_INCOMPLETE_MESSAGE) {
+        // Need more data
+        return true;
+    }
+
     if (inBuffers[1].BufferType == SECBUFFER_EXTRA) {
         // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel
         // inBuffers[1].cbBuffer indicates the amount of bytes _NOT_ processed, the rest need to
         // be stored.
         intermediateBuffer = intermediateBuffer.right(int(inBuffers[1].cbBuffer));
-    } else if (status != SEC_E_INCOMPLETE_MESSAGE) {
+    } else { /* No 'extra' data, message not incomplete */
         intermediateBuffer.clear();
     }
 
-- 
cgit v1.2.3