Bug #3974
closedUnsafe casts in ndn::nfd::*::wireDecode()
100%
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.
Updated by Junxiao Shi over 7 years ago
- Related to Bug #3497: Mgmt/Nfd/TestControlCommand/Face{Enable,Disable}LocalControl undefined behavior added
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?
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.
Updated by Junxiao Shi over 7 years ago
- Related to Bug #3200: Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse added
Updated by Davide Pesavento over 7 years ago
- Related to deleted (Bug #3200: Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse)
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.
Updated by Junxiao Shi about 7 years ago
- Status changed from In Progress to Code review
- % Done changed from 20 to 100
Updated by Junxiao Shi about 7 years ago
- Status changed from Code review to Closed
Updated by Davide Pesavento about 7 years ago
- Related to Bug #3200: Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse added