Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Null pointer dereference in ImportBlackboardFromJSON when entry creation fails #921

Closed
cktii opened this issue Jan 31, 2025 · 6 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@cktii
Copy link

cktii commented Jan 31, 2025

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.

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());
}
entry->value = res->first;
}
}
}

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 getEntry returns nullptr
  • Attempts to create entry if it doesn't exist
  • Gets entry again but doesn't verify it's valid
  • Dereferences the entry pointer 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
@facontidavide
Copy link
Collaborator

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?

@facontidavide facontidavide self-assigned this Jan 31, 2025
@facontidavide facontidavide added the bug Something isn't working label Jan 31, 2025
@cktii
Copy link
Author

cktii commented Jan 31, 2025

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!

@cktii
Copy link
Author

cktii commented Feb 3, 2025

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

@Aglargil
Copy link
Contributor

Aglargil commented Feb 6, 2025

#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

facontidavide added a commit that referenced this issue Feb 9, 2025
@facontidavide
Copy link
Collaborator

fixed making that syntax illegal, as it should be

@facontidavide
Copy link
Collaborator

thanks

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants