1581986 - left shift of 128 by 24 places cannot be represented in type 'int' in security/manager/ssl/md4.c:68:28

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Tyson Smith [:tsmith]]

This is triggered when running gtests with an UBSan build.

To enable this check add the following to your mozconfig:

ac_add_options --enable-address-sanitizer
ac_add_options --enable-undefined-sanitizer="shift"
ac_add_options --disable-jemalloc

Traceback:

[----------] 7 tests from psm_MD4/psm_MD4
[ RUN      ] psm_MD4/psm_MD4.RFC1320TestValues/0
[       OK ] psm_MD4/psm_MD4.RFC1320TestValues/0 (0 ms)
[ RUN      ] psm_MD4/psm_MD4.RFC1320TestValues/1
[       OK ] psm_MD4/psm_MD4.RFC1320TestValues/1 (0 ms)
[ RUN      ] psm_MD4/psm_MD4.RFC1320TestValues/2
security/manager/ssl/md4.c:68:28: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'
    #0 0x7fc8f7073372 in b2w security/manager/ssl/md4.c:68:28
    #1 0x7fc8f70721c9 in md4step security/manager/ssl/md4.c:76:3
    #2 0x7fc8f7071ef6 in md4sum security/manager/ssl/md4.c:174:3
    #3 0x7fc8ed6670c8 in psm_MD4_RFC1320TestValues_Test::TestBody() security/manager/ssl/tests/gtest/MD4Test.cpp:55:3
    #4 0x7fc8ecd7e55b in testing::Test::Run() testing/gtest/gtest/src/gtest.cc:2519:5
    #5 0x7fc8ecd7f405 in testing::TestInfo::Run() testing/gtest/gtest/src/gtest.cc:2695:11
    #6 0x7fc8ecd7fbf1 in testing::TestCase::Run() testing/gtest/gtest/src/gtest.cc:2813:28
    #7 0x7fc8ecd8d642 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/gtest/src/gtest.cc:5179:43
    #8 0x7fc8ecd8d106 in testing::UnitTest::Run() testing/gtest/gtest/src/gtest.cc:4788:10
    #9 0x7fc8ecdc64f7 in mozilla::RunGTestFunc(int*, char**) testing/gtest/mozilla/GTestRunner.cpp:158:10
    #10 0x7fc8f75a1d20 in XREMain::XRE_mainStartup(bool*) toolkit/xre/nsAppRunner.cpp:3788:16
    #11 0x7fc8f75aa939 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4722:12
    #12 0x7fc8f75ab3a1 in XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4816:21
    #13 0x55f679367cae in do_main(int, char**, char**) browser/app/nsBrowserApp.cpp:218:22
    #14 0x55f679367344 in main browser/app/nsBrowserApp.cpp:300:16

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://searchfox.org/mozilla-central/rev/e18057a9613ffda06dfd3640209ca234ed7dc37d/security/manager/ssl/md4.c#68:
(uint32_t)(bp[3] << 24);
bp is const uint8_t*. If I understand this error message, it's saying that the integer promotion of uint8_t is (or can be?) int, which if we have 32-bit ints, left-shifting values >=128 by 24 can't be represented, so this is undefined behavior. The obvious solution is to do the cast before shifting.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: bug 1581986 - fix undefined shift behavior in md4 implementation r?kjacobs

Using left shift on a uint8_t promotes it to a signed integer. If the shift is
large enough that the sign bit gets affected, we have undefined behavior. This
patch fixes this by first casting to uint32_t.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f2ff84ff3ee
fix undefined shift behavior in md4 implementation r=kjacobs

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9f2ff84ff3ee