Last Comment Bug 480257 - OCSP fails when response > 1K Byte
: OCSP fails when response > 1K Byte
Status: RESOLVED FIXED
SUN_MUST_HAVE
: regression
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P1 major (vote)
: 3.12.3
Assigned To: Alexei Volkov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-25 21:36 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-03-04 10:20 PST (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 - strdup context-type value and save a pointer into client internal variable (2.12 KB, patch)
2009-03-02 12:37 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2009-02-25 21:36:32 PST
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) 2009-02-25 21:46:46 PST
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) 2009-02-25 21:53:13 PST
When I insert a strdup call at line 255, the vfychain test passes.
Comment 3 Alexei Volkov 2009-03-02 12:37:09 PST
Created attachment 364987 [details] [diff] [review]
patch v1 - strdup context-type value and save a pointer into client internal variable
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-03-02 13:31:22 PST
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 2009-03-04 10:20:23 PST
committed.

Note You need to log in before you can comment on or make changes to this bug.