removeComments() overzealous parse error and possible security bypass

[katef]

This ticket describes two related issues: a possible security bypass, and a design flaw.

The removeComments() transformation is over zealous, in that it does not consider context for potential comments. For example, char *s = "abc /* ... */ xyz"; would be considered a comment inside the string.

This function also professes to strip comments for several languages, but does not have knowledge of which language it is handling. For example, char *s = "abc -- xyz"; would be considered a comment at the --, and will strip to the end of line. This applies to any language - particularly for SQL injections or XSS similar; I'm just using C syntax here as an example of the general case.

This gives a possible security bypass: If rules match patterns after the removeComments() transformation is applied, then content which should be dissallowed could be placed (e.g. after the string in the above example), and would thus be elided for the rule pattern matching.

To strip comments correctly for multiple languages, the function would need to handle strings for those languages, as in the above examples, Special characters within strings are escaped differently for each language, but this transformation is invoked without knowledge of which language is being parsed.

Without knowledge of the language being parsed, I do not see how this function could ever be implemented correctly.

And a documentation issue for the same: The documentation for removeComments() only described stripping C-style comments. This should be updated to reflect what the function actually does.

[csanders-git]

This was actually an issue that I brought up and someone else brought up. Anything that deals with remove has a possibility of causing way more harm than it's worth -- because it'd have to do so iteratively. You also mention some valid points about its documentation, As a result, this transformation is pretty weak.

@zimmerle correct me if i'm wrong but i believe we talked about removing it in 3.1.

I think a reasonable solution is to aim for it's removal - I think this should be noted however in the reference manual currently - @katef would you agree?

whether it can result in a bypassed is a result of the rule you write and the data coming in but it certainly can result in unexpected processing. In many cases triggering such functionality might result in malformed comments also (but we shouldn't rely on this)

It isn't used within owasp CRS for this very reason.

[zimmerle]

yes, removeComments will be deprecated as of v3.0

[fgsch]

Is there any update on this?

I can't see any update on the wiki mentioning its deprecation and at least rule 942100 is using it.

It'd be nice to have some clarification.

[csanders-git]

i'll open a ticket to remove it from 942100

[fgsch]

i'll open a ticket to remove it from 942100

👍

Thanks!

[victorhora]

@fgsch Just to clarify t:removeComments is currently supported in libModSecurity and I'm not sure if it we will deprecate on the next release of ModSecurity. Tagging @zimmerle here to confirm.

Anyways I've added a small note to the reference manual about potential issues with this transformation and the fact that it could be deprecated in the future.