432301 - Algorithm to find end-of-header in pkix_pl_HttpDefaultClient_RecvHdr is wrong

Status: RESOLVED INVALID

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

pkix_pl_HttpDefaultClient_RecvHdr reads the http response into a buffer
1024 bytes at a time, and scans those 1024 bytes for \r\n\r\n.  
If it doesn't find that string, it repeats this procedure, reading
in an additional 1024 bytes each time, and scanning the new 1024 bytes
for that string.  If the desired string is split across one of those
1024-byte boundaries, it will never find the string, even though the 
string is present in the http response.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Because of the security implications of failing to decode an ocsp response,
this is P1.  But it's not exploitable, AFAIK.

Comment 2

[Alexei Volkov]

Didn't find any code in pkix_pl_HttpDefaultClient_RecvHdr that tries to identify an end of the header. The header get parsed in pkix_pl_HttpDefaultClient_HdrCheckComplete function.  Here is the block that searches for the end of the header in last function:

144         alreadyScanned = client->alreadyScanned;
145         /*
146          * If this is the initial buffer, we have to scan from the beginning.
147          * If we scanned, failed to find eohMarker, and read some more, we
148          * only have to scan from where we left off.
149          */
150         if (alreadyScanned > eohMarkLen) {
151                 searchOffset = alreadyScanned - eohMarkLen;
152                 PKIX_PL_NSSCALLRV(HTTPDEFAULTCLIENT, eoh, PL_strnstr,
153                         (&(client->rcvBuf[searchOffset]),
154                         eohMarker,
155                         client->capacity - searchOffset));
156         } else {
157                 PKIX_PL_NSSCALLRV(HTTPDEFAULTCLIENT, eoh, PL_strnstr,
158                         (client->rcvBuf, eohMarker, bytesRead));
159         }

The code above start search from the beginning of the buffer if alreadyScanned is less than 4. Otherwise it searches from  alreadyScanned - 4 which is correct thing to do.

The buffer client->rcvBuf is reallocated(performance issue!) and is appended with new bytes on every read.

In my opinion the code does the right thing in the sense that it wont  miss end of header. Closing the bug as invalid.