1276192 - UAF in TlsAgent cancelling timer

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Martin Thomson [:mt:]]

Attachment: tmp

See https://public-artifacts.taskcluster.net/BzlGtG_FRfieTdtQC40I0A/0/public/logs/live_backing.log

The reason that this happens is a little convoluted.  In DTLS tests, we set a timer based on whatever DTLS_GetHandshakeTimeout() says.  Before that, we cancel the old timer, but fail to null it.  If DTLS_GetHandshakeTimeout() fails, we fail to update the timer_handle_ value in the TlsAgent.  Because when the timer runs, it deletes the object we reference.  But we still have a handle and we then try to cancel again when the TlsAgent is destructed.

Note that DTLS_GetHandshakeTimeout() fails often because it fails when we don't have a timeout set.  That's all the time because once the handshake is complete, only the side that has a holddown timer has any timers running.

Null the pointer when we cancel and it should be OK.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8757256 [details] [diff] [review]
tmp

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

Thanks!

Comment 2

[Franziskus Kiefer [:franziskus]]

This still fails for me locally with the same error

==29194==ERROR: AddressSanitizer: heap-use-after-free on address 0x6030000729b0 at pc 0x000000428401 bp 0x7ffc82d6b6e0 sp 0x7ffc82d6b6d0
WRITE of size 8 at 0x6030000729b0 thread T0
    #0 0x428400 in nss_test::Poller::Timer::Cancel() /home/franziskus/Code/nss/external_tests/ssl_gtest/test_io.h:105
    #1 0x4290ef in nss_test::Timeout::Cancel() /home/franziskus/Code/nss/external_tests/ssl_gtest/gtest_utils.h:32
    #2 0x42905b in nss_test::Timeout::~Timeout() /home/franziskus/Code/nss/external_tests/ssl_gtest/gtest_utils.h:24
    #3 0x4bedf3 in nss_test::TlsConnectTestBase::Receive(unsigned long) /home/franziskus/Code/nss/external_tests/ssl_gtest/tls_connect.cc:376
    #4 0x4bec72 in nss_test::TlsConnectTestBase::SendReceive() /home/franziskus/Code/nss/external_tests/ssl_gtest/tls_connect.cc:372
    #5 0x48d357 in nss_test::TlsCipherSuiteTest_ConnectWithCipherSuite_Test::TestBody() /home/franziskus/Code/nss/external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:77
    #6 0x505748 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) gtest/src/gtest.cc:2362
    #7 0x4fc75b in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) gtest/src/gtest.cc:2398
    #8 0x4dc503 in testing::Test::Run() gtest/src/gtest.cc:2435
    #9 0x4dd0ab in testing::TestInfo::Run() gtest/src/gtest.cc:2610
    #10 0x4dd945 in testing::TestCase::Run() gtest/src/gtest.cc:2728
    #11 0x4e84d0 in testing::internal::UnitTestImpl::RunAllTests() gtest/src/gtest.cc:4591
    #12 0x506f70 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) gtest/src/gtest.cc:2362
    #13 0x4fde05 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) gtest/src/gtest.cc:2398
    #14 0x4e6159 in testing::UnitTest::Run() gtest/src/gtest.cc:4209
    #15 0x495131 in RUN_ALL_TESTS() ../../external_tests/google_test/gtest/include/gtest/gtest.h:2304
    #16 0x49528b in main /home/franziskus/Code/nss/external_tests/ssl_gtest/ssl_gtest.cc:33
    #17 0x7fe908fc3740 in __libc_start_main (/usr/lib/libc.so.6+0x20740)
    #18 0x410208 in _start (/home/franziskus/Code/nss/external_tests/ssl_gtest/Linux4.5_x86_64_cc_glibc_PTH_64_ASAN_DBG.OBJ/ssl_gtest+0x410208)

Comment 3

[Martin Thomson [:mt:]]

What do you need to run with ASAN and I'll try it myself.  I want to use rr.

BTW, I checked it in anyway: https://hg.mozilla.org/projects/nss/rev/d0f1c1030393

Comment 4

[Martin Thomson [:mt:]]

Attachment: tmp2

Franziskus, that's a different error to the one I looked at.  That's a worse one in a sense.  The problem there is that the timeout handle is being hit after it timed out and was deleted.  Try this please.

Comment 5

[Martin Thomson [:mt:]]

Attachment: tmp3

This is nicer, I think.

Comment 6

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #3)
> What do you need to run with ASAN and I'll try it myself.  I want to use rr.
> 
> BTW, I checked it in anyway:
> https://hg.mozilla.org/projects/nss/rev/d0f1c1030393

yeah, I think there're multiple errors.

You can build and run tests with USE_ASAN=1

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8757267 [details] [diff] [review]
tmp3

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

While this is looking good to me it make gtests fail. And this time not because of some ASAN error but an actual test error (but only when built with ASAN).
[  FAILED  ] CipherSuiteAEAD12/TlsCipherSuiteTest.ConnectWithCipherSuite/7, where GetParam() = ("TLS", 771, 49188)
[  FAILED  ] CipherSuiteAEAD12/TlsCipherSuiteTest.ConnectWithCipherSuite/8, where GetParam() = ("TLS", 771, 49192)
[  FAILED  ] CipherSuiteAEAD12/TlsCipherSuiteTest.ConnectWithCipherSuite/16, where GetParam() = ("DTLS", 771, 49188)
[  FAILED  ] CipherSuiteAEAD12/TlsCipherSuiteTest.ConnectWithCipherSuite/17, where GetParam() = ("DTLS", 771, 49192)

Comment 8

[Martin Thomson [:mt:]]

franziskus, I get no failures.  Tons of leaks at the end, but no failures.  Did you make clean?

So many leaks I can't even copy and paste them.

Comment 9

[Martin Thomson [:mt:]]

This seems to be the most common stack trace for a leak.

Direct leak of 40032 byte(s) in 834 object(s) allocated from:
    #0 0x7fd767461f30 in calloc (/lib64/libasan.so.3+0xc6f30)
    #1 0x7fd76667f901 in PR_Calloc ../../../../pr/src/malloc/prmem.c:443
    #2 0x7fd7666698b7 in PR_CreateIOLayerStub ../../../../pr/src/io/prlayer.c:447
    #3 0x4a333a in nss_test::DummyPrSocket::CreateFD(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, nss_test::Mode) /home/martin/code/nss/external_tests/ssl_gtest/test_io.cc:292
    #4 0x4acb33 in nss_test::TlsAgent::Init() /home/martin/code/nss/external_tests/ssl_gtest/tls_agent.h:65

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

FWIW, the ASan failure reported by Taskcluster is gone:

https://treeherder.mozilla.org/#/jobs?repo=nss&revision=d0f1c1030393156127d3847082dd09fc0ac5bfbf

Comment 11

[Martin Thomson [:mt:]]

Given the error that franziskus was seeing seems legitimate, I think that I'd still like to make the change in Attachment #8757267 [details] [diff].

Comment 12

[Franziskus Kiefer [:franziskus]]

running USE_ASAN=1 make nss_clean_all nss_build_all and USE_ASAN=1 NSS_TESTS="ssl_gtests" NSS_CYCLES="standard" ./all.sh with Attachment #8757267 [details] [diff] gives me the errors from comment 7.

Looking at the output the first failure is the same as without the patch, but the without leaking. So it looks to me like the patch fixes the leak but not the test failure.

The cipher suites 49188 and 49192 were introduced in bug 923089. I'll do some debugging and see if I can figure out what's going wrong.

Comment 13

[Franziskus Kiefer [:franziskus]]

The problem appears to be the build. We don't link ssl_gtest against libsoftokn, which makes my machine use the system libsoftokn, which obviously can't handle the sha384 case. Adding |$(DIST)/lib/$(LIB_PREFIX)softokn3.$(DLL_SUFFIX)| to the EXTRA_LIBS in the ssl_gtest manifest.mn solves this. Now I also see leaks similar to the ones in bug 1276612.

Comment 14

[Franziskus Kiefer [:franziskus]]

Attachment: linkSoftoken.patch

Comment 15

[Martin Thomson [:mt:]]

Franziskus, should we also link to libnss?

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8757267 [details] [diff] [review]
tmp3

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

I assume that it's OK now.

Comment 17

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8757267 [details] [diff] [review]
tmp3

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

yeah, this patch seems to fix a couple of leaks and works for me with the fixed manifest.

> Franziskus, should we also link to libnss?
hm, that's a good question. I actually assumed that adding things to REQUIRE would link against the newly built libs. But it seems it doesn't (or only statically?). I think we should land this as is and file a follow up to investigate how our build system works and if there are more pitfalls like this.

Comment 18

[Martin Thomson [:mt:]]

Both patches landed.  Let's see what happens to the leak reporting now...

https://hg.mozilla.org/projects/nss/rev/d406bc66ef47
https://hg.mozilla.org/projects/nss/rev/658c53ba58c1

Comment 19

[Martin Thomson [:mt:]]

That's enough changes for this bug.