Skip to content

Commit

Permalink
nss: fix undefined behavior due to too large shift (#221)
Browse files Browse the repository at this point in the history
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"
  • Loading branch information
vmiklos authored and lsh123 committed Sep 8, 2018
1 parent 9ec370e commit 10c9708
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/nss/x509.c
Expand Up @@ -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);
Expand Down

0 comments on commit 10c9708

Please sign in to comment.