MS3 false positive: incorrect match of HTTP POST body with string in`lfi-os-files.data'

[mmelo-yottaa]

Describe the bug

strings in lfi-os-files.data beginning with "." are incorrectly matched against HTTP requests, "Matched "Operator PmFromFile' with parameter lfi-os-files.data'" is incorrectly reported and traffic is blocked.

Logs and dumps

2019/07/24 19:16:45 [warn] 51957#51957: *22640 [client 127.0.0.1] ModSecurity: Access denied with code 403 (phase 2). Matched "Operator PmFromFile' with parameter lfi-os-files.data' against variable ARGS_NAMES:json.profile.array_0' (Value: json.profile.array_0' ) [file "/etc/yottaa-nginx/modsecurity.d//site.d/www_website_com/../crs3.0.2/REQUEST-930-APPLICATION-ATTACK-LFI.conf"] [line "9"] [id "930120"] [rev "4"] [msg "OS File Access Attempt"] [data "Matched Data: .profile found within ARGS_NAMES:json.profile.array_0: json.profile.array_0"] [severity "2"] [ver "OWASP_CRS/3.0.0"] [maturity "9"] [accuracy "9"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-lfi"] [tag "OWASP_CRS"] [tag "OWASP_CRS/WEB_ATTACK/FILE_INJECTION"] [tag "WASCTC/WASC-33"] [tag "OWASP_TOP_10/A4"] [tag "PCI/6.5.4"] [hostname "127.0.0.1"] [uri "/www_website_com"] [unique_id "15639958055.218714"] [ref "o4,8v0,20t:utf8toUnicode,t:urlDecodeUni,t:normalizePathWin,t:lowercaseo4,8v0,20t:utf8toUnicode,t:urlDecodeUni,t:normalizePathWin,t:lowercaseo4,8v0,20t:utf8toUnicode,t:urlDecodeUni,t:normalizePathWin,t (10 characters omitted)"], client: 127.0.0.1, server: _, request: "POST /www_website_com HTTP/1.1", host: "localhost:8080"

Use the curl command below, MS3 will block, MS2 will not. If you change "profile" to ".profile" in the curl command JSON, MS2 will then correctly block.

To Reproduce

notice the "profile" string in the JSON:

curl --header "Content-Type: application/json" --request POST --data '{"profile":["JimmyJohn","","99999999999"],"shippingAddress":["800 main street","United States of America","99999","Waltham","MA","Residential","",""],"validate":false,"addressId":""}' http://localhost:8080/www_website_com

Expected behavior

I expected MS3 to allow this HTTP POST (as MS2 does).

Server (please complete the following information):
ModSecurity v3.0.3 commit 6ab464a
nginx version: openresty/1.13.6.2
ModSecurity-nginx-1.0.0
CentOS Linux release 7.3.1611 (Core) kernel: 3.10.0-514.16.1.el7.x86_64

Rule Set (please complete the following information):
OWASP CRS 3.0.2

Additional context

[airween]

I think the bad news is that this behavior is by design - see this comment under the #1576 (but I don't think that was the intention).

The keys of values under the profile key in your JSON array will renamed to json.profile.array_0, json.profile.array_1, ..., as you can see in your pasted log line. So, the .profile pattern matches with these keys - that's why the request is false positive.

More bad news that this issue could occur not just with the @pmf, but with all operator which checks the pattern matches, eg. @rx array, @beginsWith json, @contains .array, and so on.

Edit: what you can do as quick workaround, extend the rule id:200001 like this:

SecRule REQUEST_HEADERS:Content-Type "application/json" \ 
        "id:'200001',phase:1,t:none,t:lowercase,pass,nolog,ctl:requestBodyProcessor=JSON,ctl:ruleRemoveById=930120"

in your modsecurity.conf, line 30.

[mmelo-yottaa]

Thanks for the insight, I will take a look at MS3 source to see the details. Is it possible to limit the key renaming to cases where the JSON contains key:value pairs where the key is not specified? That would make MS3 more back-compatible with MS2...

[airween]

With this patch you can avoid to renaming of JSON variables:

diff --git a/src/request_body_processor/json.cc b/src/request_body_processor/json.cc
index a6dd4ad4..9279156b 100644
--- a/src/request_body_processor/json.cc
+++ b/src/request_body_processor/json.cc
@@ -118,15 +118,15 @@ int JSON::addArgument(const std::string& value) {
     std::string data("");
     std::string path;
 
-    for (size_t i =  0; i < m_containers.size(); i++) {
+    for (size_t i =  1; i < m_containers.size(); i++) {
         JSONContainerArray *a = dynamic_cast<JSONContainerArray *>(
             m_containers[i]);
         path = path + m_containers[i]->m_name;
-        if (a != NULL) {
+        /* if (a != NULL) {
             path = path + ".array_" + std::to_string(a->m_elementCounter);
         } else {
             path = path + ".";
-        }
+        } */
     }
 
     if (m_containers.size() > 0) {

So, the profile: ['foo', ...] will remain as profile, foo, ..., not moving to json.profile.array_0. I think this is the most compatible with v2 :), but I guess there was an important reason to build this part to the code. Eg. with this patch some regression tests will failed, eg. this.

Please note, that this is just an ugly test, not a recommended solution. With this, your HTTP post worked with all operators as well.

For the finally fix it should be know the idea behind the renaming. Hope that @zimmerle will read this issue soon, and can help :).

[zimmerle]

Hi @mmelo-yottaa,

It seems to me that it is more a rule issue than an engine problem. Matching with the key name in that scenario does not sound like a good idea. Changing the behavior of the engine may lead you to other consequences.

Important to notice that this naming schema was first made on v3 and ported to v2, not the other way around.

[airween]

Hi @zimmerle,

sorry, I'm not sure that I understand this:

Matching with the key name in that scenario does not sound like a good idea.

You mean the end-users have to know, which keys are disabled when they use JSON request?

And for example if they buy a commercial application, then they needs to replace the string occurrence in whole code? :)

[zimmerle]

Hi @airween,

The user is trying to execute the operator PmFromFile at ARGS_NAMES which makes a lookup for the string .profile.

The ARGS_NAMES is filled with an information collected from a JSON, as listed:

{ 
   "profile":[ 
      "JimmyJohn",
      "",
      "99999999999"
   ],
   "shippingAddress":[ 
      "800 main street",
      "United States of America",
      "99999",
      "Waltham",
      "MA",
      "Residential",
      "",
      ""
   ],
   "validate":false,
   "addressId":""
}

According to the way that ModSecurity maps the array to a collection we are going to have:

ARGS_NAMES:json.profile.array_0

That is being identified by the rule as a lfi. If I understood correctly you think that this specific rule is right and ModSecurity code needs to be changed in order to make the rule workable. Is that it?

[airween]

I think the problem is with this behavior:

• lfi list contains ".profile" pattern (explicitly with a dot - not "profile", without dot)
• user send "profile" pattern without dot, not ".profile"
• even so, the engine evaluates it as matching

It's similar, if the rule contains @rx array, and looks up the ARGS_NAMES|ARGS|XML:/* - but none of them contains array - the engine now will catch it.

Hope now it's clear.

[zimmerle]

Hi @airween,

I don't think there is a misunderstanding from my side, let me put in a different way -

The reason it matches is the fact that there is a naming convention on version 3 which allow the user to navigate/select an element of the JSON using its keys names.

SecRule ...ARGS:json.profile.array_0... "test" "id:0,log"

If you think there is a problem in that structure, we will be pleased to consider a change. For that, I would recommend a new issue and the technical reasons that motivate the change. Before open any new issue, please consider seeing the past discussion on this subject here on GitHub.

Other than that, ARGS_NAMES:json.profile.array_0 does not look like a lfi; Therefore it seems to me like a rule issue. I would suggest a chained rule to that one, or better payloads on the lfi data file. profile in a very common word. Thus I am closing this issue.

If the doubt was not yet mitigated, please consider to re-open the issue.

[airween]

Hi @zimmerle,

of course, you (and me too) understand the reason, why matches this pattern in the given example.

I'm talking about the effect (users don't know the reason, there isn't any documentation - except this issue), the possible solution, and about my opinion, which is I can't agree that there are some words (one or more - it doesn't matter) what user can't use in rule.

Other than that, ARGS_NAMES:json.profile.array_0 does not look like a lfi;

Correct - but the subpart of this pattern is look like a lfi.

Therefore it seems to me like a rule issue. I would suggest a chained rule to that one, or better payloads on the lfi data file.

It sounds like a possible solution :)

profile in a very common word

Sorry, I can't agree this opinion - I think any engine has to work without any word dependency, except the used keywords.

There can't be that like common word.

For eg. in HTTP protocol, the Cookie is a reserved word for a header name, and obviously the engine has to recognize it. But IMHO there can't be an expression in this terminology, like common word.

I think we can use an another possible solution, but may be it's a bit complex. There is the naming convention as you described above, which is nearly constant. It's a transformation for the whole payload. What do you think about if we make a reverse transformation for the transformed payload, if the CT header is application/json, and the case of pattern-matching-like operators (@rx, @beginsWith, @endsWith, @contains, ...) we applied the re-transformed value? Whit this solution, the engine would independent from any pattern. Or we don't need to make the reverse transformation, enough to store the original value, and apply it for the operators in the case.

Any idea?

[zimmerle]

@airween there is no limitation in the word naming from the engine. The word profile came from to different places: the rule and the client's payload (json).

[airween]

Yes, but the rule and the client's payload patterns are not equals: the rule pattern is wider than the payload. Therefore there shouldn't match the payload with the rule pattern.

So far it's because the engine used that word too.

What you wrote about the chaining rule can be a solution. But what do you think about my idea?

[zimmerle]

@airween what is the problem that you are trying to solve?

[airween]

Consider a SecRule with this operator and an argument:

@rx \.ANYWORD

and imagine that you send a request with application/json content-type, and your json payload contains a key with name 'ANYWORD' - not '.ANYWORD', just 'ANYWORD', without dot.

From the point of users view, you can really agree that this request has to match with the given rule?

You can say, that this isn't a bug because the reason is the engine works that way - but again: from the point of users view it's an abnormal operation until we give a satisfactory answer.

But here is the another aspect of this problem: could you help to the user, how can he solve this situation?

Consider the user wants to catch the request which contains the pattern \.profile (the dot is not a metachar), but he wants to use a key with name profile. What's your solution?