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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Tyson Smith [:tsmith]]

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

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Mass cc to get some NSS eyes on these bugs.

Comment 2

[Ryan Sleevi]

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?

Comment 3

[Wan-Teh Chang]

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'.)

Comment 4

[Tyson Smith [:tsmith]]

Sure thing. I will test it when I return from PTO next week.

Comment 5

[Ryan Sleevi]

Tyson: Any update? :)

Comment 6

[Tyson Smith [:tsmith]]

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

Comment 7

[Tyson Smith [:tsmith]]

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.

Comment 8

[Tyson Smith [:tsmith]]

The command that lead to this was:
pk12util -i ../stapling/ca.p12 -k ../tests.pw -w ../tests.pw -d ../stapling

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Tyson Smith [:tsmith]]

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)

Comment 11

[Wan-Teh Chang]

Attachment: 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.

Comment 12

[Wan-Teh Chang]

Attachment: Work around the bug of LL_INIT() with a first argument whose most significant bit is set

Comment 13

[Wan-Teh Chang]

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

Comment 14

[Franziskus Kiefer [:franziskus]]

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

Comment 15

[Wan-Teh Chang]

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

Comment 16

[Wan-Teh Chang]

This bug should not be marked as a security bug.

Comment 17

[Franziskus Kiefer [:franziskus]]

agree, we should open up this bug