432260 - [@ pkix_pl_HttpDefaultClient_HdrCheckComplete - PKIX_PL_Memcpy] crashes when there is no content-length header in the http response

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Using a debug trunk build, I ran the command
   vfyserv -o www.diginotar.nl

and it crashed with a segmentation violation in libPKIX.  Stack below.
I don't understand why it was in libPKIX at all.  
I had not set the environment variables to make NSS use libPKIX.

nss3.dll!PKIX_PL_Memcpy(void * source=0x00dcf978, 
unsigned int length=0x00000330, void * * pDest=0x010af520, 
void * plContext=0x00000000)  Line 225 + 0x13 bytes

nss3.dll!pkix_pl_HttpDefaultClient_HdrCheckComplete(
PKIX_PL_HttpDefaultClientStruct * client=0x00dccf1c, 
unsigned int bytesRead=0x00000400, int * pKeepGoing=0x010af5e8, 
void * plContext=0x00000000)  Line 350 + 0x1e bytes

nss3.dll!pkix_pl_HttpDefaultClient_RecvHdr(
PKIX_PL_HttpDefaultClientStruct * client=0x00dccf1c, 
int * pKeepGoing=0x010af5e8, 
void * plContext=0x00000000)  Line 861 + 0x15 bytes

nss3.dll!pkix_pl_HttpDefaultClient_Dispatch(PKIX_PL_HttpDefaultClientStruct * client=0x00dccf1c, void * plContext=0x00000000)  Line 1129 + 0x11 bytes

nss3.dll!pkix_pl_HttpDefaultClient_TrySendAndReceive(
void * request=0x00dccf1c, 
unsigned short * http_response_code=0x010af6e0, 
const char * * http_response_content_type=0x00000000, 
const char * * http_response_headers=0x00000000, 
const char * * http_response_data=0x010af6ec, 
unsigned int * http_response_data_len=0x010af6d8, 
PRPollDesc * * pPollDesc=0x00000000, _SECStatus * pSECReturn=0x010af6b0, 
void * plContext=0x00000000)  Line 1484 + 0xd bytes

nss3.dll!pkix_pl_HttpDefaultClient_TrySendAndReceiveFcn(
void * request=0x00dccf1c, PRPollDesc * * pPollDesc=0x00000000, 
unsigned short * http_response_code=0x010af6e0, 
const char * * http_response_content_type=0x00000000, 
const char * * http_response_headers=0x00000000, 
const char * * http_response_data=0x010af6ec, 
unsigned int * http_response_data_len=0x010af6d8)  Line 1692 + 0x2b bytes

nss3.dll!fetchOcspHttpClientV1(PLArenaPool * arena=0x00000000, 
const SEC_HttpClientFcnV1Struct * hcv1=0x0065bd20, 
char * location=0x00dca668, 
SECItemStr * encodedRequest=0x00dca620)  Line 3322 + 0x1e bytes

nss3.dll!ocsp_GetEncodedOCSPResponseFromRequest(
PLArenaPool * arena=0x00000000, CERTOCSPRequestStr * request=0x00dced08, 
char * location=0x00dca668, __int64 time=0x00044c7eab7e111e, 
int addServiceLocator=0x00000000, void * pwArg=0x00000000, 
CERTOCSPRequestStr * * pRequest=0x010af784)  Line 3451 + 0x18 bytes

nss3.dll!ocsp_GetEncodedOCSPResponseForSingleCert(
PLArenaPool * arena=0x00000000, CERTOCSPCertIDStr * certID=0x00dc8920, 
CERTCertificateStr * singleCert=0x00dc11c8, char * location=0x00dca668, 
__int64 time=0x00044c7eab7e111e, int addServiceLocator=0x00000000, 
void * pwArg=0x00000000, 
CERTOCSPRequestStr * * pRequest=0x010af784)  Line 3495 + 0x25 bytes

nss3.dll!ocsp_GetOCSPStatusFromNetwork(NSSTrustDomainStr * handle=0x00d89de8, 
CERTOCSPCertIDStr * certID=0x00dc8920, CERTCertificateStr * cert=0x00dc11c8, 
__int64 time=0x00044c7eab7e111e, void * pwArg=0x00000000, 
int * certIDWasConsumed=0x010af7d8, 
_SECStatus * rv_ocsp=0x010af7dc)  Line 4795 + 0x27 bytes

nss3.dll!CERT_CheckOCSPStatus()          Line 4703 + 0x25 bytes
nss3.dll!CERT_VerifyCertificate()        Line 1310 + 0x1b bytes
nss3.dll!CERT_VerifyCertificateNow()     Line 1521 + 0x2b bytes
vfyserv.exe!myAuthCertificate()          Line 153 + 0x1f bytes
ssl3.dll!ssl3_HandleCertificate()        Line 7260 + 0x21 bytes
ssl3.dll!ssl3_HandleHandshakeMessage()   Line 7938 + 0x11 bytes
ssl3.dll!ssl3_HandleHandshake()          Line 8062 + 0x19 bytes
ssl3.dll!ssl3_HandleRecord()             Line 8325 + 0xd bytes
ssl3.dll!ssl3_GatherCompleteHandshake()  Line 206 + 0x17 bytes
ssl3.dll!ssl_GatherRecord1stHandshake()  Line 1258 + 0xb bytes
ssl3.dll!ssl_Do1stHandshake()            Line 151 + 0xf bytes
ssl3.dll!ssl_SecureSend()                Line 1152 + 0x9 bytes
ssl3.dll!ssl_SecureWrite()               Line 1197 + 0x13 bytes
ssl3.dll!ssl_Write()                     Line 1487 + 0x17 bytes
nspr4.dll!PR_Write()                     Line 146 + 0x16 bytes
vfyserv.exe!handle_connection()          Line 233 + 0x1d bytes
vfyserv.exe!do_connects()                Line 348 + 0x10 bytes
vfyserv.exe!thread_wrapper()             Line 450 + 0x15 bytes

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I figured out why vfyserv was using PKIX to do OCSP.  
NSS Initialization configured pkix as the default http client for OCSP.
The stack is:

nss3.dll!SEC_RegisterDefaultHttpClient(fcnTable=0x0065bd1c)  Line 283
nss3.dll!pkix_pl_HttpDefaultClient_RegisterSelf()            Line 560
nss3.dll!PKIX_PL_Initialize()                                Line 254 
nss3.dll!PKIX_Initialize()                                   Line 106
nss3.dll!nss_Init()                                          Line 577
nss3.dll!NSS_NoDB_Init()                                     Line 702 
vfyserv.exe!main()                                           Line 477

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The server sent back an http response with no content-length header.
pkix_pl_HttpDefaultClient_HdrCheckComplete crashes when there is no 
content-length header in the http response.

The response from the ocsp responder began with this http header:

HTTP/1.1 200 OK..
Server: Apache-Coyote/1.1..
Set-Cookie: JSESSIONID=4F724CE80FA119F30ABFC2B5B20CA1D7; Path=/..
Content-Type: application/ocsp-response..
Date: Mon, 05 May 2008 20:23:19 GMT..
Connection: close..
..

Where the trailing ".." on each line are \r\n

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The above response is valid and legal.  The code should handle it without
crashing and without error.  This is P1 for Sun.

I worked around it in the debugger by setting breakpoints at 4 locations
and taking certain corrective steps.  Set all 4 breakpoints up front, then
take the actions shown below as you hit those breakpoints.  Those were:

1. Breakpoint in function pkix_pl_HttpDefaultClient_HdrCheckComplete() Line 327
        client->rcv_http_data_len = contentLength;
conditional on contentLength being zero.  
When hit, change it to the value in client->maxResponseLen and continue.

2. Breakpoint in function pkix_pl_Socket_Recv() at Line 1068
                PKIX_ERROR(PKIX_PRRECVREPORTSNETWORKCONNECTIONCLOSED);
when hit, move the program counter down to line 1089
        *pBytesRead = (PKIX_Int32)bytesRead;
and continue

3. Breakpoint in function pkix_pl_HttpDefaultClient_RecvBody() at Line 1000
                client->connectStatus = HTTP_RECV_BODY_PENDING;
when hit, set client->connectStatus to HTTP_COMPLETE (which is 8), 
set *pKeepGoing to 0, move the program counter down to line 1006 
(label cleanup:) and continue.

4. Breakpoint in function pkix_pl_HttpDefaultClient_TrySendAndReceive() at  Line 1516
                                *http_response_data_len =
                                         client->rcv_http_data_len;
when hit, step over it, then 
set *http_response_data_len to client->currentBytesAvailable and continue.

Comment 4

[Julien Pierre]

Is there any code to handle chunked encoding ? (Transfer-encoding: Chunked header) . That is another type of valid encoding in HTTP/1.1 where content-length is omitted.

Comment 5

[Alexei Volkov]

(In reply to comment #4)
> Is there any code to handle chunked encoding ? (Transfer-encoding: Chunked
> header) . That is another type of valid encoding in HTTP/1.1 where
> content-length is omitted.
There is not code to support it. In fact the code that exists today can not handle any response which header does not have Context-length.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Right.  This http engine needs a lot of work to be ready for our servers. :(

Comment 7

[Alexei Volkov]

 PKIX_PL_HttpDefaultClient has the field maxResponseLen that tells to the http client when response should be considered to be too big and the client should stop receiving it. It set to 0(unlimited) in most of the cases of use http client through out libpkix code. This is potential security problem for servers especially for cert fetching through AIA url, since trust of the peer can not be established before communication.

Comment 8

[Alexei Volkov]

Attachment: Patch v1 (not for review)

Need to add code to pkix_pl_HttpDefaultClient_RecvBody to handle error that pkix_pk_Socket_Receive throws when server closes connection.

Comment 9

[Julien Pierre]

Actually as long as we send an HTTP/1.0 request, we aren't required to handle 1.1 features such as chunked encoding. This feature can help the server maintain the connection open if it doesn't know the content-length upfront. The client isn't set to take advantage of keep-alives today, so this is not really relevant. But if and when we do want to handle HTTP keep-alives, then we should improve the client to support HTTP/1.1 and chunked-encoding.

Comment 10

[Alexei Volkov]

Attachment: Patch v2 - read until connection is closed

read until connection connection is closed in cases when context length was not specified in a header.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 326393 [details] [diff] [review]
Patch v2 - read until connection is closed

>-/* ***** BEGIN LICENSE BLOCK *****
>+/* ***** Begin LICENSE BLOCK *****

Uh, no.


>+         case -1:
>+             client->rcv_http_data_len = -1; /* Will be set when
>+                                              * connection get closed */ 

It would be better if that comment said where (in what function) the 
correct value will be set.  

>+             client->bytesToRead = -1;       /* Unknow contentLength indicator*/

The above statements assign a signed negative value to unsigned variable 
members of the client struct.  ALL of the integer variables in that struct
are unsigned.  

Also, there is a spelling error.  Should be "Unknown".  

>+        bytesToRead = (client->bytesToRead != -1) ? client->bytesToRead :
>+                                                    client->capacity;

That line tests an unsigned member variable for a negative value. 

>+
>+        
>+        /* XXX Need to do something with pkix_pl_Socket_Recv throwing error
>+         * then server connection is closed. bytesRead in this case will
>+         * be eq to 0. */

Please eliminate that comment.

>+		if (bytesToRead > (PKIX_UInt32)bytesRead) {
>+                       if (client->bytesToRead != -1) {

Another comparison of -1 and an unsigned variable.

Apart from the signed/unsigned issues, I think there is another problem 
with assigning -1 to some of these variables.  I will write another comment
about that.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 326393 [details] [diff] [review]
Patch v2 - read until connection is closed

In file pkix_pl_httpdefaultclient.c in function  
pkix_pl_HttpDefaultClient_TrySendAndReceive, we see the following code 
that attempts to deal with the HTTP_ERROR error case.

1478            case HTTP_ERROR:
1479                    /* Did caller provide a pointer for length? */
1480                    if (client->pRcv_http_data_len != NULL) {
1481                            /* Was error "response too big?" */
1482                            if (client->maxResponseLen >=
1483                                    client->rcv_http_data_len) {
1484                                    /* Yes, report needed space */
1485                                    *(client->pRcv_http_data_len) =
1486                                             client->rcv_http_data_len;

The problem I see here is that the new patch has set 
client->rcv_http_data_len to the value -1, which ensures that the test 
         if (client->maxResponseLen >= client->rcv_http_data_len) {
will evaluate to true.  This means "response too big", but that is 
actually not the real problem in this case.

Comment 13

[Alexei Volkov]

Attachment: Patch v3

Changes according review comments.

Added code to increase buffer size for downloads with unknown context length(function RecvBody). See comments in the function.

Comment 14

[Alexei Volkov]

Attachment: Patch v3 - Resubmitting the patch to be able to compare with v2

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 331002 [details] [diff] [review]
Patch v3 - Resubmitting the patch to be able to compare with v2

This patch appears to be functionally correct, but I'd like to see
it be improved in a number of ways.  The biggest problem is with the
naming of variables, especially members of the client structure. 
Here are some suggested changes:

1) create a new #define symbol, something like CONTENT_LENGTH_UNKONWN
and define it to -1, then use that symbol instead of -1 in the 
relevant places.

2) the variable named client->capacity is not the allocated buffer size
as the name suggests, but rather is the unused unfilled portion of the 
allocated buffer.  Please rename it to something like unusedBytes.

3) the variable named client->bytesToRead is actually the number of 
*remaining* bytes of the response that have not yet been gathered in, 
but only when the total content length is known at the beginning 
(specified in Content-Length).  When that value is unknown (-1), then 
another variable serves the purpose of telling how much of the response 
remains to be read, and that is known as client->capacity.

It seems like there should be only one variable which is the number 
of bytes of allocated buffer that remain to be filled, and it should
be used whether the total content length is known or not.  

I'd suggest that we have one variable which is the total expected 
length, and is either unknown or some non-zero value that does not 
change as the buffer is filled.  Another variable can keep track of 
how much of the allocated buffer remains to be filled.

4) Please add a new variable to the client structure that holds the 
allocated size of the buffer, so that it is not necessary to frequently
recompute that value by adding the variables that are presently named
     client->currentBytesAvailable + client->capacity;
Replace local variables that hold that sum with references to the 
variable in the client structure.  I might suggest calling that variable
"capacity".  Then eliminate all the many places that recompute that 
value.  It should only need to be computed once, when the buffer is 
allocated (or reallocated).  For example, the following test

>+                if (client->capacity < newLength - client->currentBytesAvailable) {

can become 

>+                if (client->capacity < newLength) {

if and when capacity is the actual size of the allocated buffer.

6)

> cleanup:
>+        if (PKIX_ERROR_RECEIVED && pkixErrorResult &&
>+            pkixErrorResult->errCode ==
>+            PKIX_PRRECVREPORTSNETWORKCONNECTIONCLOSED) {

I believe that the expression (PKIX_ERROR_RECEIVED && pkixErrorResult) 
is equivalent to (pkixErrorResult).

Comment 16

[Alexei Volkov]

Attachment: Patch v4 - simplify code. Remove unnecessary variables.

Two variables removed from client structure: bytesToRead and alreadyScanned. Use capacity and filledupBytes as buffer size and buffer write pointer indicators.

Function pkix_pl_HttpDefaultClient_RecvBodyContinue got removed. Its code has been merged with pkix_pl_HttpDefaultClient_RecvBody function.

Code that checks content type to ocsp-response type is moved into pkix_pl_ocspresponse.c file.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 331227 [details] [diff] [review]
Patch v4 - simplify code. Remove unnecessary variables.

r+ from me, with a few cosmetic changes to the patch.

>+
>+        /*  */

Please add some text to that empty comment, or delete it.

>+            /* Already searched before. Substruct eoh length and try again. */

s/Substruct/Subtract/.  I think this comments wants to say something like
"back up and restart scaning over a few bytes that were scanned before."

>+            /* A search from the begenning of the buffer. */

s/begenning/beginning/

>+         case -1:
>+             /* Unknown contentLength indicator.Will be set by

This case should be case HTTP_UNKNOWN_CONTENT_LENGTH

>+            /* Reading till the EOF. Context lenght is not known.*/
>+                /* New lenght will be consist of available(downloaded) bytes,

s/lenght/length/g   
This spelling error occurs several places, please fix it throughout the file.

>+        /* Use pool callback if was wating on IO */

That's "poll" callback and "waiting" on IO.  I suggest you change it to say
"if using nonblocking IO" rather than "if waiting on IO".

>+        /* continue if not enought bytes read or if complete size of

s/enought/enough/

>+                                if (client->rcv_http_data_len != -1 &&
>+                                    client->maxResponseLen >=
>+                                                client->rcv_http_data_len) {

That -1 should be HTTP_UNKNOWN_CONTENT_LENGTH

>Index: lib/libpkix/pkix_pl_nss/module/pkix_pl_httpdefaultclient.h

>-#define HTTP_OCSP_BUFSIZE 1024
>+#define HTTP_DATA_BUFSIZE 4024

You said you wanted this value to be 4K, which is 4096

>@@ -556,6 +557,11 @@ pkix_pl_OcspResponse_Create(
>                         PKIX_ERROR(PKIX_OCSPSERVERERROR);
>                 }       
> 

Please add a comment here explaining why it's ok to use strcasecmp instead
of strncasecmp, and why we don't need to free the string at 
"responseContentType" in this function.

>+                if (PORT_Strcasecmp(responseContentType, 
>+                                    "application/ocsp-response")) {
>+                        PKIX_ERROR(PKIX_OCSPSERVERERROR);
>+                }

Thanks for getting rid of lots of blank lines, and lots of uses of the 
PKIX_PL_NSSCALLRV macro!  Those changes really help it be more readable.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Alexei, This patch has had r+ for almost 90 days now. 
It should have gone into 3.12.2.  A reviewed but not committed patch does
us no good.  Now, we're in a short freeze for the 3.12.2 release.
Please commit this patch as soon as the tree opens again.

Comment 19

[Alexei Volkov]

patch is integrated

Comment 20

[Alexei Volkov]

The patch cause tboxs to become orange for the reason of failure to separate http header information from the data in the case, when complete header and data was read in a single attempt.

Patch is coming...

Comment 21

[Alexei Volkov]

Attachment: Patch v5 - fix tinderbox failure.

the "goto cleanup" at line 331 of old code caused to skip separation of the received context from a header.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 346504 [details] [diff] [review]
Patch v5 - fix tinderbox failure.

r=nelson
Should this code also set *pKeepGoing in the following code path?

314          default:
315              client->rcv_http_data_len = contentLength;
316              if (client->maxResponseLen > 0 &&
317                  client->maxResponseLen < contentLength) {
318                  client->connectStatus = HTTP_ERROR;
319                  goto cleanup;
320              }

Comment 23

[Alexei Volkov]

(In reply to comment #22)
> (From update of attachment 346504 [details] [diff] [review])
> r=nelson
> Should this code also set *pKeepGoing in the following code path?
> 
> 314          default:
> 315              client->rcv_http_data_len = contentLength;
> 316              if (client->maxResponseLen > 0 &&
> 317                  client->maxResponseLen < contentLength) {
> 318                  client->connectStatus = HTTP_ERROR;
> 319                  goto cleanup;
> 320              }
Ever way is fine when connectStatus is set to ERROR.

Comment 24

[Alexei Volkov]

patch 346504 is integrated