Skip to content

Commit c947f5e

Browse files
committed
Do not assume ModSecurityIntervention argument to transaction::intervention has been initialized/cleaned
- Keep m_it->disruptive value and use it as return value to guarantee that the value is correct. - If m_it->disruptive is false and the 'it' argument has not been initialized/cleaned, the function may incorrectly return a non-zero value. - When a disruptive intervention is being reported by the function, defensively initialize log & url to NULL if there's no such data to provide to the caller. - If the caller has not initialized/cleaned those fields in the 'it' argument, after returning from transaction::intervention, the user can safely read the log & url fields and in all scenarios they'll have valid values.
1 parent 68d551c commit c947f5e

File tree

1 file changed

+21
-6
lines changed

1 file changed

+21
-6
lines changed

src/transaction.cc

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1469,16 +1469,24 @@ int Transaction::processLogging() {
14691469
* @brief Check if ModSecurity has anything to ask to the server.
14701470
*
14711471
* Intervention can generate a log event and/or perform a disruptive action.
1472+
*
1473+
* If the return value is true, all fields in the ModSecurityIntervention
1474+
* parameter have been initialized and are safe to access.
1475+
* If the return value is false, no changes to the ModSecurityIntervention
1476+
* parameter have been made.
14721477
*
1473-
* @param Pointer ModSecurityIntervention structure
1478+
* @param it Pointer to ModSecurityIntervention structure
14741479
* @retval true A intervention should be made.
14751480
* @retval false Nothing to be done.
14761481
*
14771482
*/
14781483
bool Transaction::intervention(ModSecurityIntervention *it) {
1484+
const auto disruptive = m_it.disruptive;
14791485
if (m_it.disruptive) {
14801486
if (m_it.url) {
14811487
it->url = strdup(m_it.url);
1488+
} else {
1489+
it->url = NULL;
14821490
}
14831491
it->disruptive = m_it.disruptive;
14841492
it->status = m_it.status;
@@ -1489,11 +1497,13 @@ bool Transaction::intervention(ModSecurityIntervention *it) {
14891497
utils::string::replaceAll(&log, std::string("%d"),
14901498
std::to_string(it->status));
14911499
it->log = strdup(log.c_str());
1500+
} else {
1501+
it->log = NULL;
14921502
}
14931503
intervention::reset(&m_it);
14941504
}
14951505

1496-
return it->disruptive;
1506+
return disruptive;
14971507
}
14981508

14991509

@@ -2260,11 +2270,16 @@ extern "C" void msc_transaction_cleanup(Transaction *transaction) {
22602270
*
22612271
* Intervention can generate a log event and/or perform a disruptive action.
22622272
*
2263-
* @param transaction ModSecurity transaction.
2273+
* If the return value is not zero, all fields in the ModSecurityIntervention
2274+
* parameter have been initialized and are safe to access.
2275+
* If the return value is zero, no changes to the ModSecurityIntervention
2276+
* parameter have been made.
22642277
*
2265-
* @return Pointer to ModSecurityIntervention structure
2266-
* @retval >0 A intervention should be made.
2267-
* @retval NULL Nothing to be done.
2278+
* @param transaction ModSecurity transaction.
2279+
* @param it Pointer to ModSecurityIntervention structure
2280+
* @returns If an intervention should be made
2281+
* @retval >0 A intervention should be made.
2282+
* @retval 0 Nothing to be done.
22682283
*
22692284
*/
22702285
extern "C" int msc_intervention(Transaction *transaction,

0 commit comments

Comments
 (0)