1306948 - UBSan: DER_GetInteger_Util(): left shift of 36028797018963968 by 8 places cannot be represented in type 'long'

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

dersubr.c:207:21: runtime error: left shift of 36028797018963968 by 8 places cannot be represented in type 'long'
    #0 0x7f56c2d8c9f5 in DER_GetInteger_Util /home/worker/nss/lib/util/dersubr.c:207:21
    #1 0x4f6527 in nss_test::DERIntegerDecodingTest::TestGetInteger(long, unsigned char*, unsigned int) /home/worker/nss/external_tests/der_gtest/der_getint_unittest.cc:23:5
    #2 0x4f82b7 in nss_test::DERIntegerDecodingTest_DecodeLongMin_Test::TestBody() /home/worker/nss/external_tests/der_gtest/der_getint_unittest.cc:75:3
    #3 0x67fcee in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2362:10
    #4 0x565ebd in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2398:14
    #5 0x565210 in testing::Test::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2434:5
    #6 0x56c542 in testing::TestInfo::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2610:11
    #7 0x57372f in testing::TestCase::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2728:28
    #8 0x5b081b in testing::internal::UnitTestImpl::RunAllTests() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:4591:43
    #9 0x698560 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2362:10
    #10 0x5ada9d in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2398:14
    #11 0x5ad1ce in testing::UnitTest::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:4209:10
    #12 0x6b9dbc in RUN_ALL_TESTS() /home/worker/nss/external_tests/common/../../external_tests/google_test/gtest/include/gtest/gtest.h:2304:46
    #13 0x6b9c3c in main /home/worker/nss/external_tests/common/gtests.cc:15:12
    #14 0x7f56c1b8082f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #15 0x428088 in _start (/home/worker/nss/external_tests/der_gtest/Linux4.1_x86_64_clang-3.9_glibc_PTH_64_ASAN_DBG.OBJ/der_gtest+0x428088)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Classifying as sec-moderate, just like bug 1221620.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1306948-Make-DER-integer-decoding-a-little-easer.patch

The first thing I did was change |ival| to be an unsigned long and then basically cast it back to long when we return it. I wrote a few more tests because I saw a few cases that weren't covered and what failed was parsing a long negative integer with a lot of "padding" 0xff bytes in the front. BER, not DER, but it looks like the function is supposed to handle that too. It should at least do that correctly if we do it for positive numbers already.

I found the bitmasks and all the converting at the bottom quite heavy to read and tried rewriting the function to something more readable and hopefully easier to understand.

The gtests.sh fix lets us fail when one of the "gtests" tests fails. The echo didn't seem necessary and without it "$?" refers to the exit status we're actually interested in.

Comment 3

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8796921 [details] [diff] [review]
0001-Bug-1306948-Make-DER-integer-decoding-a-little-easer.patch

Review of attachment 8796921 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/util/dersubr.c
@@ +189,5 @@
>          return 0;
>      }
>  
> +    negative = (PRBool)(*cp & 0x80);
> +    ival = negative ? -1 : 0;

I think I'd prefer something like ~0, assigning -1 to an unsigned looks strange.

::: tests/gtests/gtests.sh
@@ -51,5 @@
>      GTESTREPORT="$GTESTDIR/report.xml"
>      PARSED_REPORT="$GTESTDIR/report.parsed"
>      echo "executing $i"
>      ${BINDIR}/$i -d "$GTESTDIR" --gtest_output=xml:"${GTESTREPORT}"
> -    echo "test output dir: ${GTESTREPORT}"

Why don't you want to know where this directory is?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3)
> > +    negative = (PRBool)(*cp & 0x80);
> > +    ival = negative ? -1 : 0;
> 
> I think I'd prefer something like ~0, assigning -1 to an unsigned looks
> strange.

Ok, will do.

> ::: tests/gtests/gtests.sh
> @@ -51,5 @@
> >      GTESTREPORT="$GTESTDIR/report.xml"
> >      PARSED_REPORT="$GTESTDIR/report.parsed"
> >      echo "executing $i"
> >      ${BINDIR}/$i -d "$GTESTDIR" --gtest_output=xml:"${GTESTREPORT}"
> > -    echo "test output dir: ${GTESTREPORT}"
> 
> Why don't you want to know where this directory is?

As explained on IRC I removed this because the line below checks $? for the exit status. But instead of removing I'll move it down :)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/c0bf2f17f210

Comment 6

[Daniel Veditz [:dveditz]]

Mistakenly thought we landed 3.28.1 on the ESR-45 branch