300735 - OCSP responses limited to 8K bytes

Status: RESOLVED INACTIVE

(NSS :: Libraries, defect, P2)

Description

[Rodney McDuff]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050714 Firefox/1.0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050714 Firefox/1.0.5

I have been setting up a hierachical PKI with 3 CA levels
(ROOT_CA->LEVEL1_CA->LEVEL2_CA->EE_CERT) using OpenCA. Each CA has a
OCSP responder setup with a ocsp signing cert with the OCSPSigning extended
usage attribute and configure to send the full chain back in the
response. Firefox correctly validiated a EE CERT issuers directly
from ROOT_CA (ROOT_CA->EE_CERT). However it produces a -8063 error
for an EE CERT issued from LEVEL2_CA every time. The problem is that the
ocsp_GetEncodedResponse function in security/nss/lib/certhigh/ocsp.c is
hardwired to read a maximum of 8K. 

If one only send one or 2 certs in the chain the OCSP request is
correctly validated. Any more certs in the chain results in the -8063
error. 

Modifying security/nss/lib/certhigh/ocsp.c like so:

*** security/nss/lib/certhigh/ocsp.c    2005-07-14 14:49:23.960362699 +1000
--- security/nss/lib/certhigh/ocsp.c.dist       2005-07-14 14:48:36.714967153 +1000
***************
*** 1913,1919 ****
      PRInt32 offset = 0;
      PRInt32 inBufsize = 0;
      const PRInt32 bufSizeIncrement = OCSP_BUFSIZE; /* 1 KB at a time */
!     const PRInt32 maxBufSize = 16 * bufSizeIncrement ; /* 16 KB max */
      const char* CRLF = "\r\n";
      const PRInt32 CRLFlen = strlen(CRLF);
      const char* headerEndMark = "\r\n\r\n";
--- 1913,1919 ----
      PRInt32 offset = 0;
      PRInt32 inBufsize = 0;
      const PRInt32 bufSizeIncrement = OCSP_BUFSIZE; /* 1 KB at a time */
!     const PRInt32 maxBufSize = 8 * bufSizeIncrement ; /* 8 KB max */
      const char* CRLF = "\r\n";
      const PRInt32 CRLFlen = strlen(CRLF);
      const char* headerEndMark = "\r\n\r\n";

ie increasing the limit to 16K, solve the problem when sending the
full chain.

Also as additional information the logs from the OCSP responders all
indicate that CERTS were correctly validiated and tests with "openssl
ocsp" also were also correct with the full chain.


Reproducible: Always

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Shortened Summary from 
> Firefox can only read OCSP responses less that 8K. Other is produces the error 
> "Error trying to validate certificate from machine.domain using OCSP - 
> corrupted or unknown response.Error Code: -8063"

Comments:

a) The old rule of thumb was that 90+% of certs are less than 2KB each, 
so an 8KB buffer was thought to be enough for 4 certs most of the time.

b) But who says that chains should be limited to only 4 certs, or that 
the certs must be limited in size in this way?

c) While 16K is better than 8K, they're both arbitrarily short.  Why not
a limit of (say) 128K, or more ?

d) the code starts by allocating a 1KB buffer, and if that's not enough
the code grows it linearly by 1KB at a time until it is big enough.  
I'd suggest we start with a bigger number, (say 4k or 8k) and double it 
each time rather than grow linearly, and not stop until (say) 128KB.
The code ultimately shortens the buffer (by reallocating it) to be just
big enough to hold the needed amount anyway.  

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: implement suggestions in my previous comment

Julien, please review.

Comment 3

[Julien Pierre]

Comment on attachment 235822 [details] [diff] [review]
implement suggestions in my previous comment

I'm not sure if there is one answer for everybone here. A 128 KB buffer might be too much for some platforms (devices) and still too little for some cases (think JPG photo extension in the cert). I think the size of this buffer needs to be configurable by the application.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 235822 [details] [diff] [review]
implement suggestions in my previous comment

Julien, you understand that the buffer starts out at 1K and doubles until it's big enough or is 128KB, yes?  

Did you mean that the maximum size should be configurable by the appliation?  Do we really want to burden apps with yet another thing they must configure? 

If there isn't enough memory for the buffer at (say) 64KB, then the operation will just fail. 
Isn't that enough?

Comment 5

[Julien Pierre]

Yes, I understood what your patch was doing. The size of the buffer is effectively controlled by the size of the certs coming from the OCSP server, up to 128 KB .
If the client is memory constrained, it may not want to deal with allocating 128 KB of RAM just to check a single cert's status. So, I think this needs to be settable to a smaller value. That doesn't preclude changing the default to a slightly higher value than 8 KB, but that should be done at the same time.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Julien, while re-reviewing this patch, and the code it patches, I found 
a bug in the existing code.  It's a problem with improper use of realloc.  
If realloc returns NULL, previously allocated buffer is leaked.  Occurs 
twice in that function.  My next patch will include the fix for this.

Comment 7

[Julien Pierre]

3.11.7 is done. Retargeting to 3.11.8 .

Comment 8

[Julien Pierre]

Nelson, you didn't attach a patch after comment 6. Do you have an updated one ?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

No, I have no further intention of working on this bug.  It's all yours now.

All the realloc calls are wrong.  They all make the mistake of 
x = realloc(x, newsize);
which leaks x if realloc returns NULL.

Comment 10

[Julien Pierre]

Attachment: Fix realloc, add API to set buffer limit

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 286916 [details] [diff] [review]
Fix realloc, add API to set buffer limit

r=nelson for trunk, with a suggested change (for two places).

>Index: nss/nss.def

>+;+NSS_3.11.8 {
>+;+    global:
>+CERT_SetOCSPResponseLimit;
>+;+    local:
>+;+       *;
>+;+};

Is this really the first new function being added in 3.11.8 ?
I seem to recall reviewing some previous additions for this release.
I wonder if maybe some were added in the branch but not the trunk.


>Index: certhigh/ocsp.c

>+        savBuffer = inBuffer;
>         inBuffer = PORT_Realloc(inBuffer, inBufsize+1);
>         if (NULL == inBuffer)
>         {
>+            if (savBuffer)
>+            {
>+                PORT_Free(savBuffer);
>+            }

PORT_Free always checks its argument, so there's no need to check before
calling, and the code looks nicer without the extra if(){} block.
This comment applies to both places where PORT_Free calls were added.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 286916 [details] [diff] [review]
Fix realloc, add API to set buffer limit

>+PRInt32 maxBufSize = 128 * OCSP_BUFSIZE ; /* 128 KB max */

Please declare this global as "static", so it's local to the file.

(Alternatively, only if you want the variable to be globally visible in NSS, 


>@@ -2978,10 +2985,10 @@
>     char* inBuffer = NULL;
>+    char* savBuffer;

Thinking out loud, ignore this comment if you want:

I think it would be safer to init savBuffer to NULL at declaration time.
But I understand it's just a helper variable and only used immediately after an assignment, an init to NULL might be a waste.
I think C allows local variable declaration whenever you enter a new block of code.
You could make it look safer for the reader by moving the declaration to the beginning of each block and combining with the init:

     do
     {
         char *savBuffer = inBuffer;
...
     {
         char *savBuffer = inBuffer;
         /* we still need to receive more body data */


>-    const PRInt32 bufSizeIncrement = OCSP_BUFSIZE; /* 1 KB at a time */
>+    const PRInt32 bufSizeIncrement = OCSP_BUFSIZE; /* 1 KB min */

The loop is still reading "1 KB at a time",
so I don't understand why you're changing this comment.


>@@ -3004,10 +3011,15 @@
>-        inBufsize += bufSizeIncrement;
>+        inBufsize += inBufsize ? bufSizeIncrement : inBufsize;

This is equivalent to the following statement, is that really what you want?

          if (inBufsize != 0)
            inBufsize += bufSizeIncrement;
          else
            inBufsize += 0;

If that's what you want, I'd personally prefer the following more readable statement:

          if (inBufsize)
            inBufsize += bufSizeIncrement;

(You have that code twice)

But I think you had intended something else.
Because of your change, in the first loop iteration, inBufSize will remain at zero, resulting in a buffer allocation of a single byte (inBufsize+1), but the ocsp_read statement will attempt to read "bufSizeIncrement number of bytes" (1024).

Maybe I'm missing an important detail, sorry if I do, but I think this code will crash. r-

Comment 13

[Kai Engert (:KaiE:)]

(In reply to comment #12)
> (Alternatively, only if you want the variable to be globally visible in NSS, 

Please ignore this incomplete sentence.

Comment 14

[Julien Pierre]

Nelson,

Re: comment 11, yes, it is really the only addition to 3.11.8, even on the branch.

Kai,

Re: comment 12, 

I wanted the assignment of savbuffer to be made just before its use. I think it's clear and safe enough the way I wrote it. But I don't object to pre-initializing it to NULL.

Re: the increment statements, this was an error in Nelson's original patch. inBufsize should never remain 0. I think the intended logic was the opposite. I didn't catch that in my review last time.

Comment 15

[Julien Pierre]

Attachment: Update with feedback from Nelson and Kai

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 287029 [details] [diff] [review]
Update with feedback from Nelson and Kai

Ok, you now guarantee that inBufSize will be at least bufSizeIncrement, so it's big enough to hold the initial read.

But, in each loop iteration:
- double buffer size
- read bufSizeIncrement 

Please only reallocate and double the buffer size when you have reached the size.

Right now, you get:

iteration 1:
  buffer 1024
  read   1024

iteration 2
  buffer size 2048
  total read 2048

iteration 3
  buffer size 4096
  total read 3072

iteration 4
  buffer size 8192
  total read 4096

iteration 5
  buffer size 16384
  total read 5120

...

Comment 17

[Kai Engert (:KaiE:)]

Bugs that are currently assigned to Julien => assigning to nobody.
Search for 20100628-kaie-jp

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.