Last Comment Bug 485370 - crash, bus error due to unaligned access in pkix_pl_OcspResponse_Create
: crash, bus error due to unaligned access in pkix_pl_OcspResponse_Create
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P1 blocker (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-26 09:11 PDT by The Written Word
Modified: 2009-07-30 18:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 for NSS Trunk (3.65 KB, patch)
2009-03-27 15:59 PDT, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
wtc: superreview+
Details | Diff | Review

Description The Written Word 2009-03-26 09:11:40 PDT
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 */
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-03-27 12:50:38 PDT
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.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-03-27 15:59:43 PDT
Created attachment 369766 [details] [diff] [review]
Patch v1 for NSS Trunk

This patch cures the alignment problem.
It also fixes a bogus call with invalid arguments.
Comment 3 Robert Relyea 2009-03-27 17:07:59 PDT
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
Comment 4 Wan-Teh Chang 2009-03-27 17:16:07 PDT
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 Wan-Teh Chang 2009-03-27 17:17:45 PDT
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.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2009-03-27 18:08:18 PDT
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
Comment 7 Nelson Bolyard (seldom reads bugmail) 2009-03-27 18:16:47 PDT
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 Wan-Teh Chang 2009-03-27 21:42:04 PDT
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;
        }
Comment 9 Nelson Bolyard (seldom reads bugmail) 2009-03-27 22:36:32 PDT
This bug is now fixed.
I filed Bug 485669 about the issues in xauthkid.c.

Note You need to log in before you can comment on or make changes to this bug.