Skip to content

fix: prevent null byte truncation bypass in SQL injection detection#410

Open
PopoviciMarian wants to merge 5 commits into
mainfrom
fix/null-byte-truncation-bypass
Open

fix: prevent null byte truncation bypass in SQL injection detection#410
PopoviciMarian wants to merge 5 commits into
mainfrom
fix/null-byte-truncation-bypass

Conversation

@PopoviciMarian
Copy link
Copy Markdown
Contributor

@PopoviciMarian PopoviciMarian commented Mar 31, 2026

Changes:

  • Replace ContextCallback return type (char*) with CallbackResult struct carrying both data pointer and explicit length
  • Use malloc+memcpy instead of strdup in GoContextCallback
  • Use C.GoStringN instead of C.GoString on the Go side
  • Use binary-safe std::string(ptr, len) constructors in all SQL query
    capture handlers (PDO, PDOStatement, mysqli)
  • Use binary-safe string construction in ArrayToJson, GetVar, GetBody,
    and GetHeaders to preserve null bytes in user input data

Summary by Aikido

Security Issues: 0 🔍 Quality Issues: 4 Resolved Issues: 0

🐛 Bugfixes

  • Made GoContextCallback use malloc+memcpy and return explicit length.
  • Used binary-safe std::string(ptr,len) and C.GoStringN to preserve nulls.

🔧 Refactors

  • Replaced char* ContextCallback with CallbackResult struct carrying length.
  • Added null checks and malloc failure handling to avoid crashes.

More info

@PopoviciMarian PopoviciMarian marked this pull request as ready for review March 31, 2026 15:54
Comment thread lib/php-extension/HandleQueries.cpp
Comment thread lib/php-extension/GoWrappers.cpp
Comment thread lib/php-extension/GoWrappers.cpp
return "";
}
return Z_STRVAL_P(data);
return std::string(Z_STRVAL_P(data), Z_STRLEN_P(data));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetVar now constructs and returns a std::string copy for each call, causing a heap allocation and memory copy per call; consider returning a non-owning view or deferring copies to callers.

Details

✨ AI Reasoning
​GetVar was modified to allocate and copy the PHP string into a std::string on every call. The function is used by many higher-level methods (method, URL, headers, etc.), so each call now incurs heap allocation and memory copy. This increases work proportional to the data size per call and multiplies across callers, turning many cheap O(1) pointer accesses into O(n) allocations and copies. Avoidable repeated allocations are an obvious performance regression.

🔧 How do I fix it?
Move constant work outside loops. Use StringBuilder instead of string concatenation in loops. Cache compiled regex patterns. Use hash-based lookups instead of nested loops. Batch database operations instead of N+1 queries.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

Comment thread lib/php-extension/HandleQueries.cpp Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant