Skip to content

fix: give internal error codes names and messages (multipart, Lua, ipMatch) #3657

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
fzipi wants to merge 4 commits into
base: v3/master
Choose a base branch
from
Open

fix: give internal error codes names and messages (multipart, Lua, ipMatch) #3657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51b627e
fix: name the multipart Content-Disposition error codes
fzipi Oct 12, 2026
6840604
fix: return 0 from m.setvar on bad arguments instead of -1
fzipi Oct 12, 2026
5a60104
fix: restore the error messages of the @ipMatch tree helpers
fzipi Oct 12, 2026
86896b6
test: make the m.setvar bad-arguments case fail when the variable cha…
fzipi Oct 14, 2026
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
6 changes: 3 additions & 3 deletions src/engine/lua.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,15 +340,15 @@ int Lua::setvar(lua_State *L) {
if (nargs != 2) {
ms_dbg_a(t, 8,
"m.setvar: Failed m.setvar funtion must has 2 arguments");
return -1;
return 0;
}
var_value = luaL_checkstring(L, 2);
var_name = luaL_checkstring(L, 1);

lua_pop(L, 2);

if (var_value == NULL || var_name == NULL) {
return -1;
return 0;
}

vname.assign(var_name);
Expand All @@ -363,7 +363,7 @@ int Lua::setvar(lua_State *L) {
ms_dbg_a(t, 8,
"m.setvar: Must specify a collection using dot character" \
" - ie m.setvar(tx.myvar,mydata)");
return -1;
return 0;
}

if (collection == "TX") {
Expand Down
94 changes: 72 additions & 22 deletions src/request_body_processor/multipart.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,12 +270,60 @@
}


const char *contentDispositionErrorMessage(int rc) {
switch (rc) {
case CD_ERR_NOT_FORM_DATA:
return "disposition type is not form-data";
case CD_ERR_SEMICOLON_EXPECTED:
return "expected ';' after form-data";
case CD_ERR_PARAMETER_MISSING:
return "missing parameter after ';'";
case CD_ERR_VALUE_MISSING:
return "parameter has no value";
case CD_ERR_EQUALS_MISSING:
return "missing '=' after parameter name";
case CD_ERR_VALUE_EMPTY:
return "parameter value is empty";
case CD_ERR_QUOTE_EMPTY_UNTERMINATED:
return "end of header after opening quote";
case CD_ERR_ESCAPE_INCOMPLETE:
return "backslash at end of quoted value";
case CD_ERR_UNKNOWN_PARAMETER:
return "unknown parameter";
case CD_ERR_SEMICOLON_EXPECTED_AFTER_VALUE:
return "expected ';' after parameter value";
case CD_ERR_EQUALS_EXPECTED:
return "expected '=' after parameter name";
case CD_ERR_DUPLICATE_NAME:
return "duplicate name parameter";
case CD_ERR_QUOTE_UNTERMINATED:
return "closing quote not found";
case CD_ERR_FILENAME_STAR_CHARSET:
return "filename*: charset missing";
case CD_ERR_FILENAME_STAR_LANGUAGE:
return "filename*: closing quote of language not found";
case CD_ERR_FILENAME_STAR_PCT_ENCODING:
return "filename*: invalid percent-encoding";
case CD_ERR_FILENAME_STAR_CHARACTER:
return "filename*: invalid character";
case CD_ERR_DUPLICATE_FILENAME:
return "duplicate filename parameter";
case CD_ERR_DUPLICATE_FILENAME_STAR:
return "duplicate filename* parameter";
case CD_ERR_EXCEPTION:
return "unexpected exception";
default:
return "unknown error";
}
}


int Multipart::parse_content_disposition(const char *c_d_value, int offset) {
const char *p = NULL;

/* accept only what we understand */
if (strncmp(c_d_value, "form-data", 9) != 0) {
return -1;
return CD_ERR_NOT_FORM_DATA;
}

/* see if there are any other parts to parse */
Expand All @@ -287,7 +335,7 @@
}

if (*p != ';') {
return -2;
return CD_ERR_SEMICOLON_EXPECTED;
}
p++;

Expand All @@ -304,7 +352,7 @@
}

if (*p == '\0') {
return -3;
return CD_ERR_PARAMETER_MISSING;
}

start = p;
Expand All @@ -313,7 +361,7 @@
}

if (*p == '\0') {
return -4;
return CD_ERR_VALUE_MISSING;
}

name = std::string(start, (p - start));
Expand All @@ -323,11 +371,11 @@
}

if (*p == '\0') {
return -5;
return CD_ERR_EQUALS_MISSING;
}

if (*p != '=') {
return -13;
return CD_ERR_EQUALS_EXPECTED;
}

p++;
Expand All @@ -337,7 +385,7 @@
}

if (*p == '\0') {
return -6;
return CD_ERR_VALUE_EMPTY;
}

char quote = '\0';
Expand All @@ -349,7 +397,7 @@
p++;
}
if ((*p != '\'') || (p == start_of_charset)) {
return -16; // Must be at least one legit char before ' for start of language
return CD_ERR_FILENAME_STAR_CHARSET; // Must be at least one legit char before ' for start of language
}
m_mpp->m_filename_charset.assign(start_of_charset, (p - start_of_charset));
m_mpp->m_filename_charsetOffset = offset + ((p - c_d_value) - m_mpp->m_filename_charset.size());
Expand All @@ -364,7 +412,7 @@
}
if (*p != '\'') {
m_flag_invalid_quoting = 1;
return -17; // Single quote for end-of-language not found
return CD_ERR_FILENAME_STAR_LANGUAGE; // Single quote for end-of-language not found
}
m_mpp->m_filename_language.assign(start_of_language, (p - start_of_language));
m_mpp->m_filename_languageOffset = offset + ((p - c_d_value) - m_mpp->m_filename_language.size());
Expand All @@ -378,13 +426,13 @@
while ((*p != '\0') && (*p != ';')) {
if (*p == '%') {
if ((*(p+1) == '\0') || (*(p+2) == '\0') || (!isxdigit(static_cast<unsigned char>(*(p+1)))) || (!isxdigit(static_cast<unsigned char>(*(p+2))))) {
return -18;
return CD_ERR_FILENAME_STAR_PCT_ENCODING;
}
p += 3;
} else if (isalnum(static_cast<unsigned char>(*p)) || strchr(attr_char_special, *p)) {
p++;
} else {
return -19;
return CD_ERR_FILENAME_STAR_CHARACTER;
}
}
value.assign(start_of_filename, p - start_of_filename);
Expand All @@ -401,14 +449,14 @@

p++;
if (*p == '\0') {
return -7;
return CD_ERR_QUOTE_EMPTY_UNTERMINATED;
}

while (*p != '\0') {
if (*p == '\\') {
if (*(p + 1) == '\0') {
/* improper escaping */
return -8;
return CD_ERR_ESCAPE_INCOMPLETE;
}
/* only quote and \ can be escaped */
if ((*(p + 1) == quote) || (*(p + 1) == '\\')) {
Expand All @@ -434,7 +482,7 @@
p++; /* go over the quote at the end */
} else {
m_flag_invalid_quoting = 1;
return -15; /* closing quote not found */
return CD_ERR_QUOTE_UNTERMINATED;
}

} else {
Expand All @@ -459,7 +507,7 @@
ms_dbg_a(m_transaction, 4,
"Multipart: Warning: Duplicate Content-Disposition " \
"name: " + value + ". Previously: " + m_mpp->m_name + "");
return -14;
return CD_ERR_DUPLICATE_NAME;
}
m_mpp->m_name.assign(value);
m_mpp->m_nameOffset = offset + ((p - c_d_value) - value.size());
Expand All @@ -473,7 +521,7 @@
ms_dbg_a(m_transaction, 4,
"Multipart: Warning: Duplicate Content-Disposition " \
"filename: " + value + ".");
return -15;
return CD_ERR_DUPLICATE_FILENAME;
}
else {
validate_quotes(value.c_str(), quote);
Expand All @@ -491,7 +539,7 @@
ms_dbg_a(m_transaction, 4,
"Multipart: Warning: Duplicate Content-Disposition " \
"filename*: " + value + ".");
return -15;
return CD_ERR_DUPLICATE_FILENAME_STAR;
}

int invalid_count;
Expand All @@ -505,7 +553,7 @@
ms_dbg_a(m_transaction, 9,
"Multipart: Content-Disposition filename*: " + decoded_value + ".");
} else {
return -11;
return CD_ERR_UNKNOWN_PARAMETER;
}

if (*p != '\0') {
Expand All @@ -527,7 +575,7 @@
m_flag_invalid_quoting = 1;
}
/* p++; */
return -12;
return CD_ERR_SEMICOLON_EXPECTED_AFTER_VALUE;
}
p++; /* move over the semi-colon */
}
Expand Down Expand Up @@ -814,15 +862,17 @@
} catch (...) {
ms_dbg_a(m_transaction, 1,
"Multipart: Unexpected error parsing Content-Disposition header.");
rc = -99;
rc = CD_ERR_EXCEPTION;
}
if (rc < 0) {
ms_dbg_a(m_transaction, 1,
"Multipart: Invalid Content-Disposition header ("
+ std::to_string(rc) + "): " + header_value);
+ std::to_string(rc) + ": " + contentDispositionErrorMessage(rc)
+ "): " + header_value);

Check warning on line 871 in src/request_body_processor/multipart.cc

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Modify the macro definition so that it needs to be followed by a semicolon, or remove this empty statement. 


See more on https://sonarcloud.io/project/issues?id=owasp-modsecurity_ModSecurity&issues=AaEwX0CR7OT8vPd5Tovc&open=AaEwX0CR7OT8vPd5Tovc&pullRequest=3657

error->assign("Multipart: Invalid Content-Disposition header ("
+ std::to_string(rc) + "): " + header_value);
+ std::to_string(rc) + ": " + contentDispositionErrorMessage(rc)
+ "): " + header_value);
return false;
}

Expand Down
34 changes: 34 additions & 0 deletions src/request_body_processor/multipart.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,40 @@
};



/* Return codes of Multipart::parse_content_disposition(). The numeric
* values are the ones ModSecurity v2 (msc_multipart.c) uses and they are
* part of the REQBODY_ERROR_MSG text, so keep them stable; add new codes
* at the end. Use contentDispositionErrorMessage() for the description.
*/
enum ContentDispositionError {

Check warning on line 172 in src/request_body_processor/multipart.h

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace this "enum" with "enum class". 


See more on https://sonarcloud.io/project/issues?id=owasp-modsecurity_ModSecurity&issues=AaEwXz8w7OT8vPd5Tovb&open=AaEwXz8w7OT8vPd5Tovb&pullRequest=3657
CD_ERR_NOT_FORM_DATA = -1,
CD_ERR_SEMICOLON_EXPECTED = -2,
CD_ERR_PARAMETER_MISSING = -3,
CD_ERR_VALUE_MISSING = -4,
CD_ERR_EQUALS_MISSING = -5,
CD_ERR_VALUE_EMPTY = -6,
CD_ERR_QUOTE_EMPTY_UNTERMINATED = -7,
CD_ERR_ESCAPE_INCOMPLETE = -8,
/* -9 and -10 are unused: v2 reserved them for improper escaping and
* an unquoted empty value, which v3 tolerates. */
CD_ERR_UNKNOWN_PARAMETER = -11,
CD_ERR_SEMICOLON_EXPECTED_AFTER_VALUE = -12,
CD_ERR_EQUALS_EXPECTED = -13,
CD_ERR_DUPLICATE_NAME = -14,
CD_ERR_QUOTE_UNTERMINATED = -15,
CD_ERR_FILENAME_STAR_CHARSET = -16,
CD_ERR_FILENAME_STAR_LANGUAGE = -17,
CD_ERR_FILENAME_STAR_PCT_ENCODING = -18,
CD_ERR_FILENAME_STAR_CHARACTER = -19,
CD_ERR_DUPLICATE_FILENAME = -20,
CD_ERR_DUPLICATE_FILENAME_STAR = -21,
CD_ERR_EXCEPTION = -99,
};

const char *contentDispositionErrorMessage(int rc);


class Multipart {
public:
Multipart(const std::string &header, Transaction *transaction);
Expand Down
14 changes: 4 additions & 10 deletions src/utils/ip_tree.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ void IpTree::postOrderTraversal(TreeNode *node) {

IpTree::IpTree() {
// FIXME: deal with possible error.
char *error;
std::string error;
create_radix_tree(&m_tree, &error);
}

Expand All @@ -90,17 +90,13 @@ IpTree::~IpTree() {
}

bool IpTree::addFromBuffer(std::istream *ss, std::string *error) {
char *error_msg = NULL;
for (std::string line; std::getline(*ss, line); ) {
size_t comment_start = line.find('#');
if (comment_start != std::string::npos) {
line = line.substr(0, comment_start);
}
int res = add_ip_from_param(line.c_str(), &m_tree, &error_msg);
int res = add_ip_from_param(line.c_str(), &m_tree, error);
if (res != 0) {
if (error_msg != NULL) {
error->assign(error_msg);
}
return false;
}
}
Expand Down Expand Up @@ -143,10 +139,8 @@ bool IpTree::addFromUrl(const std::string& url, std::string *error) {


bool IpTree::contains(const std::string& ip) {
int res = 0;
char *error_msg = NULL;

res = tree_contains_ip(m_tree, ip.c_str(), &error_msg);
std::string error_msg;
int res = tree_contains_ip(m_tree, ip.c_str(), &error_msg);

if (res < 0) {
return false;
Expand Down
Loading
Loading