1306958 - UBSan: PR_GetUniqueIdentity(): null pointer passed as argument 2, which is declared to never be null

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

../../../../pr/src/io/prlayer.c:657:24: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:43:28: note: nonnull attribute specified here
    #0 0x7f7e71d47a8f in PR_GetUniqueIdentity /home/worker/nspr/Linux4.1_x86_64_clang-3.9_glibc_PTH_64_ASAN_DBG.OBJ/pr/src/io/../../../../pr/src/io/prlayer.c:656:13
    #1 0xa7904e in nss_test::DummyPrSocket::CreateFD(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, nss_test::Mode) /home/worker/nss/external_tests/ssl_gtest/test_io.cc:305:24
    #2 0x93103f in nss_test::TlsAgent::Init() /home/worker/nss/external_tests/ssl_gtest/./tls_agent.h:75:14
    #3 0xb44356 in nss_test::TlsConnectTestBase::Init() /home/worker/nss/external_tests/ssl_gtest/tls_connect.cc:191:3
    #4 0xb43ecd in nss_test::TlsConnectTestBase::SetUp() /home/worker/nss/external_tests/ssl_gtest/tls_connect.cc:173:3
    #5 0xd5d6ce 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
    #6 0xc4b11d 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
    #7 0xc4a151 in testing::Test::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2430:3
    #8 0xc514d2 in testing::TestInfo::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2610:11
    #9 0xc586bf in testing::TestCase::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:2728:28
    #10 0xc9540b in testing::internal::UnitTestImpl::RunAllTests() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:4591:43
    #11 0xd75c30 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
    #12 0xc9268d 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
    #13 0xc91dbe in testing::UnitTest::Run() /home/worker/nss/external_tests/google_test/gtest/src/gtest.cc:4209:10
    #14 0x91debc in RUN_ALL_TESTS() /home/worker/nss/external_tests/ssl_gtest/../../external_tests/google_test/gtest/include/gtest/gtest.h:2304:46
    #15 0x91dd42 in main /home/worker/nss/external_tests/ssl_gtest/ssl_gtest.cc:37:12
    #16 0x7f7e7148282f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #17 0x46e528 in _start (/home/worker/nss/external_tests/ssl_gtest/Linux4.1_x86_64_clang-3.9_glibc_PTH_64_ASAN_DBG.OBJ/ssl_gtest+0x46e528)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: bug-1306958-memcpy-null.patch

The problem here is PR_GetUniqueIdentity() calling memcpy() to retain existing data when called the first time.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

This might be sec-moderate after all, sorry :/

Comment 3

[Wan-Teh Chang]

Comment on attachment 8796931 [details] [diff] [review]
bug-1306958-memcpy-null.patch

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

r=wtc. I suggest a different test.

::: pr/src/io/prlayer.c
@@ +651,5 @@
>          /* we have to do something - hopefully it's already done */
>          if ((NULL != names) && (identity < length))
>          {
>              /* what we did is still okay */
> +            if (NULL != identity_cache.name) {

Please check identity_cache.length != 0 instead.

Comment 4

[Wan-Teh Chang]

Dan, Tim: This bug should not be a security bug.

If an implementation of memcpy() still dereferences the null
pointer when the number of bytes to copy is zero, we will get
a crash due to segmentation fault. This code path in
PR_GetUniqueIdentity is executed whenever NSS is used for
SSL/TLS. But we have not received any report of this crash.

Without a crash, this bug is a just pedantic warning about
how memcpy is specified in the C Standard.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Unhiding, Wan-Teh is right.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Wan-Teh Chang from comment #3)
> >          /* we have to do something - hopefully it's already done */
> >          if ((NULL != names) && (identity < length))
> >          {
> >              /* what we did is still okay */
> > +            if (NULL != identity_cache.name) {
> 
> Please check identity_cache.length != 0 instead.

Will do, thanks!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nspr/rev/18fcb78da5e4

Will set the correct target milestone once it has been added to Bugzilla.