#1462 SSL connection returns garbage data

closed-fixed
5
2014-12-15
2014-12-05
No

When curl receives encrypted data from ssl connection, schannel_recv() tries to decrypt.
If s_pSecFn->DecryptMessage() returns a error such as SEC_I_CONTEXT_EXPIRED, schannel_recv() returns ret variable that has the encrypted packet length not decrypted packet size.

So the last line of schannel_recv() should be changed to return size variable.

Discussion

  • Tae Hyoung Ahn

    Tae Hyoung Ahn - 2014-12-05

    \lib\vtls\curl_schannel.c

     
    • Christopher Araman

      This changes the behavior of curl_easy_recv when there is no data to receive on the socket. Before this change, curl_easy_recv would return CURLE_AGAIN. Now it returns CURLE_OK with 0 bytes received, as if the socket has been closed.

       
  • Daniel Stenberg

    Daniel Stenberg - 2014-12-06

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

     
  • Daniel Stenberg

    Daniel Stenberg - 2014-12-06
    • labels: SSL --> SSL, DarwinSSL
    • assigned_to: Daniel Stenberg
     
  • Tae Hyoung Ahn

    Tae Hyoung Ahn - 2014-12-06

    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.

     
  • Daniel Stenberg

    Daniel Stenberg - 2014-12-08

    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))) {

     
  • Tae Hyoung Ahn

    Tae Hyoung Ahn - 2014-12-09

    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?

     
  • Jay Satiro

    Jay Satiro - 2014-12-09

    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

     
    Last edit: Jay Satiro 2014-12-09
  • Daniel Stenberg

    Daniel Stenberg - 2014-12-09
    • labels: SSL, DarwinSSL --> SSL, winssl
    • status: open --> open-confirmed
     
  • Daniel Stenberg

    Daniel Stenberg - 2014-12-09

    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.

     
  • Jay Satiro

    Jay Satiro - 2014-12-11

    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'

     
  • Marc

    Marc - 2014-12-14

    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

     
  • Jay Satiro

    Jay Satiro - 2014-12-15

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

     
  • Daniel Stenberg

    Daniel Stenberg - 2014-12-15

    Thanks everyone, I thus consider this issue closed!

     
  • Daniel Stenberg

    Daniel Stenberg - 2014-12-15
    • status: open-confirmed --> closed-fixed
     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks