nss: fix undefined behavior due to too large shift

[vmiklos]

When building with clang -fsanitize=undefined, ubsan says:

x509.c:1750:46: runtime error: shift exponent 64 is too large for 64-bit type 'PRUint64' (aka 'unsigned long')
#0 0x444d45 in xmlSecNssASN1IntegerWrite src/nss/x509.c:1750:46
#1 0x4443ec in xmlSecNssX509IssuerSerialNodeWrite src/nss/x509.c:1259:11
#2 0x4403ba in xmlSecNssKeyDataX509XmlWrite src/nss/x509.c:769:19
#3 0x45962a in xmlSecKeyInfoNodeWrite src/keyinfo.c:180:19
#4 0x480149 in xmlSecDSigCtxProcessKeyInfoNode src/xmldsig.c:807:15
#5 0x47c774 in xmlSecDSigCtxProcessSignatureNode src/xmldsig.c:506:11
#6 0x47bfb2 in xmlSecDSigCtxSign src/xmldsig.c:289:11

And indeed shifting a 64bit value by 64 bits happens in practice there
as num->len is 9. At the same time (at least in case of the test) in all
3 cases the value that would be shifted is 0.

Avoid undefined behavior by simply not shifting if the value is 0
anyway.

Testcase: make check-crypto-nss XMLSEC_TEST_NAME="aleksey-xmldsig-01/x509data-sn-test"

[vmiklos]

This is the full asan+ubsan config I tested, BTW:

export ASAN_OPTIONS=strip_path_prefix=$(pwd)/:handle_ioctl=1:detect_leaks=1:allow_user_segv_handler=1:use_sigaltstack=0:detect_deadlocks=1:intercept_tls_get_addr=1:check_initialization_order=1:detect_stack_use_after_return=1:strict_init_order=1:detect_invalid_pointer_pairs=1
export LSAN_OPTIONS=print_suppressions=0:report_objects=1
export UBSAN_OPTIONS=print_stacktrace=1
export CC="clang -fno-sanitize-recover=undefined,integer -fsanitize=address -fsanitize=undefined -fsanitize=local-bounds"

but it only found this single problem in shared code + the NSS backend.

[lsh123]

If this is a fix, then I believe it is a false positive. The (0 << shift) is harmless.

[lsh123]

SECItem.data is unsigned char (8 bits) and len <= 9; thus we have at most 17 bits total after the shift. Thus this is a false positive.

[lsh123]

https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/util/seccomon.h#48

[vmiklos]

Oh, this is interesting. :-)

First, possibly I'm confused, but I don't get your hint with the 17 bits. Here is how I interpret the code: SECItem.len states the size of SECItem.data in bytes. .data is a char pointer, not a char. This means that if len is <=8 then the result can be nicely converted to an 64bit unsigned int, otherwise we loose the remaining data, since "value << largenum" will not be representable with a 64bit int.

Second, I was wrong, I thought that shifting with a too large number is undefined behavior, but indeed e.g. here it's confirmed that's not the case (as you say it).

So based on the above, I guess: the 9th (most significant) byte in the ASN1 integer can be represented in the 64 bit value only in case it's 0, otherwise a data loss will happen. If this is true, then I find the assert about len <= 9 weird. Why not len <= 8? Or just don't assert if we're OK with silently loosing 9th and higher bytes. Just thinking loudly... :-)

[vmiklos]

Or third option to improve the situation here: don't assert the len, but assert that only the first 8 bytes have values which are != 0. That would keep the current tests passing, but it would not allow silent data loss. I think that would be an improvement and would eliminate this odd "9" magic allowed length. Does that make sense?

[lsh123]

Yes you are correct -- the assert is weird/wrong or the code should force later bytes to be 0.

Otherwise, it should be possible to build SECItem --> MP --> decimal string conversion similar to the code in the https://bugzilla.mozilla.org/show_bug.cgi?id=212864 (also see https://github.com/servo/nss/blob/master/lib/freebl/mpi/mpi.h)

1. Call mp_read_unsigned_octets() to convert SECItem --> MP
2. CAll mp_todecimal() to convert MP --> decimal string (also probably need something like mp_radix_size() to get the size to allocate).

[vmiklos]

Ugh, I prefer to keep it simple and just replace the assert with a better one. :-)

See #222