Closed
Bug 485370
Opened 15 years ago
Closed 15 years ago
crash, bus error due to unaligned access in pkix_pl_OcspResponse_Create
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.3
People
(Reporter: bugzilla-mozilla, Assigned: nelson)
Details
Attachments
(1 file)
3.65 KB,
patch
|
rrelyea
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121622 Fedora/3.0.5-1.fc10 Firefox/3.0.5 Build Identifier: Mozilla/5.0 (X11; U; HP-UX ia64; en-US; rv:1.9.0.7) Gecko/2009032613 Minefield/3.0.7 Bus error in libnss due to unaligned access. struct PKIX_PL_OcspResponseStruct has an int64 member, and therefore must be 8 byte aligned. struct PKIX_PL_ObjectStruct is 4 byte aligned, and is what is malloced. This is the same as http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=509930 Reproducible: Always Steps to Reproduce: 1.build firefox 3 on a platform that requires strict alignment 2.run it, and type https://www.facebook.com in to the address bar 3.crash Backtrace: #0 pkix_pl_OcspResponse_Create () at pkix_pl_ocspresponse.c:531 #1 0x60000000cd520bf0:0 in pkix_OcspChecker_Check () at pkix_ocspchecker.c:311 #2 0x60000000cd552570:0 in pkix_RevCheckCert () at pkix_validate.c:339 Line 531 of pkix_pl_ocspresponse.c is ocspResponse->producedAt = 0; producedAt is an int64, so must be 8 byte aligned. This problem can be fixed by ensuring that PKIX_PL_OcspResponseStruct is 8 byte aligned. e.g. Index: security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h =================================================================== --- security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h.orig 2009-03-26 14:08:42.514199707 +0000 +++ security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.h 2009-03-26 16:09:20.420673751 +0000 @@ -78,6 +78,7 @@ PKIX_PL_String *stringRep; PKIX_UInt32 hashcode; PKIX_Boolean hashcodeCached; + int64 pad; }; /* see source file for function documentation */
Reporter | ||
Updated•15 years ago
|
OS: Other → HP-UX
Hardware: Other → HP
Assignee | ||
Comment 1•15 years ago
|
||
What version of NSS is exhibiting this problem, exactly? NSS 3.??.?? Is your version of NSS a debian-modified version of NSS? Or is it one that comes straight from Mozilla without any Debian modifications? I ask this only to raise your awareness that, even when we fix this in NSS, the fix will not immediately be available to users of versions of NSS that have been modified by downstream Linux distros. I am puzzled about one thing. We build and test NSS daily on numerous platforms that have the same alignment requirements as iTanium, such as UltraSparc, HP PA Risc, PowerPC for AIX, but we don't see this crash on any of those. I wonder why not. It does look to me like it should be crashing on them all. And this is not new code. This is not a regression in NSS 3.12.3 (I believe). The relevant code is much older than that.
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → nelson
Severity: normal → blocker
Priority: -- → P1
Target Milestone: --- → 3.12.3
Assignee | ||
Comment 2•15 years ago
|
||
This patch cures the alignment problem. It also fixes a bogus call with invalid arguments.
Attachment #369766 -
Flags: superreview?(wtc)
Attachment #369766 -
Flags: review?(rrelyea)
Comment 3•15 years ago
|
||
Comment on attachment 369766 [details] [diff] [review] Patch v1 for NSS Trunk r+ rrelyea cute... the good string even reads as english on LE machines. bob
Attachment #369766 -
Flags: review?(rrelyea) → review+
Updated•15 years ago
|
Attachment #369766 -
Flags: superreview?(wtc) → superreview+
Comment 4•15 years ago
|
||
Comment on attachment 369766 [details] [diff] [review] Patch v1 for NSS Trunk The change to lib/certdb/xauthkid.c is completely unrelated to the alignment problem of struct PKIX_PL_ObjectStruct. The root cause of the lib/certdb/xauthkid.c problem is that value->DERAuthCertIssuer is NULL after SEC_QuickDERDecodeItem returns successfully. Isn't that a bug? If that's not a bug, do we want to return NULL, or return a value with a NULL value->authCertIssuer? Also, do we need to set rv to SECFailure before breaking out of the do-while? if (value->authCertIssuer == NULL) break; /* what if the general name contains other format but not URI ? hl */ if ((value->authCertSerialNumber.data && !value->authCertIssuer) || (!value->authCertSerialNumber.data && value->authCertIssuer)){ PORT_SetError (SEC_ERROR_EXTENSION_VALUE_INVALID); break; } The fix for the alignment problem is fine. Strictly speaking, LL_INIT can only be used in static initializers (hence the _INIT in the name), but it'll only fail to compile if HAVE_LONG_LONG is not defined. >@@ -83,7 +83,7 @@ static PKIX_Alloc_Error_Object pkix_Allo > > PKIX_Error* PKIX_ALLOC_ERROR(void) > { >- return &pkix_Alloc_Error_Data.error; >+ return (PKIX_Error *)&pkix_Alloc_Error_Data.error; > } Please add a comment to note the purpose of the cast is to cast away const.
Comment 5•15 years ago
|
||
Comment on attachment 369766 [details] [diff] [review] Patch v1 for NSS Trunk > struct PKIX_PL_ObjectStruct { >- PKIX_UInt32 magicHeader; >+ PRUint64 magicHeader; Please also add a comment to note that this is declared PRUint64 to force alignment.
Assignee | ||
Comment 6•15 years ago
|
||
pkix/util/pkix_tools.h; new revision: 1.24; previous: 1.23 pkix_pl_nss/system/pkix_pl_lifecycle.c; new revision: 1.23; previous: 1.22 pkix_pl_nss/system/pkix_pl_object.h; new revision: 1.6; previous: 1.5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 7•15 years ago
|
||
Hmm. The emails for Wan-Teh's comment 4 and comment 5 have STILL not reached my mailbox, even though they're an hour old now. I didn't see them until after I committed the patch. In answer to some of the questions Wan-Teh asked in comment 4, > The root cause of the lib/certdb/xauthkid.c problem is that > value->DERAuthCertIssuer is NULL after SEC_QuickDERDecodeItem > returns successfully. Isn't that a bug? No, it's not. Those fields are formally optional. The decoder signifies missing optional fields by setting the pointer to NULL. It's not an error for option fields to be missing. > If that's not a bug, do we want to return NULL, or return a value with > a NULL value->authCertIssuer? The latter. The code was/is already returning the right value, but it is causing an error code to be set when no error has occurred. This causes the wrong errors to be reported under some conditions, the very conditions under which the reported bug was experienced. I think there's already another bug filed about getting INVALID_ARGS error codes from cert validations. I'll see if I can find it, to see if this patch fixes that problem, or if it is a different problem.
Comment 8•15 years ago
|
||
Thanks. You should also clean up this code in xauthkid.c: if (value->authCertIssuer == NULL) break; /* what if the general name contains other format but not URI ? hl */ if ((value->authCertSerialNumber.data && !value->authCertIssuer) || (!value->authCertSerialNumber.data && value->authCertIssuer)){ PORT_SetError (SEC_ERROR_EXTENSION_VALUE_INVALID); break; } After the first if statement, value->authCertIssuer cannot be NULL, so the second if statement can be simplified to: if (!value->authCertSerialNumber.data) { rv = SECFailure; <=== ADD THIS PORT_SetError (SEC_ERROR_EXTENSION_VALUE_INVALID); break; } Also, the first if statement needs more thought. value->authCertIssuer can be NULL for two reasons. 1. value->DERAuthCertIssuer is NULL. You said this is not a bug. 2. cert_DecodeGeneralNames (arena, value->DERAuthCertIssuer) fails. It seems that this should be handled as a failure, because either the DER is bad, or we run out of arena memory. So it seems that the code should look like this: if (value->DERAuthCertIssuer) { value->authCertIssuer = cert_DecodeGeneralNames (arena, value->DERAuthCertIssuer); if (value->authCertIssuer == NULL) { rv = SECFailure; break; } } /* what if the general name contains other format but not URI ? hl */ if ((value->authCertSerialNumber.data && !value->authCertIssuer) || (!value->authCertSerialNumber.data && value->authCertIssuer)){ rv = SECFailure; <=== ADD THIS PORT_SetError (SEC_ERROR_EXTENSION_VALUE_INVALID); break; }
Assignee | ||
Comment 9•15 years ago
|
||
This bug is now fixed. I filed Bug 485669 about the issues in xauthkid.c.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Version: unspecified → 3.12
Assignee | ||
Updated•15 years ago
|
OS: HP-UX → All
Hardware: HP → All
You need to log in
before you can comment on or make changes to this bug.
Description
•