Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash doing message counter sync #13668

Closed
bzbarsky-apple opened this issue Jan 18, 2022 · 12 comments · Fixed by #14853
Closed

Crash doing message counter sync #13668

bzbarsky-apple opened this issue Jan 18, 2022 · 12 comments · Fixed by #14853

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

Finally got fuzzing stood up, ran the fuzzer, got:

=34895== ERROR: libFuzzer: deadly signal
    #0 0x10e7fff95 in __sanitizer_print_stack_trace+0x35 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x53f95)
    #1 0x10dac5798 in fuzzer::PrintStackTrace() FuzzerUtil.cpp:210
    #2 0x10daa9473 in fuzzer::Fuzzer::CrashCallback() FuzzerLoop.cpp:233
    #3 0x7fff6838d5fc in _sigtramp+0x1c (libsystem_platform.dylib:x86_64+0x35fc)
    #4 0x10dcfb15f  (<unknown module>)
    #5 0x7fff68263807 in abort+0x77 (libsystem_c.dylib:x86_64+0x7f807)
    #6 0x10d53a419 in chip::Transport::Session::AsSecureSession() Session.cpp:27
    #7 0x10da32851 in chip::secure_channel::MessageCounterManager::SendMsgCounterSyncResp(chip::Messaging::ExchangeContext*, chip::FixedSpan<unsigned char const, 8ul>) MessageCounterManager.cpp:236
    #8 0x10da30a87 in chip::secure_channel::MessageCounterManager::HandleMsgCounterSyncReq(chip::Messaging::ExchangeContext*, chip::System::PacketBufferHandle&&) MessageCounterManager.cpp:261
    #9 0x10da30610 in chip::secure_channel::MessageCounterManager::OnMessageReceived(chip::Messaging::ExchangeContext*, chip::PayloadHeader const&, chip::System::PacketBufferHandle&&) MessageCounterManager.cpp:96
    #10 0x10d4e83e2 in chip::Messaging::ExchangeContext::HandleMessage(unsigned int, chip::PayloadHeader const&, chip::Transport::PeerAddress const&, chip::BitFlags<chip::Messaging::MessageFlagValues, unsigned int>, chip::System::PacketBufferHandle&&) ExchangeContext.cpp:461
    #11 0x10d4f0b22 in chip::Messaging::ExchangeManager::OnMessageReceived(chip::PacketHeader const&, chip::PayloadHeader const&, chip::SessionHandle const&, chip::Transport::PeerAddress const&, chip::SessionMessageDelegate::DuplicateMessage, chip::System::PacketBufferHandle&&) ExchangeMgr.cpp:305
    #12 0x10d545516 in chip::SessionManager::SecureGroupMessageDispatch(chip::PacketHeader const&, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&) SessionManager.cpp:612
    #13 0x10d544984 in chip::SessionManager::OnMessageReceived(chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&) SessionManager.cpp:375
    #14 0x10cfb9ac7 in LLVMFuzzerTestOneInput main.cpp:147
    #15 0x10daaa8c3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:611
    #16 0x10daaa0da in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:514
    #17 0x10daab589 in fuzzer::Fuzzer::MutateAndTestOne() FuzzerLoop.cpp:757
    #18 0x10daabfb5 in fuzzer::Fuzzer::Loop(std::__2::vector<fuzzer::SizedFile, std::__2::allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:895
    #19 0x10da9adea in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:912
    #20 0x10dac6252 in main FuzzerMain.cpp:20
    #21 0x7fff68194cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 3 ChangeBinInt-ChangeByte-PersAutoDict- DE: "\007\000\000\000\000\000\000\000"-; base unit: fa91c7912f718cfd3f1e59b25dea323175b722f3
0xe,0x0,0xf1,0xb1,0xf1,0xf1,0xf1,0xf1,0xed,0x73,0x7,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xc1,0x0,0x0,0x0,0x0,0x0,0x5c,0xf3,0x25,0x0,0x0,0x0,0x0,0x0,
\016\000\361\261\361\361\361\361\355s\007\000\000\000\000\000\000\000\301\000\000\000\000\000\\\363%\000\000\000\000\000
artifact_prefix='./'; Test unit written to ./crash-c9fd2434ccf4a33a7f49765dcc519e1fd529a8e5
Base64: DgDxsfHx8fHtcwcAAAAAAAAAwQAAAAAAXPMlAAAAAAA=

Proposed Solution

Investigate why receiving the given payload:

0xe,0x0,0xf1,0xb1,0xf1,0xf1,0xf1,0xf1,0xed,0x73,0x7,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xc1,0x0,0x0,0x0,0x0,0x0,0x5c,0xf3,0x25,0x0,0x0,0x0,0x0,0x0,

as a message leads to the above attempt to treat a non-CASE session as a SecureSession.

@bzbarsky-apple
Copy link
Contributor Author

See also #13397

@andy31415
Copy link
Contributor

From the stack trace, it looks like a Message counter sync is received as a group message. Guessing that groups are not a secure session Unlikely to yet they want to get secure counters from secure sessions.

@andy31415
Copy link
Contributor

Learning how to investigate this. So far I found:

  • building target linux-x64-all-clusters-libfuzzer seems to enable all cluster fuzzing
  • ./out/linux-x64-all-clusters-libfuzzer/chip-all-clusters-app-fuzzing -help=1 gives some usage examples

@andy31415
Copy link
Contributor

@bzbarsky-apple - Do you by any chance have the reproduction file?

https://github.com/google/fuzzing/blob/master/tutorial/libFuzzerTutorial.md seems to say that to reproduce crashes without re-running fuzzing, one would pass in the crash file (?) as an argument to the binary.

@andy31415
Copy link
Contributor

For what its worth, after running fuzzing for a while, I seem to have 3 separate crash files in my directory. Will check to see if they have the same stack trace as this bug.

@andy31415
Copy link
Contributor

Created a crash file with:

 open('crash-test', 'wb').write(binascii.a2b_base64('DgDxsfHx8fHtcwcAAAAAAAAAwQAAAAAAXPMlAAAAAAA='))

Crash test seems to say just 'Incorrect state' for ble. Did not see an obvious crash, but I may be building the wrong version of the app.

@bzbarsky-apple
Copy link
Contributor Author

bzbarsky-apple commented Feb 7, 2022

Merging #14514 made it much harder for the fuzzer to reach the relevant code, because now we need an actual valid MIC and whatnot to reach it....

@andy31415
Copy link
Contributor

andy31415 commented Feb 7, 2022

I will try to change things to reproduce it. For now ran the fuzzer on 12 cores for about 20 minutes and no crash yet (previous crashes I had were due to me pressing CTRL+C to stop the fuzzing)

Created a fix of 'validate that input argument is indeed a secure session'. I also wonder if this may be a stateful thing as sessions are global state that the fuzzer likely does not reset.

@bzbarsky-apple
Copy link
Contributor Author

Group sessions are not global state, fwiw...

@andy31415
Copy link
Contributor

I checked out ae05dfc and then was able to reproduce:

[1644262052.243952][4137057:4137057] CHIP:SPT: VerifyOrDie failure at ../../examples/all-clusters-app/linux/third_party/connectedhomeip/src/transport/Session.cpp:27: GetSessionType() == SessionType::kSecure
==4137057== ERROR: libFuzzer: deadly signal
    #0 0x558813dbacc4 in __sanitizer_print_stack_trace ../staging/llvm_build/tools/clang/stage2-bins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/ubsan/ubsan_diag_standalone.cpp:31:3

Will cherrypick my fix and validate

@andy31415
Copy link
Contributor

I can confirm that the CP made the test not crash.

@andy31415
Copy link
Contributor

From discussions with @bzbarsky-apple - it seems that this not really 'fix the crash' and more like 'group messaging does not implement MCSP' and existing code is not correct because it assumes 'secure session' (non-group) which would generally not work. This code path is supposed to only apply to groups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants