Multiple SIGILL crashes in SafeAny::details::checkTruncation due to undefined behavior in type conversions

[cktii]

Description

When converting a double value larger than INT_MAX to an integer using the Blackboard system, the program encounters undefined behavior, leading to a SIGILL (illegal instruction). This occurs in the checkTruncation function when attempting to validate numeric conversions.

The function attempts to detect truncation by converting back and forth between types, but does so without first checking if the value is within the target type's limits, leading to undefined behavior.

Found in commit: 48f6c5b

Bug Class

Integer/Type Conversion Vulnerability - Undefined Behavior

Impact

Program crash (SIGILL) when converting large floating-point values to integers
Potential security implications in parsing untrusted data
Can affect any system using the Blackboard for numeric type conversions

Reproducer

BT::Blackboard::Ptr bb = BT::Blackboard::create();
bb->set("test", 2.7249173913590775e+38);  // Any double > INT_MAX
bb->get<int>("test");  // Triggers SIGILL

Root Cause

In convert_impl.hpp:91:

BehaviorTree.CPP/include/behaviortree_cpp/utils/convert_impl.hpp

Lines 88 to 95 in 48f6c5b

	template <typename From, typename To>
	inline void checkTruncation(const From& from)
	{
	if(from != static_cast<From>(static_cast<To>(from)))
	{
	throw std::runtime_error("Floating point truncated");
	}
	}

The issue occurs because static_cast<To>(from) is undefined behavior when from exceeds the limits of To. This undefined value is then used in another cast and comparison operation.

Stack trace

#0  checkTruncation<double, int> (from=2.7249173913590775e+38)
#1  convertNumber<double, int>
#2  BT::Any::convert<int>
#3  BT::Any::tryCast<int>
#4  BT::Any::cast<int>
#5  BT::Blackboard::get<int>

Proposed Fix

Replace the current implementation with a bounds check before attempting the conversion:

template <typename From, typename To>
inline void checkTruncation(const From& from)
{
    // First check numeric limits
    if (from > static_cast<From>(std::numeric_limits<To>::max()) || 
        from < static_cast<From>(std::numeric_limits<To>::min())) {
        throw std::runtime_error("Value outside numeric limits");
    }
    
    // Now safe to do the truncation check
    To as_target = static_cast<To>(from);
    From back_to_source = static_cast<From>(as_target);
    if(from != back_to_source) {
        throw std::runtime_error("Floating point truncated");
    }
}

Additional Notes

This was found via fuzzing with AFL++. The crash is reproducible in both Debug and Release builds.

[cktii]

Upon further inspection, the above fix is not sufficient.

Observation

• For integer types, static_cast<To> is undefined when the value exceeds the target type's limits
• For floating-point types, the conversion might lose precision or be impossible to represent

New reproducer

bb->set("test2", 18446744073709551360ul);  // Near ULONG_MAX
bb->get<double>("test2");  // Triggers SIGILL

Stack trace

#0  checkTruncation<unsigned long, double> (from=18446744073709551360)
#1  convertNumber<unsigned long, double>
#2  BT::Any::convert<double>
#3  BT::Any::tryCast<double>
#4  BT::Any::cast<double>
#5  BT::Blackboard::get<double>

Extended proposed fix

Maybe something like this:

template <typename From, typename To>
inline void checkTruncation(const From& from)
{
    // Handle integer to floating point
    if constexpr(std::is_integral_v<From> && std::is_floating_point_v<To>) {
        // Check if value can be represented exactly in the target type
        To as_float = static_cast<To>(from);
        From back_conv = static_cast<From>(as_float);
        if(back_conv != from) {
            throw std::runtime_error("Loss of precision in conversion");
        }
    }
    // Handle floating point to integer
    else if constexpr(std::is_floating_point_v<From> && std::is_integral_v<To>) {
        if (from > static_cast<From>(std::numeric_limits<To>::max()) || 
            from < static_cast<From>(std::numeric_limits<To>::min()) ||
            from != std::trunc(from)) {
            throw std::runtime_error("Invalid floating point to integer conversion");
        }
    }
    // Handle other conversions
    else {
        if (from > static_cast<From>(std::numeric_limits<To>::max()) || 
            from < static_cast<From>(std::numeric_limits<To>::min())) {
            throw std::runtime_error("Value outside numeric limits");
        }
        To as_target = static_cast<To>(from);
        From back_to_source = static_cast<From>(as_target);
        if(from != back_to_source) {
            throw std::runtime_error("Value truncated in conversion");
        }
    }
}

[facontidavide]

fixed with some changes. please have a look at 0c4a74e