Fix audit findings: soundness, coverage reporting, docs, and review follow-ups #1
9 changed files with 231 additions and 38 deletions
fix(model): apply libc models only to C library callees
malloc/free heap modeling (encoder), use-after-free's free/realloc, and
the taint source/sink tables selected a model by the callee's bare
identifier. A project function that merely shares a libc name was
modeled as libc:
namespace mine { void free(void*); }
mine::free(p); ... std::free(p); // double-free "proved", plus
// oob "double free" and a UAF
and `Pool::free(p)` behaved the same way, as did a member function
named `system` as a command-execution sink.
isCLibraryFunction / isCLibraryCallee (encoder_callee_names) accept a
direct callee that is not a class member and is declared at global
scope, in namespace std, or extern "C". The encoder's malloc-family and
free models, use-after-free's free/realloc, and every libc/POSIX taint
entry now require it. Entries that are deliberately method-name
heuristics (get, Query, execute, query, fromJson, readString*, the
request-parameter getters) are marked c_api = false and keep matching
by name as before.
Tests: test_library_callee_identity (namespaced and member `free` are
not double-free/UAF; member `system` is not a sink; libc free twice and
libc system(getenv()) still are), and isCLibraryCallee cases in
test_encoder_callee_names (::free, std::free, mine::free, Pool::free,
a global helper, an indirect call). Full suite 137/137.
commit
67483ee6b0
|
|
@ -30,6 +30,9 @@ struct TaintSource {
|
|||
std::string func;
|
||||
int param = 0; // -1 = return value
|
||||
std::string description = "tainted data";
|
||||
// A C library / POSIX function: matches only a callee that is one
|
||||
// (isCLibraryCallee). False for method-name heuristics such as `get`.
|
||||
bool c_api = true;
|
||||
};
|
||||
|
||||
struct TaintSink {
|
||||
|
|
@ -37,6 +40,7 @@ struct TaintSink {
|
|||
int param = 0;
|
||||
std::string description = "dangerous sink";
|
||||
unsigned domain = taint_domain::kAny; // which sanitizer domain neutralises it
|
||||
bool c_api = true; // see TaintSource::c_api
|
||||
};
|
||||
|
||||
class TaintTracker : public SafetyObserver {
|
||||
|
|
|
|||
|
|
@ -29,4 +29,13 @@ namespace fold {
|
|||
|
||||
[[nodiscard]] std::string qualifiedCalleeName(const clang::CallExpr* ce);
|
||||
|
||||
// True when `fd` can be the C library function its name suggests: not a class
|
||||
// member, and declared at global scope, in namespace std, or extern "C".
|
||||
// Library models (malloc/free, taint sources and sinks) must check this; the
|
||||
// bare identifier alone also matches `mine::free` or `Pool::free`.
|
||||
[[nodiscard]] bool isCLibraryFunction(const clang::FunctionDecl* fd);
|
||||
|
||||
// isCLibraryFunction of a call's direct callee; false for indirect calls.
|
||||
[[nodiscard]] bool isCLibraryCallee(const clang::CallExpr* ce);
|
||||
|
||||
} // namespace fold
|
||||
|
|
|
|||
|
|
@ -20,12 +20,14 @@
|
|||
|
||||
#include "fold/detect/model_validation.hpp"
|
||||
#include "fold/detect/security_walker.hpp"
|
||||
#include "fold/symbolic/encoder_callee_names.hpp"
|
||||
|
||||
namespace fold::detect {
|
||||
|
||||
namespace {
|
||||
|
||||
const std::vector<TaintSource> kSources = {{"scanf", 1, "untrusted input"},
|
||||
const std::vector<TaintSource> kSources = {
|
||||
{"scanf", 1, "untrusted input"},
|
||||
{"fscanf", 2, "untrusted input"},
|
||||
{"sscanf", 2, "untrusted input"},
|
||||
{"recv", 1, "network input"},
|
||||
|
|
@ -41,24 +43,24 @@ const std::vector<TaintSource> kSources = {{"scanf", 1, "untrusted input"},
|
|||
{"accept", -1, "network handle"},
|
||||
{"curl_easy_perform", -1, "network response"},
|
||||
{"curl_easy_getinfo", 2, "network metadata"},
|
||||
{"queryParam", -1, "URL query parameter"},
|
||||
{"get_query_param", -1, "URL query parameter"},
|
||||
{"get_form_param", -1, "Form data parameter"},
|
||||
{"read_header", -1, "HTTP header"},
|
||||
{"Query", -1, "Database query result"},
|
||||
{"get", -1, "Map/Request lookup"},
|
||||
{"readString", -1, "deserialized string"},
|
||||
{"readStringVector", -1, "deserialized string list"},
|
||||
{"readStringMap", -1, "deserialized string map"},
|
||||
{"readStringArray", 2, "deserialized string array"},
|
||||
{"fromJson", -1, "JSON deserialization source"}};
|
||||
{"queryParam", -1, "URL query parameter", false},
|
||||
{"get_query_param", -1, "URL query parameter", false},
|
||||
{"get_form_param", -1, "Form data parameter", false},
|
||||
{"read_header", -1, "HTTP header", false},
|
||||
{"Query", -1, "Database query result", false},
|
||||
{"get", -1, "Map/Request lookup", false},
|
||||
{"readString", -1, "deserialized string", false},
|
||||
{"readStringVector", -1, "deserialized string list", false},
|
||||
{"readStringMap", -1, "deserialized string map", false},
|
||||
{"readStringArray", 2, "deserialized string array", false},
|
||||
{"fromJson", -1, "JSON deserialization source", false}};
|
||||
|
||||
const std::vector<TaintSink> kSinks = {
|
||||
{"sqlite3_prepare_v2", 1, "SQL injection", taint_domain::kSql},
|
||||
{"sqlite3_prepare_v3", 1, "SQL injection", taint_domain::kSql},
|
||||
{"PQexecParams", 1, "SQL injection", taint_domain::kSql},
|
||||
{"execute", -1, "database execution", taint_domain::kSql},
|
||||
{"query", -1, "database query", taint_domain::kSql},
|
||||
{"execute", -1, "database execution", taint_domain::kSql, false},
|
||||
{"query", -1, "database query", taint_domain::kSql, false},
|
||||
{"send", -1, "network transmission", taint_domain::kNetwork},
|
||||
{"write", -1, "file/socket write", taint_domain::kNetwork},
|
||||
{"open", 0, "path access", taint_domain::kPath},
|
||||
|
|
@ -218,8 +220,11 @@ std::string canonicalSinkName(const std::string& callee_name) {
|
|||
return isExecFamilySink(callee_name) ? "exec" : callee_name;
|
||||
}
|
||||
|
||||
bool sinkMatches(const TaintSink& entry, const std::string& callee_name) {
|
||||
return entry.func == callee_name;
|
||||
// A libc/POSIX entry matches only the C library function itself: a method or
|
||||
// namespaced function that shares the name (`Shell::system`) is not the sink.
|
||||
bool sinkMatches(const TaintSink& entry, const std::string& callee_name,
|
||||
const clang::CallExpr* call) {
|
||||
return entry.func == callee_name && (!entry.c_api || isCLibraryCallee(call));
|
||||
}
|
||||
|
||||
} // namespace
|
||||
|
|
@ -276,7 +281,7 @@ void TaintTracker::onBinaryArith(BinOp op, const z3::expr& lhs, const z3::expr&
|
|||
bool TaintTracker::isSourceCall(const clang::CallExpr* call, const std::string& callee_name,
|
||||
TaintSource& source) const {
|
||||
for (const auto& entry : kSources) {
|
||||
if (entry.func == callee_name) {
|
||||
if (entry.func == callee_name && (!entry.c_api || isCLibraryCallee(call))) {
|
||||
if (callee_name == "get" && call) {
|
||||
if (const auto* callee = call->getDirectCallee()) {
|
||||
if (callee->isInStdNamespace()) {
|
||||
|
|
@ -300,7 +305,7 @@ bool TaintTracker::isSourceCall(const clang::CallExpr* call, const std::string&
|
|||
|
||||
bool TaintTracker::isSinkCall(const std::string& callee_name, TaintSink& sink) const {
|
||||
for (const auto& entry : kSinks) {
|
||||
if (sinkMatches(entry, callee_name)) {
|
||||
if (entry.func == callee_name) {
|
||||
sink = entry;
|
||||
return true;
|
||||
}
|
||||
|
|
@ -675,7 +680,7 @@ void TaintTracker::onCallExpr(const fold::ir::Call& call_ir,
|
|||
|
||||
bool matched_sink = false;
|
||||
for (const auto& sink : kSinks) {
|
||||
if (!sinkMatches(sink, callee_name))
|
||||
if (!sinkMatches(sink, callee_name, call))
|
||||
continue;
|
||||
matched_sink = true;
|
||||
bool tainted = false;
|
||||
|
|
|
|||
|
|
@ -23,6 +23,7 @@
|
|||
|
||||
#include "fold/detect/security_walker.hpp"
|
||||
#include "fold/symbolic/budget.hpp"
|
||||
#include "fold/symbolic/encoder_callee_names.hpp"
|
||||
#include "fold/symbolic/heap_model.hpp"
|
||||
#include "fold/symbolic/safety_observer.hpp"
|
||||
#include "fold/symbolic/tactics.hpp"
|
||||
|
|
@ -121,6 +122,12 @@ std::string calleeName(const clang::CallExpr* call) {
|
|||
return callee != nullptr ? callee->getNameAsString() : std::string{};
|
||||
}
|
||||
|
||||
// The allocator functions this detector models, by resolved declaration: a
|
||||
// project `mine::free` or `Pool::free` is not libc free.
|
||||
std::string libraryCalleeName(const clang::CallExpr* call) {
|
||||
return isCLibraryCallee(call) ? calleeName(call) : std::string{};
|
||||
}
|
||||
|
||||
class Analyzer {
|
||||
public:
|
||||
Analyzer(const clang::ASTContext& ctx, std::vector<Finding>& out) : ctx_(ctx), out_(out) {}
|
||||
|
|
@ -275,14 +282,14 @@ class Analyzer {
|
|||
|
||||
bool isSameVarRealloc(const clang::VarDecl* lhs, const clang::Expr* rhs) const {
|
||||
const auto* call = clang::dyn_cast_or_null<clang::CallExpr>(ignore(rhs));
|
||||
if (lhs == nullptr || call == nullptr || calleeName(call) != "realloc" ||
|
||||
if (lhs == nullptr || call == nullptr || libraryCalleeName(call) != "realloc" ||
|
||||
call->getNumArgs() == 0)
|
||||
return false;
|
||||
return varFromExpr(call->getArg(0)) == lhs;
|
||||
}
|
||||
|
||||
bool handleFreeLikeCall(const clang::CallExpr* call, State& state) {
|
||||
if (calleeName(call) == "free" && call->getNumArgs() == 1) {
|
||||
if (libraryCalleeName(call) == "free" && call->getNumArgs() == 1) {
|
||||
poisonVar(state, varFromExpr(call->getArg(0)));
|
||||
return true;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -61,6 +61,18 @@ std::string qualifiedFunctionName(const clang::FunctionDecl* fd) {
|
|||
return name;
|
||||
}
|
||||
|
||||
bool isCLibraryFunction(const clang::FunctionDecl* fd) {
|
||||
if (fd == nullptr || clang::isa<clang::CXXMethodDecl>(fd))
|
||||
return false;
|
||||
if (fd->isExternC() || fd->isInStdNamespace())
|
||||
return true;
|
||||
return fd->getDeclContext()->getRedeclContext()->isTranslationUnit();
|
||||
}
|
||||
|
||||
bool isCLibraryCallee(const clang::CallExpr* ce) {
|
||||
return ce != nullptr && isCLibraryFunction(ce->getDirectCallee());
|
||||
}
|
||||
|
||||
std::string qualifiedCalleeName(const clang::CallExpr* ce) {
|
||||
if (ce == nullptr)
|
||||
return {};
|
||||
|
|
|
|||
|
|
@ -575,7 +575,10 @@ z3::expr encodeCallExprArm(const clang::CallExpr* ce, const clang::ASTContext& c
|
|||
}
|
||||
return returnCallValue(name, env.ctx().bv_val(0, kBitvecWidth), false);
|
||||
}
|
||||
if (isMallocLikeCall(name)) {
|
||||
// Heap models apply only to the C library functions, not to a project
|
||||
// function or method that shares the name (`mine::free`, `Pool::free`).
|
||||
const bool c_library_callee = isCLibraryCallee(ce);
|
||||
if (c_library_callee && isMallocLikeCall(name)) {
|
||||
z3::expr size = env.ctx().bv_val(0, kBitvecWidth);
|
||||
bool is_stack = (name == "alloca" || name == "_alloca" || name == "__builtin_alloca");
|
||||
if (name == "malloc" && ce->getNumArgs() >= 1) {
|
||||
|
|
@ -606,7 +609,7 @@ z3::expr encodeCallExprArm(const clang::CallExpr* ce, const clang::ASTContext& c
|
|||
}
|
||||
return returnCallValue(name, ptr);
|
||||
}
|
||||
if (isFreeCall(name) && ce->getNumArgs() >= 1) {
|
||||
if (c_library_callee && isFreeCall(name) && ce->getNumArgs() >= 1) {
|
||||
z3::expr ptr = coerceToBv(env, arg_values[0]);
|
||||
bool known_region = env.heapModel().isTrackedPointer(ptr);
|
||||
bool already_freed = env.heapModel().isFreed(ptr);
|
||||
|
|
|
|||
|
|
@ -195,6 +195,15 @@ test_loop_exit_soundness = executable(
|
|||
)
|
||||
test('loop_exit_soundness', test_loop_exit_soundness)
|
||||
|
||||
test_library_callee_identity = executable(
|
||||
'test_library_callee_identity',
|
||||
'test_library_callee_identity.cpp',
|
||||
dependencies: [fold_core_dep, catch2_dep],
|
||||
install: false,
|
||||
build_by_default: false,
|
||||
)
|
||||
test('library_callee_identity', test_library_callee_identity)
|
||||
|
||||
test_batch_safety_observer = executable(
|
||||
'test_batch_safety_observer',
|
||||
'test_batch_safety_observer.cpp',
|
||||
|
|
|
|||
|
|
@ -153,3 +153,30 @@ TEST_CASE("qualifiedCalleeName resolves through a direct CallExpr",
|
|||
TEST_CASE("qualifiedCalleeName returns empty for null", "[encoder][callee_names][qualified]") {
|
||||
REQUIRE(fold::qualifiedCalleeName(nullptr).empty());
|
||||
}
|
||||
|
||||
TEST_CASE("isCLibraryCallee: only C library functions, never members or namespaced lookalikes",
|
||||
"[encoder-callee-names][library]") {
|
||||
auto unit = parse("extern \"C\" void free(void*);\n"
|
||||
"namespace std { using ::free; }\n"
|
||||
"namespace mine { void free(void*) {} }\n"
|
||||
"struct Pool { void free(void*) {} };\n"
|
||||
"void global_helper(void*) {}\n"
|
||||
"void f(void* p, Pool& pool, void (*fp)(void*)) {\n"
|
||||
" free(p);\n"
|
||||
" std::free(p);\n"
|
||||
" mine::free(p);\n"
|
||||
" pool.free(p);\n"
|
||||
" global_helper(p);\n"
|
||||
" fp(p);\n"
|
||||
"}\n");
|
||||
REQUIRE(unit);
|
||||
const auto calls = collectCalls(*unit, "f");
|
||||
REQUIRE(calls.size() == 6);
|
||||
CHECK(fold::isCLibraryCallee(calls[0])); // ::free (extern "C")
|
||||
CHECK(fold::isCLibraryCallee(calls[1])); // std::free resolves to ::free
|
||||
CHECK_FALSE(fold::isCLibraryCallee(calls[2])); // mine::free
|
||||
CHECK_FALSE(fold::isCLibraryCallee(calls[3])); // Pool::free
|
||||
CHECK(fold::isCLibraryCallee(calls[4])); // global scope: C semantics
|
||||
CHECK_FALSE(fold::isCLibraryCallee(calls[5])); // indirect call
|
||||
CHECK_FALSE(fold::isCLibraryCallee(nullptr));
|
||||
}
|
||||
|
|
|
|||
117
tests/unit/test_library_callee_identity.cpp
Normal file
117
tests/unit/test_library_callee_identity.cpp
Normal file
|
|
@ -0,0 +1,117 @@
|
|||
#include <clang/Frontend/ASTUnit.h>
|
||||
#include <clang/Tooling/Tooling.h>
|
||||
|
||||
#include <catch2/catch_test_macros.hpp>
|
||||
#include <memory>
|
||||
#include <string>
|
||||
#include <vector>
|
||||
|
||||
#include "fold/detect/cross_tu_cache.hpp"
|
||||
#include "fold/detect/double_free.hpp"
|
||||
#include "fold/detect/taint_track.hpp"
|
||||
#include "fold/detect/use_after_free.hpp"
|
||||
#include "fold/report/finding.hpp"
|
||||
#include "fold/symbolic/budget.hpp"
|
||||
#include "fold/symbolic/encoder_callee_names.hpp"
|
||||
|
||||
// Library models (malloc/free, taint sources and sinks) were selected by the
|
||||
// callee's bare identifier, so a project function that merely shares a libc
|
||||
// name was modeled as libc: `mine::free(p)` followed by `free(p)` was a Proved
|
||||
// double-free. Models now require the resolved declaration to be a C library
|
||||
// function (global scope, namespace std, or extern "C"; never a member).
|
||||
|
||||
namespace {
|
||||
|
||||
std::unique_ptr<clang::ASTUnit> parse(const std::string& code) {
|
||||
return clang::tooling::buildASTFromCodeWithArgs(code, {"-std=c++20"}, "input.cpp");
|
||||
}
|
||||
|
||||
std::size_t countKind(const std::vector<fold::Finding>& findings, fold::FindingKind kind) {
|
||||
std::size_t n = 0;
|
||||
for (const auto& f : findings)
|
||||
if (f.kind == kind)
|
||||
++n;
|
||||
return n;
|
||||
}
|
||||
|
||||
std::vector<fold::Finding> runDoubleFree(const std::string& code) {
|
||||
auto unit = parse(code);
|
||||
REQUIRE(unit);
|
||||
fold::SolverBudget b;
|
||||
fold::SolverStats s;
|
||||
return fold::detectDoubleFreeInContext(unit->getASTContext(), b, s);
|
||||
}
|
||||
|
||||
std::vector<fold::Finding> runUseAfterFree(const std::string& code) {
|
||||
auto unit = parse(code);
|
||||
REQUIRE(unit);
|
||||
return fold::detectUseAfterFreeInContext(unit->getASTContext());
|
||||
}
|
||||
|
||||
std::vector<fold::Finding> runTaint(const std::string& code) {
|
||||
fold::detect::CrossTUCache::instance().clear();
|
||||
auto unit = parse(code);
|
||||
REQUIRE(unit);
|
||||
fold::SolverBudget b;
|
||||
fold::SolverStats s;
|
||||
return fold::detectTaintTrackInContext(unit->getASTContext(), b, s);
|
||||
}
|
||||
|
||||
const std::string kLibc = "extern \"C\" void* malloc(unsigned long);\n"
|
||||
"extern \"C\" void free(void*);\n";
|
||||
|
||||
} // namespace
|
||||
|
||||
TEST_CASE("heap model: a namespaced function named free is not libc free",
|
||||
"[library-callee][double-free]") {
|
||||
const std::string code = kLibc + "namespace mine { inline void free(void*) {} }\n"
|
||||
"int f() {\n"
|
||||
" int* p = static_cast<int*>(malloc(sizeof(int)));\n"
|
||||
" if (!p) return 0;\n"
|
||||
" *p = 1;\n"
|
||||
" mine::free(p);\n"
|
||||
" int v = *p;\n"
|
||||
" free(p);\n"
|
||||
" return v;\n"
|
||||
"}\n";
|
||||
CHECK(countKind(runDoubleFree(code), fold::FindingKind::DoubleFree) == 0);
|
||||
CHECK(countKind(runUseAfterFree(code), fold::FindingKind::UseAfterFree) == 0);
|
||||
}
|
||||
|
||||
TEST_CASE("heap model: a member function named free is not libc free",
|
||||
"[library-callee][double-free]") {
|
||||
const std::string code = kLibc + "struct Pool { void free(void*) {} };\n"
|
||||
"void f(Pool& pool) {\n"
|
||||
" void* p = malloc(8);\n"
|
||||
" if (!p) return;\n"
|
||||
" pool.free(p);\n"
|
||||
" free(p);\n"
|
||||
"}\n";
|
||||
CHECK(countKind(runDoubleFree(code), fold::FindingKind::DoubleFree) == 0);
|
||||
}
|
||||
|
||||
TEST_CASE("heap model: libc free twice is still a double free", "[library-callee][double-free]") {
|
||||
// Negative control: the fix must not drop the real model.
|
||||
const std::string code = kLibc + "void f() {\n"
|
||||
" void* p = malloc(8);\n"
|
||||
" if (!p) return;\n"
|
||||
" free(p);\n"
|
||||
" free(p);\n"
|
||||
"}\n";
|
||||
CHECK(countKind(runDoubleFree(code), fold::FindingKind::DoubleFree) == 1);
|
||||
}
|
||||
|
||||
TEST_CASE("taint: a member function named system is not a command sink",
|
||||
"[library-callee][taint]") {
|
||||
const std::string code = "extern \"C\" char* getenv(const char*);\n"
|
||||
"struct Shell { int system(const char*) { return 0; } };\n"
|
||||
"int f(Shell& sh) { return sh.system(getenv(\"CMD\")); }\n";
|
||||
CHECK(countKind(runTaint(code), fold::FindingKind::TaintTrack) == 0);
|
||||
}
|
||||
|
||||
TEST_CASE("taint: libc system with tainted input is still a sink", "[library-callee][taint]") {
|
||||
const std::string code = "extern \"C\" char* getenv(const char*);\n"
|
||||
"extern \"C\" int system(const char*);\n"
|
||||
"int f() { return system(getenv(\"CMD\")); }\n";
|
||||
CHECK(countKind(runTaint(code), fold::FindingKind::TaintTrack) == 1);
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue