Project

General

Profile

Actions

Bug #3974

closed

Unsafe casts in ndn::nfd::*::wireDecode()

Added by Davide Pesavento almost 8 years ago. Updated over 7 years ago.

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

100%

Estimated time:
3.00 h

Description

Fields such as FacePersistency, FaceScope, LinkType are defined as 8-bit long enumerations. However, when they're being decoded from a TLV block, the nonNegativeInteger value, which can be as long as a uint64_t, is simply cast to the target type, without any range checks. The result of this operation is unspecified (undefined behavior since C++17) if the value, converted to the enumeration's underlying type (uint8_t), is out of the enumeration's range.

An example of buggy code in ndn::nfd::FaceStatus is:

  m_faceScope = static_cast<FaceScope>(readNonNegativeInteger(*val));

This can be considered a security vulnerability, because it's trivial to craft packets that remotely trigger the unspecified/undefined behavior.

The same applies to several other unsafe casts throughout the management module, and possibly elsewhere.


Related issues 2 (0 open2 closed)

Related to ndn-cxx - Bug #3497: Mgmt/Nfd/TestControlCommand/Face{Enable,Disable}LocalControl undefined behaviorClosed

Actions
Related to ndn-cxx - Bug #3200: Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponseClosedJunxiao Shi

Actions
Actions #1

Updated by Junxiao Shi almost 8 years ago

  • Related to Bug #3497: Mgmt/Nfd/TestControlCommand/Face{Enable,Disable}LocalControl undefined behavior added
Actions #2

Updated by Junxiao Shi over 7 years ago

I'm not getting any undefined behavior warning in this snippet:

// g++ -std=c++11 -o x x.cpp $(pkg-config --cflags --libs libndn-cxx) -fsanitize=undefined

#include <cstdint>

enum class A : uint8_t
{
  A0 = 0,
  A1 = 1,
  A2 = 2
};

int
main()
{
  static_cast<A>(3);
  static_cast<A>(259);
  static_cast<A>(65539);
}

Is the problem with "narrowing conversion", or with uint8_t value with no corresponding named constant?

Actions #3

Updated by Davide Pesavento over 7 years ago

Junxiao Shi wrote:

I'm not getting any undefined behavior warning in this snippet:

That's irrelevant. You can't assume that UBSan will find (or even knows about) every instance of undefined behavior in a program. Moreover, as I said in the description, technically the behavior is undefined since C++17 only; in C++11 there's no UB, but the resulting value is "unspecified" (which is equally useless and dangerous for all practical purposes).

Is the problem with "narrowing conversion", or with uint8_t value with no corresponding named constant?

Neither. It's already explained in the issue description, but I'll also cite the C++ standard (current draft):

[expr.static.cast]/10
A value of integral or enumeration type can be explicitly converted to a complete enumeration type. The value
is unchanged if the original value is within the range of the enumeration values (10.2). Otherwise, the behavior
is undefined.

Where "the range of the enumeration values" is defined as follows:

[dcl.enum]/8
For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the
underlying type.
[...]
It is possible to define an enumeration that has values not defined by any of its enumerators.

Actions #4

Updated by Junxiao Shi over 7 years ago

  • Related to Bug #3200: Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse added
Actions #5

Updated by Davide Pesavento over 7 years ago

  • Related to deleted (Bug #3200: Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse)
Actions #6

Updated by Junxiao Shi over 7 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
  • % Done changed from 0 to 20
  • Estimated time set to 3.00 h

https://gerrit.named-data.net/4083 patchset1 is a preview of how I plan to solve this issue. The solution is applied to FaceStatus only.

Actions #7

Updated by Junxiao Shi over 7 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 20 to 100
Actions #8

Updated by Junxiao Shi over 7 years ago

  • Status changed from Code review to Closed
Actions #9

Updated by Davide Pesavento over 7 years ago

  • Related to Bug #3200: Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse added
Actions

Also available in: Atom PDF