Skip to content
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.

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

Feature request: adding severity in ModSecurityIntervention #2748

Open
wants to merge 2 commits into
base: v3/master
Choose a base branch
from

Conversation

FedericoHeichou
Copy link

Hi, I think would be very useful adding in ModSecurityIntervention a severity field populated by disruptives evaluate function.
In this way a connector can use the intervention.log based the severity of the log.

For example owasp-modsecurity/ModSecurity-nginx#274

@martinhsv
Copy link
Contributor

Hello @FedericoHeichou ,

Could you describe more fully what you're hoping to achieve here. Perhaps a specific example that outlines the tangible benefit?

@FedericoHeichou
Copy link
Author

Hello @FedericoHeichou ,

Could you describe more fully what you're hoping to achieve here. Perhaps a specific example that outlines the tangible benefit?

Hello @martinhsv
For example in the nginx connector Modsecurity-nginx there are these lines of code

    log = intervention.log;
    if (intervention.log == NULL) {
        log = "(no log message was specified)";
    }

    ngx_log_error(NGX_LOG_ERR, (ngx_log_t *)r->connection->log, 0, "%s", log);

As you can see it prints everything with the severity level NGX_LOG_ERROR. If I set in a rule that a transaction has a severity of DEBUG the connector should know the severity, so it can use it to handle own right log level. With the code committed the Modsecurity-nginx code could became

    log = intervention.log;
    if (intervention.log == NULL) {
        log = "(no log message was specified)";
    }

    ngx_log_error(intervention.severity, (ngx_log_t *)r->connection->log, 0, "%s", log);

(I don't know if they uses the same level so we should check if the levels are the same but it is just an example)

@martinhsv
Copy link
Contributor

HI @FedericoHeichou ,

Ignore the source code for now. What I was really prompting for was a description of what the benefit is, or what the user-visible effect is.

Suppose you have two rules, something like:

SecRule ... "... id:1001, severity:CRITICAL"
SecRule ... "... id:1002, severity:WARNING"

And the nginx config includes:

error_log /var/log/nginx/error.log error;

With your suggestion here, how would rules 1001 and 1002 be treated differently? What would look different as far as the admin is concerned? And how is this different from current functionality?

@FedericoHeichou
Copy link
Author

HI @FedericoHeichou ,

Ignore the source code for now. What I was really prompting for was a description of what the benefit is, or what the user-visible effect is.

Suppose you have two rules, something like:

SecRule ... "... id:1001, severity:CRITICAL"
SecRule ... "... id:1002, severity:WARNING"

And the nginx config includes:

error_log /var/log/nginx/error.log error;

With your suggestion here, how would rules 1001 and 1002 be treated differently? What would look different as far as the admin is concerned? And how is this different from current functionality?

With your example it now prints both 1001 and 1002 even though you set the logger level to "error" and the 1002 is "warning" and the nginx.conf asking to add to the error.log file only >= errors.
Adding the severity in the "intervention" it would be possible to modify a connector to print the intervention in the right logging level, in this way only 1001 would be added in the log file with the error log you specified.
As a sysadm I added the code I committed to my running modsecurity (and modified modsecurity-nginx too) because it was creating too much logs because every severity was threaten like a "error"

@martinhsv
Copy link
Contributor

martinhsv commented Aug 30, 2022

I see. I can think of at least a couple of reasons to be hesitant about an approach like that.

  1. Although both Apache HTTP Server and nginx use comparable log level structures, we cannot (and should not) assume that that will always be the case. Creating unnecessary dependencies and linkages makes ModSecurity less flexible for the future. For example, I believe IIS already does not have the same syslog-inspired log levels.

  2. I don't think we should assume that it's even desirable that there be a direct mapping between the 'severity' action of a ModSecurity rule and the severity of any output in error.log. They are intended to reference different things. One is the relative severity of a ModSecurity rule, while the other is the relative severity of an effect on the web server. To make an analogy. if a web server might result in output to syslog in some cases, I wouldn't assume that the syslog level must be the same as a related message in the web server's error.log. And must a 'CRITICAL' rule really generate a 'crit' level log line in the error.log rather than 'error'?

  3. [-- or maybe it's just 2b)] Tying the two severities together makes it impossible for a rule to generate different error.log severities under circumstances separate from the rule itself. For example, if a rule has a severity of ERROR, we may want that rule to result in an 'error' level message in error.log when SecRuleEngine On but not when SecRuleEngine DetectionOnly

@FedericoHeichou
Copy link
Author

FedericoHeichou commented Aug 30, 2022

I see. I can think of at least a couple of reasons to be hesitant about an approach like that.

  1. Although both Apache HTTP Server and nginx use comparable log level structures, we cannot (and should not) assume that that will always be the case. Creating unnecessary dependencies and linkages makes ModSecurity less flexible for the future. For example, I believe IIS already does not have the same syslog-inspired log levels.
  2. I don't think we should assume that it's even desirable that there be a direct mapping between the 'severity' action of a ModSecurity rule and the severity of any output in error.log. They are intended to reference different things. One is the relative severity of a ModSecurity rule, while the other is the relative severity of an effect on the web server. To make an analogy. if a web server might result in output to syslog in some cases, I wouldn't assume that the syslog level must be the same as a related message in the web server's error.log. And must a 'CRITICAL' rule really generate a 'crit' level log line in the error.log rather than 'error'?
  3. [-- or maybe it's just 2b)] Tying the two severities together makes it impossible for a rule to generate different error.log severities under circumstances separate from the rule itself. For example, if a rule has a severity of ERROR, we may want that rule to result in an 'error' level message in error.log when SecRuleEngine On but not when SecRuleEngine DetectionOnly
  1. I think it should be the connector's task to map the "standard" (maybe the facto) logging level to its logging level
  2. You are right but I could ask you: do you really want a DEBUG/INFO severity rule being logged as ERROR in a connector? Modsecurity has the settings to prevent the execution of the callback (setting it to warn will never execute the logging callback with info), still the severity is not provided in the response, so a connector can not know how to act. A better example of usage can just be a connector can provide a setting of "modsecurity-max-logging-level" and for example if you set to WARN it will send to logger everything as ERROR but only with level >= WARN. If a connector doesn't provide this setting or a user doesn't use it it will print everything as the current implementation. I think this would be a better usage of this intervention->severity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants