Channel bindings

[jborean93]

This PR brings in support for Channel Binding Tokens through RFC5929 https://github.com/mongodb-labs/winkerberos/issues/16. I've manually tested it against a WinRM endpoint with the following value set.

Set-Item -Path "WSMan:\localhost\Service\Auth\CbtHardeningLevel" -Value Strict

Happy to change anything that is concerning with how it is implemented but it is designed to be as cross compatible with https://github.com/apple/ccs-pykerberos/pull/55.

[jborean93]

Hey @behackett I've made the changes requested, please let me know if I've missed anything as I probably have. I changed the name of the methods and kwargs for the step method based on what was requested in the ccs-pykerberos PR.

[jborean93]

Thanks very much for the feedback appreciate the time and I'll try and make the changes shortly. I am hoping to get a consensus with this PR and https://github.com/apple/ccs-pykerberos/pull/55 before merging either but hopefully the other one is nearing completion.

[jborean93]

@behackett changes have been made.

[behackett]

Wonderful. I'll run it through coverity, fix anything coverity complains about, and merge it. Thanks again!

[jborean93]

@behackett how did the coverity report go, is this a scan I can run myself or is it some propriety product you have access to?

[behackett]

Sorry for the late response. What version of Python did you use to develop the patch set? It won't build for me under Python 2.6 and 2.7, but does build under 3.3+. I'm guessing the issue has something to do with the Visual Studio 2008 compiler (necessary for Python 2.6 and 2.7) vs. the Visual Studio 2010 compiler (Python 3.3 and 3.4) or Visual Studio 2015 compiler (Python 3.5+).

PS C:\10gen\winkerberos> C:\Python27\python.exe .\setup.py build_ext -i
running build_ext
building 'winkerberos' extension
creating build
creating build\temp.win-amd64-2.7
creating build\temp.win-amd64-2.7\Release
creating build\temp.win-amd64-2.7\Release\src
c:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\BIN\amd64\cl.exe /c /nologo /Ox /MD /W3 /GS- /DNDEBUG -IC:\Python2
7\include -IC:\Python27\PC /Tcsrc/winkerberos.c /Fobuild\temp.win-amd64-2.7\Release\src/winkerberos.obj
winkerberos.c
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2143: syntax error : missing ')' before '*'
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2081: 'SecPkgContext_Bindings' : name in formal parameter list ill
egal
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2143: syntax error : missing '{' before '*'
c:\10gen\winkerberos\src\kerberos_sspi.h(57) : error C2059: syntax error : ')'
src/winkerberos.c(445) : error C2065: 'SecPkgContext_Bindings' : undeclared identifier
src/winkerberos.c(445) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(445) : error C2065: 'SecPkgContext_Bindings' : undeclared identifier
src/winkerberos.c(445) : error C2059: syntax error : ')'
src/winkerberos.c(447) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(447) : warning C4047: '!=' : 'int' differs in levels of indirection from 'void *'
src/winkerberos.c(448) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(448) : error C2223: left of '->Bindings' must point to struct/union
src/winkerberos.c(448) : error C2198: 'free' : too few arguments for call
src/winkerberos.c(449) : error C2065: 'context_bindings' : undeclared identifier
src/winkerberos.c(449) : warning C4022: 'free' : pointer mismatch for actual parameter 1
...more...

[behackett]

Before you ask, I'm building on Windows 7, which msdn claims is the minimum required Windows version.

https://msdn.microsoft.com/en-us/library/windows/desktop/dd919960(v=vs.85).aspx

[jborean93]

I swear I tested from python 2.6-3.6 manually but I'll get back to you on that as I don't have access to the original environment right now, shouldn't take too long to rebuild though.

[jborean93]

@behackett I've been able to compile it on all those Python versions but I'm getting some other errors which I will follow up on. I thought I tested it but it seems I didn't on the latest changes.

[behackett]

Which version of Windows are you using, and are you using this compiler?

[jborean93]

For 2.6 and 2.7 yes that is the one. I used this page https://wiki.python.org/moin/WindowsCompilers to see what other compilers to use and was able to compile 3.3-6 as well with their respective ones.

This is the Windows details I have

C:\Users\WINDOWS7>systeminfo | findstr /B /C:"OS Name" /C:"OS Version" OS Name: Microsoft Windows 7 Professional OS Version: 6.1.7601 Service Pack 1 Build 7601

This is a brand new Windows 7 image that has all the latest updates installed and setuptools/pip are all up to date. I am using a virtualenv though unlike your example where it seems like you are using the default installed Python.

[behackett]

Thanks. I'm using the compilers from Visual Studio 2008 (the actual IDE). I'll try again with the Python specific package and see what happens.

[jborean93]

I'm coming across a really weird issue (probably due to my faulty code). I can get it working on all Python versions by having the following diff

diff --git a/src/kerberos_sspi.c b/src/kerberos_sspi.c
index 948ea99..09ba2c3 100644
--- a/src/kerberos_sspi.c
+++ b/src/kerberos_sspi.c
@@ -357,7 +357,8 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
         status = AUTH_GSS_CONTINUE;
     }
 done:
-    if (inBufs[tokenBufferIndex].pvBuffer) {
+    if (inBufs[tokenBufferIndex].pvBuffer && inBufs[tokenBufferIndex].BufferType != SECBUFFER_EMPTY) {
+        printf("This is weirdly required\n");
         free(inBufs[tokenBufferIndex].pvBuffer);
     }
     if (outBufs[0].pvBuffer) **{**

For whatever reason it will fail with a 0xc0000374 STATUS_HEAP_CORRUPTION fault if the printf statement is not there but works with it in. This to me is an indication I've done something wrong somewhere but I cannot figure it out. Do you have any ideas?

[behackett]

I spun up a new Windows 7 VM using the Microsoft Python 2.7 compiler instead of Visual Studio 2008 and can build the project again. I'll update the build docs to require the right compiler after we get this merged. I assume at this point that the Python compiler just bundles a newer set of SSPI headers than Visual Studio 2008.

Sorry this is taking so long. A lot of other things at MongoDB are drawing my attention right now. I hope to be able to investigate that heap corruption later tonight.

[behackett]

What options are you using when you get STATUS_HEAP_CORRUPTION? I don't have an environment to test channel bindings, and the existing test suite runs without issue with your patch.

[jborean93]

I'm installing pywinrm and my forked version of requests-kerberos https://github.com/jborean93/requests-kerberos/tree/add-cbt and running the following script

from winrm.protocol import Protocol

p = Protocol(
    endpoint='https://SERVER.domain.local:5986/wsman',
    transport='kerberos',
    server_cert_validation='ignore')
shell_id = p.open_shell()
command_id = p.run_command(shell_id, 'ipconfig', ['/all'])
std_out, std_err, status_code = p.get_command_output(shell_id, command_id)
p.cleanup_command(shell_id, command_id)
p.close_shell(shell_id)

print("STDOUT")
print(std_out)
print("STDERR")
print(std_err)
print("RC")
print(status_code)

You will need a WinRM endpoint currently setup for this to work, if you want a quick and easy one this can do it for you on a host https://github.com/ansible/ansible/blob/devel/examples/scripts/ConfigureRemotingForAnsible.ps1

In it's current form this work with Python 2.6, 2.7 and 3.6, 3.3-3.5 are returning the STATUS_HEAP_CORRUPTION error.

[behackett]

I think I understand what the problem is. Your test program is always passing two SecBuffers to InitializeSecurityContext, even on the first call when there is no token from the server yet. In that first call the second SecBuffer is set to type SECBUFFER_EMPTY. According to the docs on msdn, that is apparently a placeholder type for output buffers, rather than an input (read only) type. That may or may not contribute to the problem. The real problem, I think, is that you are also incrementing inbuf.cBuffers in that case, so InitializeSecurityContext is trying to access the data in that "empty" SecBuffer. I bet you can solve the problem by not incrementing cBuffers for the empty buffer.

Also, the docs for InitializeSecurityContext claim that pInput must be NULL on first call, but you're passing channel bindings on first call. Does authentication fail if you don't pass channel bindings on the first call? It wouldn't surprise me if the msdn docs are misleading or flat out wrong.

[behackett]

Can you try this patch on top of your existing changes?

diff --git a/src/kerberos_sspi.c b/src/kerberos_sspi.c
index 948ea99..0ce4e4c 100644
--- a/src/kerberos_sspi.c
+++ b/src/kerberos_sspi.c
@@ -249,8 +249,8 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
     ULONG ignored;
     SECURITY_STATUS status = AUTH_GSS_CONTINUE;
     DWORD len;
-    int secBufferCount = 0;
-    int tokenBufferIndex = 0;
+    BOOL haveToken = FALSE;
+    INT tokenBufferIndex = 0;

     if (state->response != NULL) {
         free(state->response);
@@ -258,35 +258,27 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
     }

     inbuf.ulVersion = SECBUFFER_VERSION;
+    inbuf.cBuffers = 0;
     inbuf.pBuffers = inBufs;

     if (sec_pkg_context_bindings != NULL) {
-        inBufs[secBufferCount].BufferType = SECBUFFER_CHANNEL_BINDINGS;
-        inBufs[secBufferCount].pvBuffer = sec_pkg_context_bindings->Bindings;
-        inBufs[secBufferCount].cbBuffer = sec_pkg_context_bindings->BindingsLength;
-
-        secBufferCount++;
+        inBufs[inbuf.cBuffers].BufferType = SECBUFFER_CHANNEL_BINDINGS;
+        inBufs[inbuf.cBuffers].pvBuffer = sec_pkg_context_bindings->Bindings;
+        inBufs[inbuf.cBuffers].cbBuffer = sec_pkg_context_bindings->BindingsLength;
+        inbuf.cBuffers++;
     }

-    tokenBufferIndex = secBufferCount;
+    tokenBufferIndex = inbuf.cBuffers;
     if (state->haveCtx) {
-        inBufs[secBufferCount].BufferType = SECBUFFER_TOKEN;
-        inBufs[secBufferCount].pvBuffer = base64_decode(challenge, &len);
-        if (!inBufs[secBufferCount].pvBuffer) {
+        haveToken = TRUE;
+        inBufs[tokenBufferIndex].BufferType = SECBUFFER_TOKEN;
+        inBufs[tokenBufferIndex].pvBuffer = base64_decode(challenge, &len);
+        if (!inBufs[tokenBufferIndex].pvBuffer) {
             return AUTH_GSS_ERROR;
         }
-        inBufs[secBufferCount].cbBuffer = len;
-
-        secBufferCount++;
-    } else if (!state->haveCtx && secBufferCount > 0) {
-        // Set the buffer to SECBUFFER_EMPTY if context bindings are set but we have no Context
-        inBufs[secBufferCount].BufferType = SECBUFFER_EMPTY;
-        inBufs[secBufferCount].pvBuffer = NULL;
-        inBufs[secBufferCount].cbBuffer = 0;
-
-        secBufferCount++;
+        inBufs[tokenBufferIndex].cbBuffer = len;
+        inbuf.cBuffers++;
     }
-    inbuf.cBuffers = secBufferCount;

     outbuf.ulVersion = SECBUFFER_VERSION;
     outbuf.cBuffers = 1;
@@ -357,7 +349,7 @@ auth_sspi_client_step(sspi_client_state* state, SEC_CHAR* challenge, SecPkgConte
         status = AUTH_GSS_CONTINUE;
     }
 done:
-    if (inBufs[tokenBufferIndex].pvBuffer) {
+    if (haveToken) {
         free(inBufs[tokenBufferIndex].pvBuffer);
     }
     if (outBufs[0].pvBuffer) {

[behackett]

Assuming that patch fixes the memory corruption issue you're having, I can move this along quickly. I've developed some patches that clean up compiler warnings and fix a few documentation issues (including documenting the compiler you need for Python 2.6 and 2.7).

[jborean93]

Thanks @behackett for your suggestions, I've applied them and tested it locally and it works for me now. As for pInput being null on the first call, I would say it is a docs error as the credentials are rejected if I don't pass in the channel bindings structure everytime to that method.

Let me know if there is anything else you wish for me to change but I'm fairly happy things are working locally.

[behackett]

Awesome. I've merged this and added a few follow up commits. Let me know if you have any problems.

[jborean93]

Thanks for all of this, really appreciate you helping me with some of the issues I had.