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

[0xst3m]

What

When t:base64Decode encounters non-base64 input, the transformation now preserves the original value unchanged and returns false (no change), instead of silently replacing it with an empty string and returning true.

Why

The base64Helper() function in src/utils/base64.cc calls mbedtls_base64_decode but ignores its error code return value. It only checks the out_len output parameter, which stays 0 when mbedtls returns MBEDTLS_ERR_BASE64_INVALID_CHARACTER (-0x002C). This causes Base64Decode::transform() to unconditionally replace the variable's value with an empty string for any non-base64 input.

Impact:

• All transformations after t:base64Decode in a pipeline run on an empty string — effectively dead code for non-base64 input
• CRS rules 934100, 934101, 934160 have degraded post-decode transformation pipelines
• CRS was forced to remove t:base64Decode from rules 934130/934131 as a workaround (coreruleset/coreruleset#3376)

How

Added Base64::tryDecode() which checks the mbedtls_base64_decode return value before proceeding:

• Returns MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL → valid base64, proceed with decode
• Returns anything else → not valid base64, return false (value preserved)

This is a whitelist approach — only the known-good return code proceeds. Any current or future error codes from mbedtls (including the new MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED in mbedtls v4.x) are automatically handled.

Files changed:

File	Change
src/utils/base64.h	Added tryDecode() method declaration
src/utils/base64.cc	Added tryDecode() implementation
src/actions/transformations/base64_decode.cc	Use tryDecode(), preserve value on failure

Backward compatibility:

• The existing Base64::decode() API is unchanged — remote_user.cc is not affected
• Compatible with both mbedtls v3.6.x and v4.x (tested against both)

References

• Closes Base64 decoding generates no warning #1037 (base64 decoding generates no warning — open since 2015)
• Closes base64decode behaviour  #3103 (base64decode behaviour — value destroyed on non-base64 input)
• Related: base64Decode behaviour against payload which contain + and / #3327 (base64Decode behaviour against payload with + and /)
• CRS workaround: coreruleset/coreruleset#3376
• CRS affected rules: 934100, 934101, 934160 (Node.js injection/DoS detection)

Proposed test additions

For secrules-language-tests transformations/base64Decode.json:

{
   "ret" : 0,
   "input" : "not;valid;base64",
   "type" : "tfn",
   "name" : "base64Decode",
   "output" : "not;valid;base64"
},
{
   "ret" : 0,
   "input" : "hello world!",
   "type" : "tfn",
   "name" : "base64Decode",
   "output" : "hello world!"
}

[fzipi]

Thanks for the fix — the core logic (checking the mbedtls return code) is correct and the bug analysis is solid.

One suggestion: rather than adding a new tryDecode() method alongside the broken decode(), could we just fix decode() itself?

The only other caller is remote_user.cc, which decodes the HTTP Basic Auth Authorization header — always valid base64 set by the HTTP server. Even there, silently returning an empty string on invalid input is wrong; it's just less likely to trigger.

Leaving the broken decode() around is a footgun for future callers. A cleaner approach would be:

1. Change decode() signature to return success/failure (e.g. bool decode(const std::string &data, std::string &out) or std::optional<std::string>)
2. Update both base64_decode.cc and remote_user.cc callers
3. Drop tryDecode() entirely

This keeps the API surface smaller and ensures all base64 decoding in the project handles errors properly.

[0xst3m]

Good point — leaving the broken decode() could introduce bugs down the line. I'll update the PR to:

1. Replace std::string decode(const std::string&) with bool decode(const std::string&, std::string &out)
2. Update decode(const std::string&, bool) wrapper to use the new signature
3. Update remote_user.cc to handle decode failure
4. Drop tryDecode()

[airween]

Hi @0xst3m,

Good point — leaving the broken decode() could introduce bugs down the line. I'll update the PR to:

1. Replace `std::string decode(const std::string&)` with `bool decode(const std::string&, std::string &out)`
2. Update `decode(const std::string&, bool)` wrapper to use the new signature
3. Update `remote_user.cc` to handle decode failure
4. Drop `tryDecode()`

do you have any plan when could you add these changes? Also, could you merge the changes from the upstream's v3/master branch?

Thank you!

[Copilot]

Pull request overview

This PR adjusts Base64 decoding behavior so that t:base64Decode no longer destroys the input on invalid Base64, preserving the original value and reporting “no change” instead—preventing downstream transformation pipelines from becoming effectively dead code on non-Base64 input.

Changes:

• Add a boolean-returning Base64 decode helper that validates mbedtls_base64_decode’s return code before modifying output.
• Update t:base64Decode to preserve the original value and return false when decoding fails.
• Update REMOTE_USER parsing to use the new boolean decode API.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/variables/remote_user.cc	Switches Basic auth decoding to a success-checked Base64 decode path.
src/utils/base64.h	Changes Base64 decode API surface (adds bool decode overload, removes prior overload).
src/utils/base64.cc	Implements the new boolean Base64 decode logic using mbedtls return codes.
src/actions/transformations/base64_decode.cc	Preserves value on invalid Base64 input by only assigning decoded output on success.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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.

[0xst3m]

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

[Copilot]

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();

[0xst3m]

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

[Copilot]

In RemoteUser parsing, a failed Base64 decode now leaves base64 as the raw post-"Basic " header value, and the subsequent find(":") will treat inputs like Authorization: Basic user:pass (not base64 per RFC 7617) as valid credentials and set REMOTE_USER to user. To avoid spoofing/behavior regression, only attempt the ":" split/assignment when the Base64 decode succeeded (or clear/skip processing on decode failure).

[0xst3m]

fixed it

[airween]

@0xst3m please take a look at the Copilot notices too.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[0xst3m]

All done, the changes were already applied and I've rebased onto the latest v3/master. The PR now includes:

1. Replaced std::string decode(const std::string&) with bool decode(const std::string&, std::string &out)
2. Updated decode(const std::string&, bool) wrapper to use the new signature
3. Updated remote_user.cc to handle decode failure
4. Dropped tryDecode()

Ready for review.