Closed Bug 697420 Opened 13 years ago Closed 12 years ago

crash in [@ CERT_DestroyOCSPResponse]

Categories

(NSS :: Libraries, defect, P1)

3.12.11
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.13.4

People

(Reporter: wolfiR, Assigned: wtc)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

Got a report with the stacktrace below.

ocsp_GetResponseSignature() returns invalid data apparently. Haven't checked how that gets in though.

## bt full
#0  0x00007ffff42e1d53 in CERT_DestroyOCSPResponse (response=0x7fffd7676020) at ocsp.c:2728
        signature = 0x20
#1  0x00007ffff42e2f1f in ocsp_CacheEncodedOCSPResponse (handle=0x7fffd5b4e030, certID=0x7fffd765a020, cert=0x7fffd3952020, time=1319634420354281, pwArg=0x7fffd712c810, encodedResponse=<optimized out>, 
    certIDWasConsumed=0x7fffe0cfd890, cacheNegative=1, rv_ocsp=0x7fffe0cfd894) at ocsp.c:5099
        response = 0x7fffd7676020
        signerCert = 0x0
        issuerCert = 0x0
        single = 0x0
        rv = SECFailure
#2  0x00007ffff42e3233 in ocsp_GetOCSPStatusFromNetwork (rv_ocsp=0x7fffe0cfd894, certIDWasConsumed=0x7fffe0cfd890, pwArg=0x7fffd712c810, time=1319634420354281, cert=0x7fffd3952020, certID=0x7fffd765a020, 
    handle=0x7fffd5b4e030) at ocsp.c:4965
        encodedResponse = <optimized out>
        request = 0x7fffdf786020
        rv = SECFailure
        location = 0x7fffd5cae3e0 "http://ca.nordisch.org:2560/"
        locationIsDefault = 0
#3  CERT_CheckOCSPStatus (handle=0x7fffd5b4e030, cert=0x7fffd3952020, time=1319634420354281, pwArg=0x7fffd712c810) at ocsp.c:4800
        certID = 0x7fffd765a020
        certIDWasConsumed = 1
        rv = <optimized out>
        rvOcsp = SECFailure
        dummy_error_code = 0
#4  0x00007ffff42e6247 in CERT_VerifyCert (handle=0x7fffd5b4e030, cert=0x7fffd3952020, checkSig=1, certUsage=certUsageSSLServer, t=1319634420354281, wincx=0x7fffd712c810, log=0x0) at certvfy.c:1307
        rv = <optimized out>
        requiredKeyUsage = 16384
        requiredCertType = 64
        flags = <optimized out>
        certType = <optimized out>
        allowOverride = 1
        validity = <optimized out>
        statusConfig = <optimized out>
#5  0x00007ffff42e6420 in CERT_VerifyCertNow (handle=0x7fffd5b4e030, cert=0x7fffd3952020, checkSig=1, certUsage=certUsageSSLServer, wincx=0x7fffd712c810) at certvfy.c:1342
Crash Signature: CERT_DestroyOCSPResponse → [@ CERT_DestroyOCSPResponse ]
Severity: normal → critical
Blocks: 707873
Depends on: 716345
Depends on: CVE-2012-0441
I studied the code related to the call stack carefully.  I will
attach a patch that should fix this crash.
Assignee: nobody → wtc
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P1
Target Milestone: --- → 3.13.4
Attached patch Proposed patch (obsolete) — Splinter Review
Bob: the following new code I added to ocsp.c is the crash fix:

     basic = response->responseBytes->decodedResponse.basic;
     PORT_Assert(basic != NULL);
+    if (NULL == basic) {
+        return NULL;
+    }

     return &(basic->responseSignature);

The other changes are just code cleanup.  I explain the fix
below.  If it is too long, you can just read the last two
paragraphs.

I think the following events led to the crash:

ocsp_CacheEncodedOCSPResponse called CERT_DecodeOCSPResponse.

In CERT_DecodeOCSPResponse, the SEC_QuickDERDecodeItem call returned
a response->responseStatus with a zero 'len' field.

The DER_GetInteger(&response->responseStatus) call dereferenced the
first byte in response->responseStatus->data.  It happened to have
the most significant bit set, so DER_GetInteger returned -1L.

-1L is not equal to ocspResponse_successful (0), so CERT_DecodeOCSPResponse
returned 'response' without calling ocsp_DecodeResponseBytes.
Without calling ocsp_DecodeResponseBytes,
response->responseBytes->decodedResponse.basic was not set, so it remained
NULL.

Now back to ocsp_CacheEncodedOCSPResponse.  ocsp_CacheEncodedOCSPResponse
proceeded to call CERT_GetOCSPResponseStatus(response).  Since
response->statusValue was -1L, CERT_GetOCSPResponseStatus returned
SECFailure, so we did "goto loser" and called CERT_DestroyOCSPResponse.

CERT_DestroyOCSPResponse called ocsp_GetResponseSignature.
'basic' was NULL.  In a release build the PORT_Assert was gone.
So ocsp_GetResponseSignature returned &(basic->responseSignature),
which was 32 (0x20) for 64-bit platforms.  Unfortunately that
caused the null pointer check for 'signature' to pass, so
CERT_DestroyOCSPResponse dereferenced 'signature' to get
signature->cert and crashed.

In summary, if ocsp_GetResponseSignature returns NULL instead
of &(basic->responseSignature) when 'basic' is NULL, the
null pointer check for 'signature' will correctly detect it and
won't dereference the null 'signature'.
Attachment #600588 - Flags: review?(rrelyea)
My previous patch is not good enough for a debug build.
The assertion
    
    PORT_Assert(response->responseBytes->responseTypeTag
		== SEC_OID_PKIX_OCSP_BASIC_RESPONSE);

will fail under the condition that leads to the crash
because response->responseBytes->responseTypeTag will
still have the initial value SEC_OID_UNKNOWN.

I verified that if response->responseBytes->responseTypeTag
is SEC_OID_PKIX_OCSP_BASIC_RESPONSE,
response->responseBytes->decodedResponse.basic cannot be NULL.
So I keep
    PORT_Assert(basic != NULL);

Please review the new patch.  Thanks.
Attachment #600588 - Attachment is obsolete: true
Attachment #600588 - Flags: review?(rrelyea)
Attachment #600595 - Flags: review?(rrelyea)
The change to ocsp.c looks good to me, r=kaie on that part.

I cannot judge whether your change to dersubr.c is safe.
You introduce a new hard assert (abort on failure), while the current code returns gracefully if the input is empty.
Comment on attachment 600595 [details] [diff] [review]
Proposed patch v2 (checked in ocsp.c only)

> You introduce a new hard assert (abort on failure), while the current code
> returns gracefully if the input is empty.

I take that back.
The code in dersubr.c already dereferences the SECItem's data without length checking.

r=kaie
Attachment #600595 - Flags: review+
Comment on attachment 600595 [details] [diff] [review]
Proposed patch v2 (checked in ocsp.c only)

r+ I had the same question as kai reguarding length (weren't we just returning 0 for the value if len =0), but it's clear that we try to dereference the first byte of data.

There may be cases where data was not null that we would get a potential UMR but not crash, but I think the asserts are safest.


bob
Attachment #600595 - Flags: review?(rrelyea) → review+
Comment on attachment 600595 [details] [diff] [review]
Proposed patch v2 (checked in ocsp.c only)

Bob: thanks for the review.

If len is 0, DER_GetInteger will return either 0 or -1 depending on
value of the UMR byte.  I guess you'd prefer that we return 0
deterministically.  I'll write a patch to do that.

I checked in the ocsp.c change in this patch on the NSS trunk (NSS 3.13.4).

Checking in ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.69; previous revision: 1.68
done
Attachment #600595 - Attachment description: Proposed patch v2 → Proposed patch v2 (checked in ocsp.c only)
Today, if len is 0, DER_GetInteger will either crash (if data is NULL),
or return 0 or -1 depending on the UMR value.

With this patch, DER_GetInteger will return 0 if len is 0.  It also
sets an error code, but DER_GetInteger doesn't return a status, so the
caller will need to call PR_SetError(0) before calling DER_GetInteger
to detect if DER_GetInteger failed.
Attachment #605999 - Flags: review?(rrelyea)
Crash Signature: [@ CERT_DestroyOCSPResponse ] → [@ CERT_DestroyOCSPResponse ] [@ CERT_DestroyOCSPResponse | ocsp_CacheEncodedOCSPResponse]
Comment on attachment 605999 [details] [diff] [review]
DER_GetInteger should return 0 deterministically if len is 0

r+ rrelyea
Attachment #605999 - Flags: review?(rrelyea) → review+
looks like this second patch needs to be checked in. will do so.
Shall we keep the target milestone at 3.13.4 ?

Checking in dersubr.c;
/cvsroot/mozilla/security/nss/lib/util/dersubr.c,v  <--  dersubr.c
new revision: 1.10; previous revision: 1.9
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Kai: thank you for your help.  The target milestone should remain 3.13.4.

(In general we should open a new bug for the remaining work since 3.13.4
has been released.)
You need to log in before you can comment on or make changes to this bug.