1234990 - heap-use-after-free of nss_test::Poller::Timer in gtest TlsConnectGeneric.ConnectResumeSupportBoth

Status: NEW

(NSS :: Test, defect, P5)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Not a security bug because this is in the test harness, not NSS itself.

While running the ssl_gtests under Address Sanitizer (Clang 3.6, Linux, x86_64, with edge coverage enabled (-fsanitize-coverage=3 and ASAN_OPTIONS=coverage=1) and with optimization, if any of that matters), I got a heap-use-after-free on one run.  It doesn't happen reliably, and I saw it only once, but I haven't actively tried to reproduce it.  I don't know if the specific test is important.  Output was:

[ RUN      ] VariantsAll/TlsConnectGeneric.ConnectResumeSupportBoth/3
Version: DTLS 1.2
server: Changing state from INIT to CONNECTING
client: Changing state from INIT to CONNECTING
client: Would have blocked
server: Would have blocked
Poll()
client: Readable
client: Would have blocked
Poll()
server: Readable
server: Handshake success
server: Changing state from CONNECTING to CONNECTED
client: Readable
client: Handshake success
client: Changing state from CONNECTING to CONNECTED
=================================================================
==87544==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000051da0 at pc 0x0000006e0c4e bp 0x7ffceb7d5d50 sp 0x7ffceb7d5d48
WRITE of size 8 at 0x603000051da0 thread T0

The allocation was here:

nss_test::Poller::SetTimer(unsigned int, nss_test::PollTarget*, void (*)(nss_test::PollTarget*, nss_test::Event), nss_test::Poller::Timer**)
/home/jld/src/NSS/nss/external_tests/ssl_gtest/test_io.cc:429
Timeout
/home/jld/src/NSS/nss/external_tests/ssl_gtest/./gtest_utils.h:20
nss_test::TlsConnectTestBase::Handshake()
/home/jld/src/NSS/nss/external_tests/ssl_gtest/tls_connect.cc:137
nss_test::TlsConnectTestBase::Connect()
/home/jld/src/NSS/nss/external_tests/ssl_gtest/tls_connect.cc:151
testing::Test::Run()
/home/jld/src/NSS/nss/external_tests/google_test/gtest/src/gtest.cc:2434
[...]

The deletion was here (where timers that have already fired are deleted):

operator delete(void*)
??:?
nss_test::Poller::Poll()
/home/jld/src/NSS/nss/external_tests/ssl_gtest/test_io.cc:487 (discriminator 1)

The misuse was here:

~Timeout
/home/jld/src/NSS/nss/external_tests/ssl_gtest/./test_io.h:97
nss_test::TlsConnectTestBase::Handshake()
/home/jld/src/NSS/nss/external_tests/ssl_gtest/tls_connect.cc:137
nss_test::TlsConnectTestBase::Connect()
/home/jld/src/NSS/nss/external_tests/ssl_gtest/tls_connect.cc:151
nss_test::TlsConnectGeneric_ConnectResumeSupportBoth_Test::TestBody()
/home/jld/src/NSS/nss/external_tests/ssl_gtest/ssl_loopback_unittest.cc:145

i.e., an nss_test::Timeout being destroyed had a pointer to the already-deleted Timer and tried to call Cancel() on it.

Fixing this might be a simple matter of making Timeout::ExpiredCallback clear handle_ (and checking it before use), but what ought to happen longer-term is converting this code to use safe pointer classes (unique_ptr, shared_ptr, etc.).