Project

General

Profile

Actions

Bug #4997

closed

FreshnessPeriod may become negative

Added by Davide Pesavento about 5 years ago. Updated 9 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Base
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
2.00 h
Tags:

Description

FreshnessPeriod is defined as a NonNegativeInteger in NDN protocol. ndn-cxx's decoding routines for NNI return a uint64_t type. In MetaInfo::wireDecode however, the uint64_t is converted to time::milliseconds, which is a signed type and therefore cannot represent the whole range of uint64_t numbers. This operation has implementation-defined behavior in C++. Typically, large values cause a wraparound and the result becomes negative. Most, if not all, other code in the library and NFD is not prepared to handle a negative FreshnessPeriod, thus causing all sorts of problems. For instance, this NFD assertion will fail.

This bug was found by NFDFuzz, an experimental fuzzer for NFD and ndn-cxx.

Stack trace of the failed assertion in NFD:

fuzzer: ../daemon/fw/forwarder.cpp:526: void nfd::Forwarder::insertDeadNonceList(pit::Entry &, nfd::face::Face *): Assertion `pitEntry.dataFreshnessPeriod >= 0_ms' failed.
==29831== ERROR: libFuzzer: deadly signal
    #0 0x602cf7 in __sanitizer_print_stack_trace /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_stack.cc:38:3
    #1 0x540b46 in fuzzer::Fuzzer::CrashCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:5
    #2 0x540b0f in fuzzer::Fuzzer::StaticCrashSignalCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:206:6
    #3 0x7fa90de9538f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1138f)
    #4 0x7fa90d18a427 in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x35427)
    #5 0x7fa90d18c029 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x37029)
    #6 0x7fa90d182bd6  (/lib/x86_64-linux-gnu/libc.so.6+0x2dbd6)
    #7 0x7fa90d182c81 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x2dc81)
    #8 0x814b3a in nfd::Forwarder::insertDeadNonceList(nfd::pit::Entry&, nfd::face::Face*) /users/gjt3/NFD/build/../daemon/fw/forwarder.cpp:526:5
    #9 0x81521b in nfd::Forwarder::onIncomingData(nfd::FaceEndpoint const&, ndn::Data const&) /users/gjt3/NFD/build/../daemon/fw/forwarder.cpp:311:11
    #10 0x81b0e3 in nfd::Forwarder::Forwarder()::$_2::operator()(nfd::face::Face&) const::{lambda(ndn::Data const&, unsigned long const&)#1}::operator()(ndn::Data const&, unsigned long const&) const /users/gjt3/NFD/build/../daemon/fw/forwarder.cpp:61:15
    #11 0x6f28f0 in ndn::util::signal::Signal<nfd::face::LinkService, ndn::Data, unsigned long>::operator()(ndn::Data const&, unsigned long const&) /usr/local/include/ndn-cxx/util/signal/signal.hpp:232:7
    #12 0x6f1344 in nfd::face::LinkService::receiveData(ndn::Data const&, unsigned long const&) /users/gjt3/NFD/build/../daemon/face/link-service.cpp:104:3
    #13 0x6c2bc8 in nfd::face::GenericLinkService::decodeData(ndn::Block const&, ndn::lp::Packet const&, unsigned long const&) /users/gjt3/NFD/build/../daemon/face/generic-link-service.cpp:445:9
    #14 0x6bf908 in nfd::face::GenericLinkService::decodeNetPacket(ndn::Block const&, ndn::lp::Packet const&, unsigned long const&) /users/gjt3/NFD/build/../daemon/face/generic-link-service.cpp:328:15
    #15 0x6bedb7 in nfd::face::GenericLinkService::doReceivePacket(ndn::Block const&, unsigned long const&) /users/gjt3/NFD/build/../daemon/face/generic-link-service.cpp:304:13
    #16 0x9fe210 in nfd::face::StreamTransport<boost::asio::local::stream_protocol>::handleReceive(boost::system::error_code const&, unsigned long) /users/gjt3/NFD/build/../daemon/face/stream-transport.hpp:258:11
    #17 0x9fd9da in _ZN5boost4asio6detail23reactive_socket_recv_opINS0_17mutable_buffers_1EZN3nfd4face15StreamTransportINS0_5local15stream_protocolEE12startReceiveEvEUlDpOT_E_E11do_completeEPNS1_15task_io_serviceEPNS1_25task_io_service_operationERKNS_6system10error_codeEm /usr/include/boost/asio/detail/reactive_socket_recv_op.hpp:110:7
    #18 0x67129a in boost::asio::detail::task_io_service::do_run_one(boost::asio::detail::scoped_lock<boost::asio::detail::posix_mutex>&, boost::asio::detail::task_io_service_thread_info&, boost::system::error_code const&) /usr/include/boost/asio/detail/impl/task_io_service.ipp:372:12
    #19 0x670a0d in boost::asio::detail::task_io_service::run(boost::system::error_code&) /usr/include/boost/asio/detail/impl/task_io_service.ipp:149:10
    #20 0x66e13d in boost::asio::io_service::run() /usr/include/boost/asio/impl/io_service.ipp:59:25
    #21 0x66138a in nfd::NfdRunner::run() /users/gjt3/NFD/build/../daemon/fuzzer.cpp:159:15
    #22 0x646118 in LLVMFuzzerTestOneInput::$_1::operator()() const /users/gjt3/NFD/build/../daemon/fuzzer.cpp:425:17
    #23 0x7fa91540dc7f  (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0xb8c7f)
    #24 0x7fa90de8b6b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
Actions #1

Updated by Davide Pesavento about 5 years ago

  • Subject changed from Integer overflow in FreshnessPeriod decoding to FreshnessPeriod may become negative
  • Description updated (diff)
Actions #2

Updated by Davide Pesavento over 4 years ago

  • Tags set to fuzzing
Actions #3

Updated by Davide Pesavento about 4 years ago

  • Description updated (diff)
Actions #4

Updated by Junxiao Shi over 1 year ago

  • Assignee set to Junxiao Shi
  • Estimated time set to 2.00 h

https://gerrit.named-data.net/c/ndn-cxx/+/7151
FreshnessPeriod cannot go negative in ndn-cxx.

NFD may also need a patch to ensure now+fp is in the future.

Actions #5

Updated by Junxiao Shi over 1 year ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 40
Actions #6

Updated by Davide Pesavento over 1 year ago

I suppose InterestLifetime suffers from the same issue?

Actions #7

Updated by Davide Pesavento over 1 year ago

  • Target version set to 0.9.0
Actions #8

Updated by Junxiao Shi about 1 year ago

  • % Done changed from 40 to 50
Actions #9

Updated by Junxiao Shi about 1 year ago

  • Status changed from In Progress to Closed
Actions #10

Updated by Davide Pesavento about 1 year ago

  • Status changed from Closed to Feedback

Junxiao Shi wrote in #note-4:

NFD may also need a patch to ensure now+fp is in the future.

What about this?

Actions #11

Updated by Davide Pesavento 9 months ago

  • Status changed from Feedback to Closed
  • % Done changed from 50 to 100

https://gerrit.named-data.net/c/ndn-cxx/+/7375 implements a similar fix for InterestLifetime.

I'm closing this issue since it's about the ndn-cxx bug(s), which should be fixed now. It's unclear to me if there are any related overflow bugs in NFD, but in any case it should be reported as a separate issue.

Actions

Also available in: Atom PDF