697420 - crash in [@ CERT_DestroyOCSPResponse]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wolfgang Rosenauer [:wolfiR]]

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

Comment 1

[Wan-Teh Chang]

I studied the code related to the call stack carefully.  I will
attach a patch that should fix this crash.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch

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'.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch v2 (checked in ocsp.c only)

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.

Comment 4

[Kai Engert [:KaiE:]]

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 5

[Kai Engert [:KaiE:]]

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

Comment 6

[Robert Relyea]

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

Comment 7

[Wan-Teh Chang]

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

Comment 8

[Wan-Teh Chang]

Attachment: DER_GetInteger should return 0 deterministically if len is 0

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.

Comment 9

[Robert Relyea]

Comment on attachment 605999 [details] [diff] [review]
DER_GetInteger should return 0 deterministically if len is 0

r+ rrelyea

Comment 10

[Kai Engert [:KaiE:]]

looks like this second patch needs to be checked in. will do so.

Comment 11

[Kai Engert [:KaiE:]]

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

Comment 12

[Wan-Teh Chang]

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