Closed
Bug 1180095
Opened 9 years ago
Closed 8 years ago
UBSAN left shift of 4276994303 by 32 places cannot be represented in type 'long'
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
Tracking
(firefox42 affected)
RESOLVED
FIXED
3.22
Tracking | Status | |
---|---|---|
firefox42 | --- | affected |
People
(Reporter: tsmith, Assigned: wtc)
References
(Blocks 1 open bug)
Details
(Keywords: sec-audit)
Attachments
(3 files)
1.69 KB,
patch
|
Details | Diff | Splinter Review | |
590 bytes,
patch
|
Details | Diff | Splinter Review | |
1.40 KB,
patch
|
franziskus
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
This was found by running the test suite with an Undefined Behavior Sanitizer (UBSAN) build. pkix_pl_object.c:580:31: runtime error: left shift of 4276994303 by 32 places cannot be represented in type 'long' #0 0x7f8dda82f7ff in PKIX_PL_Object_Alloc /home/user/Desktop/nss_ubsan/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c:580 (discriminator 5) #1 0x7f8dda81d2bd in PKIX_PL_HashTable_Create /home/user/Desktop/nss_ubsan/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c:149 (discriminator 1) #2 0x7f8dda61e69f in PKIX_Initialize /home/user/Desktop/nss_ubsan/nss/lib/libpkix/pkix/top/pkix_lifecycle.c:91 (discriminator 1) #3 0x7f8dda252ee3 in nss_Init /home/user/Desktop/nss_ubsan/nss/lib/nss/nssinit.c:688 #4 0x7f8dda253781 in NSS_Initialize /home/user/Desktop/nss_ubsan/nss/lib/nss/nssinit.c:813 #5 0x43e3c5 in certutil_main /home/user/Desktop/nss_ubsan/nss/cmd/certutil/certutil.c:2878 #6 0x435106 in main /home/user/Desktop/nss_ubsan/nss/cmd/certutil/certutil.c:3565 #7 0x7f8dd8e27ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287 #8 0x407e50 in _start ??:?
Mass cc to get some NSS eyes on these bugs.
Comment 2•9 years ago
|
||
Is there actual documentation for reproducing this with UBSAN? Also, can you provide more details about your setup - were you testing on Linux, Mac, Windows? 32-bit or 64-bit? https://hg.mozilla.org/projects/nss/annotate/6c43fe3ab5dd/lib/libpkix/pkix_pl_nss/system/pkix_pl_object.c#l580 contains line 580 at time of report. PKIX_MAGIC_HEADER is a #define, http://mxr.mozilla.org/nss/source/lib/libpkix/pkix/util/pkix_tools.h#1461 LL_INIT comes from NSPR, which is defined at http://mxr.mozilla.org/nspr/source/pr/include/prlong.h#37 The relevant #define should only be triggered if BYTES_PER_LONG is 8, at which point, it should be defined behaviour. Was there perhaps a misconfiguration when setting up UBSAN, such that ./configure reported PR_BYTES_PER_LONG as 8, but it was compiled at 4?
Assignee | ||
Comment 3•9 years ago
|
||
Thanks for the analysis, Ryan. The stack trace in comment 0 shows several 64-bit addresses, so it looks like a 64-bit build, and the pathnames of the source files show the build was most likely done on a Linux platform (that uses eglibc-2.19). Note that 4276994303 is 0xFEEDC0FF. The most significant bit is set. Since 'long' is a signed type, I wonder if UBSAN doesn't like us to shift a one bit into the sign bit. I remember this is defined in the C standard. Tyson: please change http://mxr.mozilla.org/nss/source/lib/libpkix/pkix/util/pkix_tools.h#1461 from #define PKIX_MAGIC_HEADER LL_INIT(0xFEEDC0FF, 0xEEFACADE) to #define PKIX_MAGIC_HEADER LL_INIT(0x7EEDC0FF, 0xEEFACADE) and see if this fixes the UBSAN error. Thanks. (Note the 'F' is changed to a '7'.)
Reporter | ||
Updated•9 years ago
|
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Reporter | ||
Comment 4•9 years ago
|
||
Sure thing. I will test it when I return from PTO next week.
Flags: needinfo?(twsmith)
Updated•9 years ago
|
Group: core-security → crypto-core-security
Comment 5•9 years ago
|
||
Tyson: Any update? :)
Reporter | ||
Comment 6•9 years ago
|
||
I guess I hit snooze on this one for too long. Sorry I'll have a look today :)
Reporter | ||
Comment 7•9 years ago
|
||
I tested with: #define PKIX_MAGIC_HEADER LL_INIT(0x7EEDC0FF, 0x7EFACADE) #define PKIX_MAGIC_HEADER_DESTROYED LL_INIT(0x7AADF00D, 0x7EADBEEF) That seemed to make UBSsan happy, I did not the error.
Flags: needinfo?(twsmith)
Reporter | ||
Comment 8•9 years ago
|
||
The command that lead to this was: pk12util -i ../stapling/ca.p12 -k ../tests.pw -w ../tests.pw -d ../stapling
Assignee | ||
Comment 9•9 years ago
|
||
Tyson: how do I do a UBSAN build of NSS? It would be more efficient for the same person (either you or me) to do a UBSAN build and experiment with various fixes. Thanks. In the experiment I proposed in comment 3, only the first argument of LL_INIT() needs to be modified. The second argument of LL_INIT() should still start with 0xE or 0xD. Could you please try that experiment again? There are three possible fixes. 1. What I proposed in comment 3. Note that the bug in LL_INIT() still remains. 2. Fix the LL_INIT() macro. This is more work than the other two options and is not worth our time in my opinion. 3. Consider the LL_INIT() macro as an obsolete feature and change NSS to use PR_UINT64 instead: #define PKIX_MAGIC_HEADER PR_UINT64(0xFEEDC0FFEEFACADE) #define PKIX_MAGIC_HEADER_DESTROYED PR_UINT64(0xBAADF00DDEADBEEF) I propose we do #3. But I'd appreciate if you could repeat your experiment of #1 correctly.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(nobody)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(nobody) → needinfo?(twsmith)
Reporter | ||
Comment 10•9 years ago
|
||
Wan-Teh: I have tested both fixes as requested in comment 9. Both tests passed, the error was not reported by UBSan. Note: for #1 I used the the following values as requested. #define PKIX_MAGIC_HEADER LL_INIT(0x7EEDC0FF, 0xEEFACADE) #define PKIX_MAGIC_HEADER_DESTROYED LL_INIT(0x7AADF00D, 0xDEADBEEF)
Flags: needinfo?(twsmith)
Assignee | ||
Comment 11•9 years ago
|
||
This is the second fix I described in comment 9. I verified it works. As the patch shows, it may break existing workarounds for this bug and require undoing the workarounds.
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
The LL_INIT() macro was defined when not all compilers supported a 64-bit integer type. Now that all compilers support long long, we can use the better PR_INT64() and PR_UINT64() macros. When reviewing the changes for lib/util/dertime.c, please note that the PRTime type is defined as PRInt64, and the code I modified was added in this checkin: https://hg.mozilla.org/projects/nss/rev/74da4fc148eb
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Attachment #8701207 -
Flags: superreview?(kaie)
Attachment #8701207 -
Flags: review?(franziskuskiefer)
Comment 14•9 years ago
|
||
Comment on attachment 8701207 [details] [diff] [review] Use PR_INT64() or PR_UINT64() instead of LL_INIT() Review of attachment 8701207 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8701207 -
Flags: review?(franziskuskiefer) → review+
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8701207 [details] [diff] [review] Use PR_INT64() or PR_UINT64() instead of LL_INIT() Review of attachment 8701207 [details] [diff] [review]: ----------------------------------------------------------------- https://hg.mozilla.org/projects/nss/rev/5d9f8b809e6f
Attachment #8701207 -
Flags: checked-in+
Assignee | ||
Comment 16•8 years ago
|
||
This bug should not be marked as a security bug.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
OS: Linux → All
Priority: -- → P2
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → 3.22
Updated•8 years ago
|
Group: crypto-core-security → core-security-release
Updated•8 years ago
|
Attachment #8701207 -
Flags: superreview?(kaie)
Updated•7 years ago
|
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in
before you can comment on or make changes to this bug.
Description
•