From e470f353e4b2428236bbcbbab2d38d41ccde6e00 Mon Sep 17 00:00:00 2001 From: Miklos Vajna Date: Fri, 7 Sep 2018 23:54:09 +0200 Subject: [PATCH] nss: fix undefined behavior due to too large shift 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" --- src/nss/x509.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nss/x509.c b/src/nss/x509.c index 3cdabaffb..7e713f51c 100644 --- a/src/nss/x509.c +++ b/src/nss/x509.c @@ -1747,7 +1747,9 @@ xmlSecNssASN1IntegerWrite(SECItem *num) { * NSS bug http://bugzilla.mozilla.org/show_bug.cgi?id=212864 is fixed */ for(ii = num->len; ii > 0; --ii, shift += 8) { - val |= ((PRUint64)num->data[ii - 1]) << shift; + if(num->data[ii - 1] != 0) { + val |= ((PRUint64)num->data[ii - 1]) << shift; + } } res = (xmlChar*)xmlMalloc(resLen + 1);