From 17a2cbd164bbfc06c09a84ce35206db57da71c53 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Mon, 19 Aug 2024 09:12:12 -0700 Subject: [PATCH] Perform UrlDecodeUni & UrlDecode transformations in-place - Use std::string in UrlEncode transformation, instead of manually memory management. This avoids an additional copy after completing encoding by just swapping the encoded value and the input. - Removed inplace helper function from the class, as it's only referenced by the implementation. --- src/actions/transformations/url_decode.cc | 25 +++----- src/actions/transformations/url_decode_uni.cc | 54 ++++++----------- src/actions/transformations/url_decode_uni.h | 2 - src/actions/transformations/url_encode.cc | 60 ++++++------------- src/actions/transformations/url_encode.h | 3 - 5 files changed, 45 insertions(+), 99 deletions(-) diff --git a/src/actions/transformations/url_decode.cc b/src/actions/transformations/url_decode.cc index 7e5f8767..6f231022 100644 --- a/src/actions/transformations/url_decode.cc +++ b/src/actions/transformations/url_decode.cc @@ -27,25 +27,16 @@ UrlDecode::UrlDecode(const std::string &action) } bool UrlDecode::transform(std::string &value, const Transaction *trans) const { - unsigned char *val(NULL); int invalid_count = 0; - int _changed; + int changed; + const auto new_len = utils::urldecode_nonstrict_inplace( + (unsigned char*)value.data(), + value.length(), + &invalid_count, + &changed); - val = (unsigned char *) malloc(sizeof(char) * value.size() + 1); - memcpy(val, value.c_str(), value.size() + 1); - val[value.size()] = '\0'; - - int size = utils::urldecode_nonstrict_inplace(val, value.size(), - &invalid_count, &_changed); - std::string out; - - out.append((const char *)val, size); - - free(val); - - const auto changed = out != value; - value = out; - return changed; + value.resize(new_len); + return changed != 0; } diff --git a/src/actions/transformations/url_decode_uni.cc b/src/actions/transformations/url_decode_uni.cc index fdc2e79b..57df7a56 100644 --- a/src/actions/transformations/url_decode_uni.cc +++ b/src/actions/transformations/url_decode_uni.cc @@ -22,41 +22,19 @@ namespace modsecurity::actions::transformations { -bool UrlDecodeUni::transform(std::string &value, const Transaction *t) const { - std::string ret; - unsigned char *input; - - input = reinterpret_cast - (malloc(sizeof(char) * value.length()+1)); - - if (input == NULL) { - return ""; - } - - memcpy(input, value.c_str(), value.length()+1); - - size_t i = inplace(input, value.length(), t); - - ret.assign(reinterpret_cast(input), i); - free(input); - - const auto changed = ret != value; - value = ret; - return changed; -} - - /** * * IMP1 Assumes NUL-terminated */ -int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len, +static inline bool inplace(std::string &value, const Transaction *t) { - unsigned char *d = input; - int64_t i, count, fact, j, xv; - int Code, hmap = -1; + bool changed = false; + auto d = reinterpret_cast(value.data()); + const unsigned char *input = d; + const auto input_len = value.length(); - if (input == NULL) return -1; + std::string::size_type i, count, fact, j, xv; + int Code, hmap = -1; i = count = 0; while (i < input_len) { @@ -116,19 +94,17 @@ int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len, } } d++; - count++; i += 6; + changed = true; } else { /* Invalid data, skip %u. */ *d++ = input[i++]; *d++ = input[i++]; - count += 2; } } else { /* Not enough bytes (4 data bytes), skip %u. */ *d++ = input[i++]; *d++ = input[i++]; - count += 2; } } else { /* Standard URL encoding. */ @@ -143,8 +119,8 @@ int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len, if (VALID_HEX(c1) && VALID_HEX(c2)) { *d++ = utils::string::x2c(&input[i + 1]); - count++; i += 3; + changed = true; } else { /* Not a valid encoding, skip this % */ *d++ = input[i++]; @@ -153,25 +129,31 @@ int UrlDecodeUni::inplace(unsigned char *input, uint64_t input_len, } else { /* Not enough bytes available, skip this % */ *d++ = input[i++]; - count++; } } } else { /* Character is not a percent sign. */ if (input[i] == '+') { *d++ = ' '; + changed = true; } else { *d++ = input[i]; } - count++; i++; } } *d = '\0'; - return count; + value.resize(d - input); + return changed; +} + + + +bool UrlDecodeUni::transform(std::string &value, const Transaction *trans) const { + return inplace(value, trans); } diff --git a/src/actions/transformations/url_decode_uni.h b/src/actions/transformations/url_decode_uni.h index 31962851..8dbac462 100644 --- a/src/actions/transformations/url_decode_uni.h +++ b/src/actions/transformations/url_decode_uni.h @@ -26,8 +26,6 @@ class UrlDecodeUni : public Transformation { : Transformation(action) { } bool transform(std::string &value, const Transaction *trans) const override; - static int inplace(unsigned char *input, uint64_t input_len, - const Transaction *transaction); }; } // namespace modsecurity::actions::transformations diff --git a/src/actions/transformations/url_encode.cc b/src/actions/transformations/url_encode.cc index e7f59f85..36fef771 100644 --- a/src/actions/transformations/url_encode.cc +++ b/src/actions/transformations/url_encode.cc @@ -26,65 +26,43 @@ UrlEncode::UrlEncode(const std::string &action) } -std::string UrlEncode::url_enc(const char *input, - unsigned int input_len, int *changed) { - char *rval, *d; - unsigned int i, len; - int count = 0; +static inline bool url_enc(std::string &value) { + const auto len = value.size() * 3 + 1; + std::string ret(len, {}); - *changed = 0; - - len = input_len * 3 + 1; - d = rval = reinterpret_cast(malloc(len)); - if (rval == NULL) { - return {}; - } + bool changed = false; /* ENH Only encode the characters that really need to be encoded. */ - for (i = 0; i < input_len; i++) { - unsigned char c = input[i]; - + char *d = ret.data(); + for (const auto c : value) { if (c == ' ') { *d++ = '+'; - *changed = 1; - count++; + changed = true; } else { if ( (c == 42) || ((c >= 48) && (c <= 57)) || ((c >= 65) && (c <= 90)) || ((c >= 97) && (c <= 122))) { *d++ = c; - count++; - } else { + } + else + { *d++ = '%'; - count++; - utils::string::c2x(c, (unsigned char *)d); - d += 2; - count++; - count++; - *changed = 1; + d = (char *)utils::string::c2x(c, (unsigned char *)d); + changed = true; } } } - *d = '\0'; - - std::string ret(""); - ret.append(rval, count); - free(rval); - return ret; -} - - -bool UrlEncode::transform(std::string &value, const Transaction *trans) const { - int _changed; - - std::string ret = url_enc(value.c_str(), value.size(), &_changed); - - const auto changed = ret != value; - value = ret; + ret.resize(d - ret.c_str()); + std::swap(value, ret); return changed; } +bool UrlEncode::transform(std::string &value, const Transaction *trans) const { + return url_enc(value); +} + + } // namespace modsecurity::actions::transformations diff --git a/src/actions/transformations/url_encode.h b/src/actions/transformations/url_encode.h index b300c40d..4f3ffe73 100644 --- a/src/actions/transformations/url_encode.h +++ b/src/actions/transformations/url_encode.h @@ -25,9 +25,6 @@ class UrlEncode : public Transformation { explicit UrlEncode(const std::string &action); bool transform(std::string &value, const Transaction *trans) const override; - - static std::string url_enc(const char *input, - unsigned int input_len, int *changed); }; } // namespace modsecurity::actions::transformations