Skip to content

fix: preserve value in t:base64Decode when input is not valid base64 #3533

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.

Sign up for GitHub

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

Open
0xst3m wants to merge 1 commit into
base: v3/master
Choose a base branch
from
+36 −8
Open

fix: preserve value in t:base64Decode when input is not valid base64 #3533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/actions/transformations/base64_decode.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,14 @@ namespace modsecurity::actions::transformations {

bool Base64Decode::transform(std::string &value, const Transaction *trans) const {
if (value.empty()) return false;
value = Utils::Base64::decode(value);

std::string decoded;
if (!Utils::Base64::decode(value, decoded)) {
return false;
}

value = std::move(decoded);
return true;
}


} // namespace modsecurity::actions::transformations
27 changes: 23 additions & 4 deletions src/utils/base64.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,33 @@ std::string Base64::decode(const std::string& data, bool forgiven) {
return decode_forgiven(data);
}

return decode(data);
std::string out;
decode(data, out);
return out;
}


std::string Base64::decode(const std::string& data) {
return base64Helper(data.c_str(), strlen(data.c_str()), mbedtls_base64_decode);
}
bool Base64::decode(const std::string& data, std::string &out) {
size_t out_len = 0;
const auto *src = reinterpret_cast<const unsigned char *>(data.c_str());
const size_t slen = strlen(data.c_str());
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

Base64::decode(const std::string&, std::string&) uses strlen(data.c_str()) for the input length. Since data is a std::string, this will truncate at the first embedded NUL and can mis-handle binary data; it’s also unnecessary work. Prefer using data.size() (and data.data() if you don’t need a NUL terminator) for the length passed to mbedtls.

Suggested change
const size_t slen = strlen(data.c_str());
const size_t slen = data.size();

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@0xst3m 0xst3m Apr 19, 2026

Choose a reason for hiding this comment

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

Kept strlen intentionally to preserve the original decode() behavior. Existing test case with embedded NUL depends on truncation at first NUL.


const int ret = mbedtls_base64_decode(nullptr, 0, &out_len, src, slen);

if (ret != MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL) {
return false;
}

out.resize(out_len);
if (mbedtls_base64_decode(
reinterpret_cast<unsigned char *>(out.data()),
out.size(), &out_len, src, slen) != 0) {
return false;
}

out.resize(out_len);
return true;
}

std::string Base64::decode_forgiven(const std::string& data) {
return base64Helper(data.c_str(), data.size(), decode_forgiven_engine);
Expand Down
2 changes: 1 addition & 1 deletion src/utils/base64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Base64 {
static std::string encode(const std::string& data);

static std::string decode(const std::string& data, bool forgiven);
static std::string decode(const std::string& data);
static bool decode(const std::string& data, std::string &out);
static std::string decode_forgiven(const std::string& data);
Comment on lines 31 to 33
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

This change removes the Base64::decode(const std::string&) -> std::string overload and replaces it with a bool decode(const std::string&, std::string&). That is an API break for any in-tree or downstream code including this header, and it also conflicts with the PR description claiming the existing decode() API is unchanged. Consider keeping the original overload (possibly implemented in terms of the new boolean-returning function) and/or adding a new tryDecode-style method instead of changing the signature.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@0xst3m 0xst3m Apr 19, 2026

Choose a reason for hiding this comment

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

Intentional per reviewer feedback, the broken single-arg overload is removed to prevent future misuse. All 3 callers are updated in this PR.


static void decode_forgiven_engine(unsigned char *plain_text,
Expand Down
6 changes: 5 additions & 1 deletion src/variables/remote_user.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ void RemoteUser::evaluate(Transaction *transaction,
base64 = std::string(header, 6, header.length());
}

base64 = Utils::Base64::decode(base64);
if (std::string decoded; Utils::Base64::decode(base64, decoded)) {
base64 = std::move(decoded);
} else {
base64.clear();
}

if (const auto pos{base64.find(":")}; pos != std::string::npos) {
transaction->m_variableRemoteUser.assign(std::string(base64, 0, pos));
Expand Down