Skip to content

Commit ad28de4

Browse files
WGH-zimmerle
WGH-
authored and
zimmerle
committed
Refactor regex code
This commit fixes quite a few odd things in regex code: * Lack of encapsulation. * Non-method functions for matching without retrieving all groups. * Regex class being copyable without proper copy-constructor (potential UAF and double free due to pointer members m_pc and m_pce). * Redundant SMatch::m_length, which always equals to match.size() anyway. * Weird SMatch::size_ member which is initialized only by one of the three matching functions, and equals to the return value of that function anyways. * Several places in code having std::string value instead of reference.
1 parent e0a0fa0 commit ad28de4

File tree

10 files changed

+69
-68
lines changed

10 files changed

+69
-68
lines changed

‎CHANGES

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
v3.0.4 - YYYY-MMM-DD (to be released)
22
-------------------------------------
33

4+
- Refactoring on Regex and SMatch classes.
5+
[@WGH-]
46
- Fixed buffer overflow in Utils::Md5::hexdigest()
57
[Issue #2002 - @defanator]
68
- Implemented merge() method for ConfigInt, ConfigDouble, ConfigString

‎src/collection/backend/in_memory-per_process.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ void InMemoryPerProcess::resolveRegularExpression(const std::string& var,
134134
//std::string name = std::string(var, var.find(":") + 2,
135135
// var.size() - var.find(":") - 3);
136136
//size_t keySize = col.size();
137-
Utils::Regex r = Utils::Regex(var);
137+
Utils::Regex r(var);
138138

139139
for (const auto& x : *this) {
140140
//if (x.first.size() <= keySize + 1) {

‎src/modsecurity.cc

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -259,9 +259,9 @@ int ModSecurity::processContentOffset(const char *content, size_t len,
259259
std::string value;
260260
yajl_gen_map_open(g);
261261
vars.pop_back();
262-
std::string startingAt = vars.back().match;
262+
const std::string &startingAt = vars.back().str();
263263
vars.pop_back();
264-
std::string size = vars.back().match;
264+
const std::string &size = vars.back().str();
265265
vars.pop_back();
266266
yajl_gen_string(g,
267267
reinterpret_cast<const unsigned char*>("startingAt"),
@@ -311,11 +311,11 @@ int ModSecurity::processContentOffset(const char *content, size_t len,
311311
strlen("transformation"));
312312

313313
yajl_gen_string(g,
314-
reinterpret_cast<const unsigned char*>(trans.back().match.c_str()),
315-
trans.back().match.size());
314+
reinterpret_cast<const unsigned char*>(trans.back().str().c_str()),
315+
trans.back().str().size());
316316

317317
t = modsecurity::actions::transformations::Transformation::instantiate(
318-
trans.back().match.c_str());
318+
trans.back().str().c_str());
319319
varValueRes = t->evaluate(varValue, NULL);
320320
varValue.assign(varValueRes);
321321
trans.pop_back();
@@ -343,9 +343,9 @@ int ModSecurity::processContentOffset(const char *content, size_t len,
343343
strlen("highlight"));
344344
yajl_gen_map_open(g);
345345
ops.pop_back();
346-
std::string startingAt = ops.back().match;
346+
std::string startingAt = ops.back().str();
347347
ops.pop_back();
348-
std::string size = ops.back().match;
348+
std::string size = ops.back().str();
349349
ops.pop_back();
350350
yajl_gen_string(g,
351351
reinterpret_cast<const unsigned char*>("startingAt"),

‎src/operators/rx.cc

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ bool Rx::init(const std::string &arg, std::string *error) {
3838

3939
bool Rx::evaluate(Transaction *transaction, Rule *rule,
4040
const std::string& input, std::shared_ptr<RuleMessage> ruleMessage) {
41-
SMatch match;
4241
std::list<SMatch> matches;
4342
Regex *re;
4443

@@ -59,16 +58,16 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule,
5958
matches.reverse();
6059
for (const SMatch& a : matches) {
6160
transaction->m_collections.m_tx_collection->storeOrUpdateFirst(
62-
std::to_string(i), a.match);
61+
std::to_string(i), a.str());
6362
ms_dbg_a(transaction, 7, "Added regex subexpression TX." +
64-
std::to_string(i) + ": " + a.match);
65-
transaction->m_matched.push_back(a.match);
63+
std::to_string(i) + ": " + a.str());
64+
transaction->m_matched.push_back(a.str());
6665
i++;
6766
}
6867
}
6968

7069
for (const auto & i : matches) {
71-
logOffset(ruleMessage, i.m_offset, i.m_length);
70+
logOffset(ruleMessage, i.offset(), i.str().size());
7271
}
7372

7473
if (m_string->m_containsMacro) {

‎src/operators/verify_cpf.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -130,14 +130,14 @@ bool VerifyCPF::evaluate(Transaction *t, Rule *rule,
130130
for (i = 0; i < input.size() - 1 && is_cpf == false; i++) {
131131
matches = m_re->searchAll(input.substr(i, input.size()));
132132
for (const auto & i : matches) {
133-
is_cpf = verify(i.match.c_str(), i.match.size());
133+
is_cpf = verify(i.str().c_str(), i.str().size());
134134
if (is_cpf) {
135-
logOffset(ruleMessage, i.m_offset, i.m_length);
135+
logOffset(ruleMessage, i.offset(), i.str().size());
136136
if (rule && t && rule->m_containsCaptureAction) {
137137
t->m_collections.m_tx_collection->storeOrUpdateFirst(
138-
"0", std::string(i.match));
138+
"0", i.str());
139139
ms_dbg_a(t, 7, "Added VerifyCPF match TX.0: " + \
140-
std::string(i.match));
140+
i.str());
141141
}
142142

143143
goto out;

‎src/operators/verify_ssn.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,14 +121,14 @@ bool VerifySSN::evaluate(Transaction *t, Rule *rule,
121121
for (i = 0; i < input.size() - 1 && is_ssn == false; i++) {
122122
matches = m_re->searchAll(input.substr(i, input.size()));
123123
for (const auto & i : matches) {
124-
is_ssn = verify(i.match.c_str(), i.match.size());
124+
is_ssn = verify(i.str().c_str(), i.str().size());
125125
if (is_ssn) {
126-
logOffset(ruleMessage, i.m_offset, i.m_length);
126+
logOffset(ruleMessage, i.offset(), i.str().size());
127127
if (rule && t && rule->m_containsCaptureAction) {
128128
t->m_collections.m_tx_collection->storeOrUpdateFirst(
129-
"0", std::string(i.match));
129+
"0", i.str());
130130
ms_dbg_a(t, 7, "Added VerifySSN match TX.0: " + \
131-
std::string(i.match));
131+
i.str());
132132
}
133133

134134
goto out;

‎src/utils/regex.cc

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,11 @@ namespace Utils {
3939

4040

4141
Regex::Regex(const std::string& pattern_)
42-
: pattern(pattern_),
43-
m_ovector {0} {
42+
: pattern(pattern_.empty() ? ".*" : pattern_)
43+
{
4444
const char *errptr = NULL;
4545
int erroffset;
4646

47-
if (pattern.empty() == true) {
48-
pattern.assign(".*");
49-
}
50-
5147
m_pc = pcre_compile(pattern.c_str(), PCRE_DOTALL|PCRE_MULTILINE,
5248
&errptr, &erroffset, NULL);
5349

@@ -71,7 +67,7 @@ Regex::~Regex() {
7167
}
7268

7369

74-
std::list<SMatch> Regex::searchAll(const std::string& s) {
70+
std::list<SMatch> Regex::searchAll(const std::string& s) const {
7571
const char *subject = s.c_str();
7672
const std::string tmpString = std::string(s.c_str(), s.size());
7773
int ovector[OVECCOUNT];
@@ -83,19 +79,16 @@ std::list<SMatch> Regex::searchAll(const std::string& s) {
8379
s.size(), offset, 0, ovector, OVECCOUNT);
8480

8581
for (i = 0; i < rc; i++) {
86-
SMatch match;
8782
size_t start = ovector[2*i];
8883
size_t end = ovector[2*i+1];
8984
size_t len = end - start;
9085
if (end > s.size()) {
9186
rc = 0;
9287
break;
9388
}
94-
match.match = std::string(tmpString, start, len);
95-
match.m_offset = start;
96-
match.m_length = len;
89+
std::string match = std::string(tmpString, start, len);
9790
offset = start + len;
98-
retList.push_front(match);
91+
retList.push_front(SMatch(match, start));
9992

10093
if (len == 0) {
10194
rc = 0;
@@ -107,24 +100,24 @@ std::list<SMatch> Regex::searchAll(const std::string& s) {
107100
return retList;
108101
}
109102

110-
int regex_search(const std::string& s, SMatch *match,
111-
const Regex& regex) {
103+
int Regex::search(const std::string& s, SMatch *match) const {
112104
int ovector[OVECCOUNT];
113-
int ret = pcre_exec(regex.m_pc, regex.m_pce, s.c_str(),
105+
int ret = pcre_exec(m_pc, m_pce, s.c_str(),
114106
s.size(), 0, 0, ovector, OVECCOUNT) > 0;
115107

116108
if (ret > 0) {
117-
match->match = std::string(s, ovector[ret-1],
118-
ovector[ret] - ovector[ret-1]);
119-
match->size_ = ret;
109+
*match = SMatch(
110+
std::string(s, ovector[ret-1], ovector[ret] - ovector[ret-1]),
111+
0
112+
);
120113
}
121114

122115
return ret;
123116
}
124117

125-
int regex_search(const std::string& s, const Regex& regex) {
118+
int Regex::search(const std::string& s) const {
126119
int ovector[OVECCOUNT];
127-
return pcre_exec(regex.m_pc, regex.m_pce, s.c_str(),
120+
return pcre_exec(m_pc, m_pce, s.c_str(),
128121
s.size(), 0, 0, ovector, OVECCOUNT) > 0;
129122
}
130123

‎src/utils/regex.h

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -31,39 +31,48 @@ namespace Utils {
3131

3232
class SMatch {
3333
public:
34-
SMatch() : size_(0),
35-
m_offset(0),
36-
m_length(0),
37-
match("") { }
38-
size_t size() const { return size_; }
39-
std::string str() const { return match; }
40-
41-
int size_;
42-
int m_offset;
43-
int m_length;
44-
std::string match;
45-
};
34+
SMatch()
35+
: m_match(), m_offset(0)
36+
{}
37+
38+
SMatch(const std::string &match, size_t offset)
39+
: m_match(match), m_offset(offset)
40+
{}
41+
42+
const std::string& str() const { return m_match; }
43+
size_t offset() const { return m_offset; }
4644

45+
private:
46+
std::string m_match;
47+
size_t m_offset;
48+
};
4749

4850
class Regex {
4951
public:
5052
explicit Regex(const std::string& pattern_);
5153
~Regex();
52-
std::string pattern;
53-
pcre *m_pc = NULL;
54-
pcre_extra *m_pce = NULL;
55-
int m_ovector[OVECCOUNT];
56-
57-
std::list<SMatch> searchAll(const std::string& s);
58-
};
5954

55+
// m_pc and m_pce can't be easily copied
56+
Regex(const Regex&) = delete;
57+
Regex& operator=(const Regex&) = delete;
6058

61-
int regex_search(const std::string& s, SMatch *m,
62-
const Regex& regex);
59+
std::list<SMatch> searchAll(const std::string& s) const;
60+
int search(const std::string &s, SMatch *m) const;
61+
int search(const std::string &s) const;
6362

64-
int regex_search(const std::string& s, const Regex& r);
63+
const std::string pattern;
64+
private:
65+
pcre *m_pc = NULL;
66+
pcre_extra *m_pce = NULL;
67+
};
6568

69+
static inline int regex_search(const std::string& s, SMatch *match, const Regex& regex) {
70+
return regex.search(s, match);
71+
}
6672

73+
static inline int regex_search(const std::string& s, const Regex& regex) {
74+
return regex.search(s);
75+
}
6776

6877
} // namespace Utils
6978
} // namespace modsecurity

‎test/regression/regression.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ void perform_unit_test(ModSecurityTest<RegressionTest> *test,
202202
SMatch match;
203203
std::string s = modsec_rules->getParserError();
204204

205-
if (regex_search(s, &match, re) && match.size() >= 1) {
205+
if (regex_search(s, &match, re)) {
206206
if (test->m_automake_output) {
207207
std::cout << ":test-result: PASS " << filename \
208208
<< ":" << t->name << std::endl;

‎test/unit/unit_test.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,15 @@ void json2bin(std::string *str) {
6262
modsecurity::Utils::Regex re2("\\\\u([a-z0-9A-Z]{4})");
6363
modsecurity::Utils::SMatch match;
6464

65-
while (modsecurity::Utils::regex_search(*str, &match, re)
66-
&& match.size() > 0) {
65+
while (modsecurity::Utils::regex_search(*str, &match, re)) {
6766
unsigned int p;
6867
std::string toBeReplaced = match.str();
6968
toBeReplaced.erase(0, 2);
7069
sscanf(toBeReplaced.c_str(), "%x", &p);
7170
replaceAll(str, match.str(), p);
7271
}
7372

74-
while (modsecurity::Utils::regex_search(*str, &match, re2)
75-
&& match.size() > 0) {
73+
while (modsecurity::Utils::regex_search(*str, &match, re2)) {
7674
unsigned int p;
7775
std::string toBeReplaced = match.str();
7876
toBeReplaced.erase(0, 2);

0 commit comments

Comments
 (0)