Null pointer dereference in ImportBlackboardFromJSON when entry creation fails
Closed this issue · 6 comments
Description
A null pointer dereference occurs in ImportBlackboardFromJSON when attempting to use an entry pointer without verifying that the entry was successfully created. This happens because the code assumes createEntry followed by getEntry will always return a valid entry.
BehaviorTree.CPP/src/blackboard.cpp
Lines 280 to 295 in 48f6c5b
Found in commit: 48f6c5b
Bug Class
Memory Safety - Null Pointer Dereference
Root Cause
void ImportBlackboardFromJSON(const nlohmann::json& json, Blackboard& blackboard)
{
for(auto it = json.begin(); it != json.end(); ++it)
{
if(auto res = JsonExporter::get().fromJson(it.value()))
{
auto entry = blackboard.getEntry(it.key());
if(!entry)
{
blackboard.createEntry(it.key(), res->second);
entry = blackboard.getEntry(it.key()); // Could still return nullptr
}
entry->value = res->first; // CRASH: Null pointer dereference
}
}
}The issue occurs because:
- Code checks if initial
getEntryreturnsnullptr - Attempts to create
entryif it doesn't exist - Gets
entryagain but doesn't verify it's valid - Dereferences the
entrypointer without checking
GDB
pwndbg> p entry->value
Cannot access memory at address 0x0
pwndbg> p entry
$1 = std::shared_ptr<BT::Blackboard::Entry> (empty) = {
get() = 0x0
}
pwndbg> p res->first
$2 = {
_any = {
storage = {
dynamic = 0xffffffffe010006e,
stack = {
__data = "n\000\020\340\377\377\377\377\000\000\000\000\000\000\000",
__align = {<No data fields>}
}
},
vtable = 0x555556412f40 <linb::any::vtable_for_type<long>()::table>
},
_original_type = {
_M_target = 0x7ffff7c6e610 <typeinfo for long>
}
}
pwndbg> p json
$1 = (const nlohmann::json_abi_v3_11_3::json &) @0x7ffff550dc20: {
<nlohmann::json_abi_v3_11_3::detail::json_default_base> = {<No data fields>},
members of nlohmann::json_abi_v3_11_3::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::json_abi_v3_11_3::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> >, void>:
m_data = {
m_type = nlohmann::json_abi_v3_11_3::detail::value_t::object,
m_value = {
object = 0x504000000250,
array = 0x504000000250,
string = 0x504000000250,
binary = 0x504000000250,
boolean = 80,
number_integer = 88235808129616,
number_unsigned = 88235808129616,
number_float = 4.3594281529883041e-310
}
}
}
Stack trace
#0 0x00005555558f5d1e in BT::ImportBlackboardFromJSON (json=..., blackboard=...)
Proposed Fix
void ImportBlackboardFromJSON(const nlohmann::json& json, Blackboard& blackboard)
{
for(auto it = json.begin(); it != json.end(); ++it)
{
if(auto res = JsonExporter::get().fromJson(it.value()))
{
auto entry = blackboard.getEntry(it.key());
if(!entry)
{
blackboard.createEntry(it.key(), res->second);
entry = blackboard.getEntry(it.key());
if(!entry) {
// Either throw or continue to next entry
continue;
}
}
entry->value = res->first;
}
}
}Impact
Impact
- Program crash on malformed input
- Potential security vulnerability if JSON input is untrusted
- Could be used for denial of service attacks
Thanks for the accurate description.
The more interesting question here is why "blackboard.createEntry(it.key(), res->second);" is not doing what is expected to do.
Do you have perhaps a simple example to reproduce the issue?
I must have a reproducer lying around that I used when debugging this. I’ll try to attach one ASAP.
While I’m at it, I’ll try to deliver a minimal PoC for all the other ones as well!
Okay, after some debugging I came to the conclusion that unlike stated above in the original description suggested this was a simple null pointer dereference from createEntry/getEntry, this seems to be a more complex issue involving the any type system.
Minimal Reproducer
#include "behaviortree_cpp/blackboard.h"
#include <iostream>
#include <nlohmann/json.hpp>
int main() {
try {
auto bb = BT::Blackboard::create();
// Set initial value
bb->set("mmmmmm5CnCaaG", -535822226);
// Create JSON that will trigger the crash
std::string json_str = R"({
"@@@@@@@@@@@@@@@": -535822226,
"mmmmmm5CnCaaG": null
})";
// Parse and import JSON - will crash
auto json = nlohmann::json::parse(json_str);
BT::ImportBlackboardFromJSON(json, *bb);
}
catch(const std::exception& e) {
std::cerr << "Exception: " << e.what() << std::endl;
return 1;
}
return 0;
}Updated stack-trace
AddressSanitizer:DEADLYSIGNAL
=================================================================
==3669987==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010
#0 0x7dfae074647f in linb::any::swap(linb::any&) /home/user/BehaviorTree.CPP/include/behaviortree_cpp/contrib/any.hpp:183:32
#1 0x7dfae074647f in linb::any::operator=(linb::any const&) /home/user/BehaviorTree.CPP/include/behaviortree_cpp/contrib/any.hpp:129:18
#2 0x7dfae074647f in BT::Any::operator=(BT::Any const&) /home/user/BehaviorTree.CPP/include/behaviortree_cpp/utils/safe_any.hpp:306:14
#3 0x7dfae074647f in BT::ImportBlackboardFromJSON(...) /home/user/BehaviorTree.CPP/src/blackboard.cpp:292:20
The issue appears to be related to type handling in the Any implementation when dealing with null values during JSON deserialization. This suggests the fix might need to handle:
- Proper null value handling in the JSON import
- Type safety checks in the Any implementation
- Validation of entry state before assignment
#include "behaviortree_cpp/blackboard.h"
#include <iostream>
#include <nlohmann/json.hpp>
int main() {
try {
auto bb = BT::Blackboard::create();
// Set initial value
bb->set("mmmmmm5CnCaaG", -535822226);
// Create JSON that will trigger the crash
std::string json_str = R"({
"@@@@@@@@@@@@@@@": -535822226,
"mmmmmm5CnCaaG": null
})";
// Parse and import JSON - will crash
auto json = nlohmann::json::parse(json_str);
BT::ImportBlackboardFromJSON(json, *bb);
}
catch(const std::exception& e) {
std::cerr << "Exception: " << e.what() << std::endl;
return 1;
}
return 0;
}Hi, @facontidavide
The reason why blackboard.createEntry(it.key(), res->second); did not perform as expected in this case is: When the current key was "@@@@@@@@@@@@@@@" (15 @ symbols), since the key starts with @, rootBlackboard()->createEntryImpl(key.substr(1, key.size() - 1), info); created an entry with key "@@@@@@@@@@@@@@" (14 @ symbols). The critical issue was that the implementation didn't recursively call createEntry during this process, while the getEntry method does employ recursive calls to getEntry. This inconsistency ultimately led to failing to find the entry when searching for an empty string key (after recursively stripping all @ prefixes).
I've submitted a PR to fix this behavior #929
fixed making that syntax illegal, as it should be
thanks
