1572791 - Read out-of-bounds in DER_DecodeTimeChoice_Util from SSLExp_DelegateCredential

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[J.C. Jones [:jcj] (he/they)]

[ RUN      ] Version13Only/TlsConnectTls13.DCNotConfigured/0
Version: TLS 1.3
../../gtests/ssl_gtest/tls_agent.cc:131: Failure
Expected: (nullptr) != (cert->get()), actual: (nullptr) vs NULL
../../gtests/ssl_gtest/tls_agent.cc:165: Failure
Value of: TlsAgent::LoadCertificate(name, &cert, &cert_priv)
  Actual: false
Expected: true
AddressSanitizer:DEADLYSIGNAL
=================================================================
==33536==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000168 (pc 0x0001094ebfa2 bp 0x7ffee957c2b0 sp 0x7ffee957c280 T0)
==33536==The signal is caused by a READ memory access.
==33536==Hint: address points to the zero page.
    #0 0x1094ebfa1 in DER_DecodeTimeChoice_Util sectime.c:136
    #1 0x1074f2552 in SSLExp_DelegateCredential tls13subcerts.c:676
    #2 0x10713c6b1 in nss_test::TlsAgent::DelegateCredential(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::unique_ptr<SECKEYPublicKeyStr, ScopedMaybeDelete<SECKEYPublicKeyStr> > const&, SSLSignatureScheme, unsigned int, long long, SECItemStr*) tls_agent.cc:166
    #3 0x1072990a4 in nss_test::TlsConnectTls13_DCNotConfigured_Test::TestBody() tls_subcerts_unittest.cc:40
    #4 0x10736c984 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) gtest.cc:2443
    #5 0x1072ed803 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) gtest.cc:2479
    #6 0x1072ed2c1 in testing::Test::Run() gtest.cc:2517
    #7 0x1072f00de in testing::TestInfo::Run() gtest.cc:2693
    #8 0x1072f2bca in testing::TestCase::Run() gtest.cc:2811
    #9 0x107310138 in testing::internal::UnitTestImpl::RunAllTests() gtest.cc:5177
    #10 0x1073789a4 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) gtest.cc:2443
    #11 0x10730f202 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) gtest.cc:2479
    #12 0x10730ec8a in testing::UnitTest::Run() gtest.cc:4786
    #13 0x106d081d0 in RUN_ALL_TESTS() gtest.h:2341
    #14 0x106d080a0 in main ssl_gtest.cc:43
    #15 0x7fff6bd503d4 in start (libdyld.dylib:x86_64+0x163d4)

==33536==Register values:
rax = 0x0000000000000168  rbx = 0x00007ffee957c3c0  rcx = 0x0000000000000000  rdx = 0xfffffffffffffff8
rdi = 0x000010000000002d  rsi = 0x0000000000000168  rbp = 0x00007ffee957c2b0  rsp = 0x00007ffee957c280
 r8 = 0x0000200000000000   r9 = 0x00000fffffffffff  r10 = 0x00007ffee957e538  r11 = 0x00000001094ebf40
r12 = 0x0000000000093a80  r13 = 0x00058fb1f4efb274  r14 = 0x0000000000000000  r15 = 0x000061d00109aaa0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV sectime.c:136 in DER_DecodeTimeChoice_Util
==33536==ABORTING
Abort trap: 6

This is almost certainly due to a bad cert, but it shouldn't crash anyway.

Comment 1

[J.C. Jones [:jcj] (he/they)]

Attachment: ssl_gtests directory that prompts it - 1572791.tgz

Comment 2

[J.C. Jones [:jcj] (he/they)]

In fact, the attached nss database is missing a cert with a nickname delegator_ecdsa256, but even so, DC shouldn't prompt a crash here.

Comment 3

[J.C. Jones [:jcj] (he/they)]

The tls13subcerts.c methods don't check their inputs anywhere... so perhaps this should just be a fix to the test. However, I would prefer that the primary external entrypoint, SSLExp_DelegateCredential - which does return SECStatus - do pointer checking.

Comment 4

[J.C. Jones [:jcj] (he/they)]

I don't see any dangerous paths here, so I'm preparing a patch that just makes it simpler to tell that your tests are not right, returning test failures instead of ASAN crashes of:

Value of: TlsAgent::LoadCertificate(name, &cert, &cert_priv)
  Actual: false
Expected: true
Could not load delegate certificate: delegator_ecdsa256; test db corrupt?
../../gtests/ssl_gtest/tls_agent.cc:131: Failure

Comment 5

[J.C. Jones [:jcj] (he/they)]

On Monday I'll open this bug up, unless there's commentary otherwise.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1572791 - Check for nulls in SSLExp_DelegateCredential and its tests r=mt,kjacobs

Comment 7

[J.C. Jones [:jcj] (he/they)]

Opening up, per comment 5 - not a security bug. Just null pointer checks in tests.

Comment 8

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1572791 - Fix ASAN cert errors when SSL gtests run on empty profile

Comment 9

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/cef2aa7f3b8ce52571ac4b82242ba75cdeb756ef
https://hg.mozilla.org/projects/nss/rev/ed50678575639fdf32c7d461b343aaf5f5b4ea77