OCSP fails when response > 1K Byte

RESOLVED FIXED in 3.12.3

Status

NSS
Libraries
P1
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Alexei Volkov)

Tracking

({regression})

3.12
3.12.3
regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: SUN_MUST_HAVE)

Attachments

(1 attachment)

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.
(Reporter)

Updated

9 years ago
Priority: -- → P1
Whiteboard: SUN_MUST_HAVE
(Reporter)

Updated

9 years ago
Keywords: regression
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         }
When I insert a strdup call at line 255, the vfychain test passes.
(Assignee)

Comment 3

9 years ago
Created attachment 364987 [details] [diff] [review]
patch v1 - strdup context-type value and save a pointer into client internal variable
Attachment #364987 - Flags: review?(nelson)
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
Attachment #364987 - Flags: review?(nelson) → review+
(Assignee)

Comment 5

9 years ago
committed.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.