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

RESOLVED FIXED in 3.28

Status

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

({sec-moderate})

Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 wontfix)

Details

Attachments

(1 attachment)

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)
Classifying as sec-moderate, just like bug 1221620.
Keywords: sec-moderate
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.
Attachment #8796921 - Flags: review?(franziskuskiefer)
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?
Attachment #8796921 - Flags: review?(franziskuskiefer) → review+
(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 :)
https://hg.mozilla.org/projects/nss/rev/c0bf2f17f210
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.28
Group: crypto-core-security → core-security-release
Mistakenly thought we landed 3.28.1 on the ESR-45 branch
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.