Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further amendment to #114 #128

Closed
wants to merge 4 commits into from
Closed

Further amendment to #114 #128

wants to merge 4 commits into from

Conversation

kentcb
Copy link
Contributor

@kentcb kentcb commented Aug 4, 2014

Alas, I was correct in saying I had seen SEC_E_BUFFER_TOO_SMALL being returned by InitializeSecurityContext. Here is a stack trace from my user:

System.ComponentModel.Win32Exception: The buffers supplied to a function was too small
   at Waffle.Windows.AuthProvider.WindowsSecurityContext.Initialize(SecHandle credentials, String targetName, Int32 fContextReq, Int32 targetDataRep, SecHandle context, SecBufferDesc continueTokenBuffer)
   at Waffle.Windows.AuthProvider.WindowsSecurityContext.Initialize(SecHandle credentials, String targetName, Int32 fContextReq, Int32 targetDataRep)
   at Waffle.Windows.AuthProvider.WindowsSecurityContext..ctor(String targetName, WindowsCredentialsHandle credentials, String securityPackage, Int32 fContextReq, Int32 targetDataRep)
   at Waffle.Windows.AuthProvider.WindowsSecurityContext.GetCurrent(String package, String targetName, Int32 fContextReq, Int32 targetDataRep)

Thus, I have re-added the code that treats SEC_E_BUFFER_TOO_SMALL the same as SEC_E_INSUFFICIENT_MEMORY. I realize MSDN does not indicate this error is possible from that function, but it certainly is. I have already tested the above change for my user, and all is good.

It would probably be good for someone to update the Java code too...?

Where relevant, tokens are obtained in a loop with an increasing buffer size as long as SEC_E_BUFFER_TOO_SMALL or SEC_E_INSUFFICIENT_MEMORY is returned.
…MEMORY, because even though the MSDN docs don't indicate that this error can occur when calling InitializeSecurityContext, it certainly can.
@dblock
Copy link
Collaborator

dblock commented Aug 6, 2014

I don't think we should merge it. First, I do think that we should make the change in all the code, .NET and Java if they were to be, but furthermore I think we should figure out why this is returning an error that is not documented in MSDN. In my experience this has been a side effect of a bigger problem :)

@kentcb
Copy link
Contributor Author

kentcb commented Aug 6, 2014

I will have to run with my own patched version then. I have returned to Australia so it's impossible for me to diagnose further with the user in question (not that I'd have any ideas how to pin this down any further than it already is). I work for an investment bank, and you wouldn't believe the hoops I have to jump through to do this stuff.

@dblock
Copy link
Collaborator

dblock commented Aug 6, 2014

I understand, thx @kentcb.

@wbond
Copy link

wbond commented Dec 9, 2015

I just wanted to chime in here with some info I found. I've been working on a separate project that integrates with SChannel and InitializeSecurityContext and have also occasionally seen SEC_E_BUFFER_TOO_SMALL.

Through various testing, I've tracked it down to the ServerKeyExchange handshake message that is sent from the server to the client for DHE_RSA cipher suites. That is:

  • TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
  • TLS_DHE_RSA_WITH_AES_256_GCM_SHA384

Sometimes a server will send a value for dh_Ys from https://tools.ietf.org/html/rfc5246#page-51 with a byte encoding that is one byte less than other times. This appears to be instances where the public integer can be encoded in a shorter byte string. In these situations, SChannel returns a SEC_E_BUFFER_TOO_SMALL error.

I've confirmed this to happen on Windows 7 and Windows 8.1. I have not confirmed on Windows 10 yet.

It seems the solutions to this include:

  • Restart the TLS connection (there is a very high chance the new exchange will not experience this issue)
  • Don't enable TLSv1.2, instead just TLSv1.1

@dblock
Copy link
Collaborator

dblock commented Dec 10, 2015

Thanks @wbond. I am fine merging this, @kentcb could you update CHANGELOG and squash these commits?

@hazendaz
Copy link
Member

This has been merged on bdc8ca5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants