480257 - OCSP fails when response > 1K Byte

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

When an OCSP response is too big to fit in the first 1KB receive buffer, 
so that the response header fits in the first 1KB buffer, but the body 
spills over into the second read buffer, the content-type check fails, 
because the pointer to the content-type string points into a buffer that 
has been freed.  The bug is in function pkix_pl_HttpDefaultClient_HdrCheckComplete and possibly other functions too. 
Looking in file lib/libpkix/pkix_pl_nss/module/pkix_pl_httpdefaultclient.c 
we see:

253                 comp = PORT_Strcasecmp(nextHeader, "content-type");
254                 if (comp == 0) {
255                         client->rcvContentType = value;

That really needs to be a strdup of value.  Value points into the 1KB
receive buffer, which is then freed before the function returns.

347          PKIX_CHECK(PKIX_PL_Free(client->rcvBuf, plContext),
348                     PKIX_FREEFAILED);

I found this by running a simple vfychain test on a build that uses a 
version of malloc that completely overwrites freed buffers when they 
are freed.  All our libPKIX testing should be done with a libmalloc 
that does that sort of thing.


Steps to reproduce:
1) Get a cert chain for 

1. Get the cert chain for www.paypal.com with the command:
  vfyserv -c -d DB www.paypal.com
That will create files named.cert.000, cert.001, etc.

2) Run the command 

vfychain -d DB -u 1 -pp \
-g leaf -h testLocalInfoFirst,requireFreshInfo -m ocsp -s failIfNoInfo \
-g chain -h testLocalInfoFirst,requireFreshInfo -m ocsp -s failIfNoInfo \
cert.0*

It will report that the chain is bad.  Setting a breakpoint at the lines 
shown above in pkix_pl_HttpDefaultClient_HdrCheckComplete will show the 
cause.  

When the line is changed to do a strdup, then probably other code elsewhere
will need to be changed to free that dup'ed string.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Oh, I forgot to mention these vital lines.  Perhaps this is where the 
strdup must go.

269         /* Did caller provide a pointer to return content-type? */
270         if (client->rcv_http_content_type != NULL) {
271                 *(client->rcv_http_content_type) = client->rcvContentType;
272         }

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

When I insert a strdup call at line 255, the vfychain test passes.

Comment 3

[Alexei Volkov]

Attachment: patch v1 - strdup context-type value and save a pointer into client internal variable

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 364987 [details] [diff] [review]
patch v1 - strdup context-type value and save a pointer into client internal variable

r=nelson, after you make one change:

Considering that the string is allocated with PORT_Strdup, and not
with some PKIX_PL_ function,

>-                        client->rcvContentType = value;
>+                        client->rcvContentType = PORT_Strdup(value);

It should be freed with a call to PORT_Free, not PKIX_PL_FREE.

>+        if (client->rcvContentType) {
>+            PKIX_PL_Free(client->rcvContentType, plContext);

So, change that.  Then, R=nelson

Comment 5

[Alexei Volkov]

committed.