SSL connection returns garbage data

\lib\vtls\curl_schannel.c

Can you please provide a patch for the fix you suggest is correct?

I attached 2 files.

One is the zip file that includes my test files and error.txt.

My environment is Windows 7 64 bit, Visual Studio 2010.
I built curl with option in below.
nmake /f Makefile.vc mode=static VC=10 DEBUG=yes MACHINE=x86 GEN_PDB=yes ENABLE_IDN=no

In the zip file, there is a python script for test web server.
The error happens rarely and it could be a python bug but I think curl library also has a bug.
I added error.txt to show the error situation.
After sspi_status is 590615, you can see the receiving garbage data.

== Info: >>schannel: sspi_status: 590615
<<<end =="Info:">>schannel: decrypted data buffer: offset 0 length 16384
<<<end <="Recv" data:="">>>HTTP/1.0 200 OK
Server: Simple<<<end write_callback:="">>>HTTP/1.0 200 OK
Server: Simple<<<end =="Info:">>schannel: client wants to read 16384 bytes

The other file is the diff.
I added one log line for testing and I added a comment where I think it should be fix.

It is the output in error with curl command line. You can see the garbage.

D:\Study\cpp_test\curl_bug>curl -k --cacert "./server.pem" https://localhost/
{"people":
{"name": "abc", "ip_addrs": [
{"ip": "127.0.0.1"},{"ip": "192.168.0.1"}, "id": 1}]}
{"people": [
{"name": "abc", "i
D:\Study\cpp_test\curl_bug>

I don't run or use Windows personally so I can't test this at all.

Your suggested patch has a problem though and that is that the function is declared to return an ssize_t while size is a size_t type which will cause compiler warnings with picky compilers.

This made me go back and check where exactly the missing assignment of ret is that causes it to get returned with the wrong value. How about this change?

~~~~~
diff --git a/lib/vtls/curl_schannel.c b/lib/vtls/curl_schannel.c
index 3a740bd..bc8f58a 100644
--- a/lib/vtls/curl_schannel.c
+++ b/lib/vtls/curl_schannel.c
@@ -1011,10 +1011,12 @@ schannel_recv(struct connectdata *conn, int sockindex,

 infof(data, "schannel: decrypted data returned %zd\n", size);
 infof(data, "schannel: decrypted data buffer: offset %zu length %zu\n",
       connssl->decdata_offset, connssl->decdata_length);

}
+ else
+ ret = 0;

/ check if the server closed the connection /
if(ret <= 0 && ( / special check for Windows 2000 Professional /
sspi_status == SEC_I_CONTEXT_EXPIRED || (sspi_status == SEC_E_OK &&
connssl->encdata_offset > 0 && connssl->encdata_buffer[0] == 0x15))) {

I tested with your change and it worked well.
Thank you!

Can I know when I can get the new version that includes this change?

This caught my eye:
https://github.com/bagder/curl/blob/680d5fd/lib/vtls/curl_schannel.c#L914-L921

The documentation for DecryptMessage [1] says that if the function fails to decrypt the message several errors can be returned. For two of those errors handled in curl_schannel.c SEC_I_RENEGOTIATE/SEC_I_CONTEXT_EXPIRED the code is still reading the buffers as if there could be decrypted data like it's on the success path. I can't find DecryptMessage documented behavior to support that. Maybe some undocumented win2k thing?

1: msdn.microsoft.com/en-us/library/windows/desktop/aa375348.aspx

If committed and pushed the fix for the original problem in commit 145c263a4ba. Keeping this issue open a while longer to allow comments/work on Jay's comment.

I e-mailed Marc and asked him to jump in.
https://github.com/bagder/curl/commit/15ca80c

I am able to reproduce what was reported and your patch works here.
Windows 7 SP1 x64
python-2.7.8.amd64.msi w/ pyOpenSSL-0.14
curl 7.39 built using 'DLL Debug - DLL Windows SSPI - DLL WinIDN'

Hello everyone,

thanks for taking a look into this issue. Daniel's patch looks good and fixes the issue reported within this bug.

@Jay: During the development of the SChannel backend I stumpled upon this undocumented behaviour, because if I didn't read the buffers before closing or renegotiating the connection it would be missing from the output. At least for renegotiation this behaviour is indirectly documented here: http://msdn.microsoft.com/en-us/library/windows/desktop/aa379368.aspx

"The DecryptMessage (General) function traps requests for renegotiation coming from the message sender. It notifies your application by decrypting the message data and returning the SEC_I_RENEGOTIATE value."
(the important part being marked with *)

I noticed that this also applies to connection shutdown and implemented it this way. At least I did not notice any trouble with this implementation until today. Did you?

Best regards,
Marc

Fair enough Marc, thanks for your review. I always try to stick with the documentation but I agree in this case it's inconsistent.

Thanks everyone, I thus consider this issue closed!