Closed
Bug 697420
Opened 13 years ago
Closed 12 years ago
crash in [@ CERT_DestroyOCSPResponse]
Categories
(NSS :: Libraries, defect, P1)
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)
2.31 KB,
patch
|
rrelyea
:
review+
KaiE
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
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
Updated•13 years ago
|
Crash Signature: CERT_DestroyOCSPResponse → [@ CERT_DestroyOCSPResponse ]
Updated•13 years ago
|
Severity: normal → critical
Updated•12 years ago
|
Depends on: CVE-2012-0441
Assignee | ||
Comment 1•12 years ago
|
||
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
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Comment 3•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
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)
Updated•12 years ago
|
Crash Signature: [@ CERT_DestroyOCSPResponse ] → [@ CERT_DestroyOCSPResponse ]
[@ CERT_DestroyOCSPResponse | ocsp_CacheEncodedOCSPResponse]
Comment 9•12 years ago
|
||
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+
Comment 10•12 years ago
|
||
looks like this second patch needs to be checked in. will do so.
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
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.
Description
•