UBSAN left shift of 4276994303 by 32 places cannot be represented in type 'long'

RESOLVED FIXED in 3.22

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: tsmith, Assigned: Wan-Teh Chang)

Tracking

(Blocks: 1 bug, {sec-audit})

trunk
3.22
sec-audit

Firefox Tracking Flags

(firefox42 affected)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
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

3 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

3 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

3 years ago
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
(Reporter)

Comment 4

3 years ago
Sure thing. I will test it when I return from PTO next week.
Flags: needinfo?(twsmith)

Updated

3 years ago
Group: core-security → crypto-core-security

Comment 5

3 years ago
Tyson: Any update? :)
(Reporter)

Comment 6

3 years ago
I guess I hit snooze on this one for too long. Sorry I'll have a look today :)
(Reporter)

Comment 7

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

Updated

3 years ago
Keywords: sec-audit
(Reporter)

Updated

3 years ago
See Also: → bug 809813
(Reporter)

Comment 8

3 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

3 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

3 years ago
Flags: needinfo?(nobody)
(Reporter)

Updated

3 years ago
Flags: needinfo?(nobody) → needinfo?(twsmith)
(Reporter)

Comment 10

3 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

2 years ago
Created attachment 8701172 [details] [diff] [review]
Just FYI: Fix the LL_INIT() macro

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

2 years ago
Created attachment 8701193 [details] [diff] [review]
Work around the bug of LL_INIT() with a first argument whose most significant bit is set
(Assignee)

Comment 13

2 years ago
Created attachment 8701207 [details] [diff] [review]
Use PR_INT64() or PR_UINT64() instead of LL_INIT()

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

2 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

2 years ago
This bug should not be marked as a security bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
OS: Linux → All
Priority: -- → P2
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → 3.22
agree, we should open up this bug
Flags: needinfo?(dveditz)
Group: crypto-core-security → core-security-release

Updated

2 years ago
Attachment #8701207 - Flags: superreview?(kaie)
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.