📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

Although the quantitative testing in CI didn't find any problems, I suspect that regexp is too loose, and may cause false positives in the real world. A bit of searching found this post:

Wow...\n\n{{{{{{{{{{{{Hugs}}}}}}}}}}}}} (if you want them)

which would trigger a false positive. There are surely other examples.

See for example commit 2ac26cc
which added support for a different flavor of SSTI; it went to the trouble of actually matching dangerous things between the brackets to avoid false positives.

You want tests for actual harmful requests, too, not just probes like {{ 7 * 7 }}.
And you probably want a test for the post I dredged up, demonstrating that a false positive is not triggered.

Note also that when one adds a regexp, one is expected to provide regex assembly source; the commit I linked to does this in file util/regexp-assemble/data/934100.data etc.

#3780 is about bash; this is about SSTI. They may look similar because of the braces, but they're very different.

Thank you for providing this PR and also the associated issue. The PR already looks great, and there's even a test file.
The linting and the regression tests are failing. I think the payload in the test file {{ 7*7 }} should be url encoded. Then the regression tests are successful on my machine.

It seems {{ }} is used by a large number of template languages, e.g.:

• https://community.klaviyo.com/marketing-30/using-the-first-name-code-445 dynamically populate the First Name: {{ first_name|default:'' }}
• https://www.geeksforgeeks.org/what-does-mean-in-yaml/ Before diving into {{ }}, let's briefly recap some YAML basics.
• Can Helm support to ignore {{expr}} which is just for configuration but not render? helm/helm#2798 Can Helm support to ignore {{expr}}
• https://help.marketpath.com/liquid/object/text {{text}}
• https://www.microfocus.com/documentation/extend-acucobol/925/GUID-83FF0016-6945-475A-A9D2-F76972CB74E6.html The {{Value}} Tag
• https://www.home-assistant.io/docs/configuration/templating/ {{ ... }}
• https://www.reddit.com/r/wikia/comments/1jcf4my/whats_the_difference_between_misubject_and/ difference between {{Mi|subject}} and {{Li|Subject}}
• https://www.reddit.com/r/CharacterAI_Guides/comments/1c2zqeu/user_vs_random_user/ {{user}} vs {{random_user}}

so you might want to either change the name of the rule to "Generic SSTI", or add keywords to make it specific to Flask and call it "Flask SSTI" (following the example set by a different SSTI rule in #2578 ).

Also, it's used in non-template language:

• https://www.reddit.com/r/elderscrollslegends/comments/5dmkms/comment/da60vfe/ {{Master of Thieves}} is a 3/5. I've never used Toby but I think the effect is worth the statline.
• https://www.reddit.com/r/learnprogramming/comments/kb96e/what_is_the_difference_between_a_and_a/ What is the difference between {a} and {{a}}?
• https://www.reddit.com/r/learnmath/comments/slgpor/understanding_why_its_true_is_tf/ Understanding why it's true... is {{∅}} ⊂ {∅,{∅}} T/F

thus crs standard advice "disable any rule that causes false positives for your users" will apply to this rule.

@franbuehler
Sure I will check your comments, very glad to have your reviews :)

@dkegel-fastly
I will check your comments as well thanks :)

But regarding your claim that the double parentheses is used in a lot of things, what is the point that some users may pass such expression in a query parameter for example or for a cookie??

Even if there is a case and it is very rare, I think they can be bypassed by the folks whoever will face any issue for this rule, what do you think?

I'm more worried about it happening in the body; that's where long user input goes.
And likely it'll go into a query parameter, since web apps don't tend to use bare bodies much these days.

This is already covered by rule 941380:

We need to update our documentation so that users stop reporting bypasses at PL1, when they're covered at higher PLs. Closing.

That's true. I didn't test for it.

curl -v -H "x-crs-paranoia-level: 2" -H "X-Format-Output: txt-matched-rules" 'sandbox.coreruleset.org/get?x=%7B%7B%207%2A7%20%7D%7D'
941380 PL2 AngularJS client side template injection detected
949110 PL1 Inbound Anomaly Score Exceeded (Total Score: 5)
980170 PL1 Anomaly Scores: (Inbound Scores: blocking=5, detection=5, per_pl=0-5-0-0, threshold=5) - (Outbound Scores: blocking=0, detection=0, per_pl=0-0-0-0, threshold=4) - (SQLI=0, XSS=5, RFI=0, LFI=0, RCE=0, PHPI=0, HTTP=0, SESS=0, COMBINED_SCORE=5)

However, case 2 is not covered yet:
/get?x={ { 7*7 } }"

curl -v -H "x-crs-paranoia-level: 2" -H "X-Format-Output: txt-matched-rules" 'sandbox.coreruleset.org/get?x=%7B%20%20%20%20%20%20%20%20%20%7B%207%2A7%20%7D%20%20%20%7D'

Do we want to extend the existing rule 941380 so that this case is covered as well?

Do we want to extend the existing rule 941380 so that this case is covered as well?

Yes, that'd make sense. Also, we need to update the comments to mention that the payload covers other template/non-template languages as mentioned by @dkegel-fastly.

UPDATE: I just tested with spaces, and the code is unable to evaluate due to incorrect syntax. The curly braces need to be close, atleast for Python, which I've tested.

I just checked, and we need to cover the the {% ... %} Python Jinja2 template statement syntax.

https://jinja.palletsprojects.com/en/stable/templates/#list-of-control-structures

 curl -H "x-format-output: txt-matched-rules" "https://sandbox.coreruleset.org/?test=%7B%25%20if%20True%20%25%7D%20yay%20%7B%25%20endif%20%25%7D" -H "x-crs-version: nightly" -H "x-crs-paranoia-level: 4"

We could cover this with:
{[{%].*?[}%]}

So this would be enough when the curly braces need to be close.

Not sure how to implement this, rule 941380 is XSS related, although still a template injection vuln. What do you suggest, remove that rule, maintain this PR. and change the title to Template Injection, to make it generic.

@Xhoenix Are you sure that the rule: 941380 could block the {{.*}} regex?? I have tested it multiple of times and I could still bypass the coreruleset.

@franbuehler I see that this rule: 941380 doesn't make any kind of transformation, should we add the removeWhiteSpace for example? i.e. t:removeWhiteSpace instead of t:none

Not sure how to implement this, rule 941380 is XSS related, although still a template injection vuln. What do you suggest, remove that rule, maintain this PR. and change the title to Template Injection, to make it generic.

@Xhoenix I'm not entirely sure, but I think we should move the rule to the right place. The downside is that some users may have written tuning rules to this rule 941180. Now the rule will be moved to a different ID, and the tuning rule will no longer apply. Maybe other opinions?

Are you sure that the rule: 941380 could block the {{.*}} regex?? I have tested it multiple of times and I could still bypass the coreruleset.

@TheRubick Yes, the regex of rule 941380 is almost the same: {{.*?}}. The only difference is the laziness.
You can test it with:
curl -v -H "x-crs-paranoia-level: 2" -H "X-Format-Output: txt-matched-rules" 'sandbox.coreruleset.org/get?x=%7B%7B%207%2A7%20%7D%7D'

I see that this rule: 941380 doesn't make any kind of transformation, should we add the removeWhiteSpace for example? i.e. t:removeWhiteSpace instead of t:none

@TheRubick Yes, this would also be the solution to match the second test case: { { 7*7 } }

@franbuehler all options are available on the table :)
If you wanna schedule a meeting to put the boundaries and the main decisions for this PR, I am fine as well :)

@franbuehler @Xhoenix
I have add multiple changes:

• Add the {%%} and <%%> to our regex
• Removed unnecessary comments
• Added more regression tests.
  But my gut tells me that we no need the removeWhiteSpace filter as the payload will be useless. So should I remove this filter in addition to every associated regression tests related to the white space case?? i.e. remove the regressions test from id 5 to 8??

Also, can we be confirmed from removing the old rule from the XSS??

I have added the angle brackets plus percentage sign test case and regex as well as I have found this statement in the Jinja2 template official documents: An application developer can change the syntax configuration from {% foo %} to <% foo %>, or something similar, Reference: https://jinja.palletsprojects.com/en/stable/templates/

As well as I have found some SSTI payload can be exploited using this type of regex e.g. <%= File.open('/etc/passwd').read %>, Reference: https://github.com/payloadbox/ssti-payloads

@franbuehler

@Xhoenix I'm not entirely sure, but I think we should move the rule to the right place. The downside is that some users may have written tuning rules to this rule 941180. Now the rule will be moved to a different ID, and the tuning rule will no longer apply. Maybe other opinions?

In my humble opinion, this fact should be concerned in every open source project :) I mean this may happen to someone who is using an open source library but didn't check the release notes. Does that make sense??

I see that we can remove the old rule and broadcast that we have changed to something else, folks/users should be synced to the latest change.

@franbuehler I have added all the changes that we have agreed upon from the monthly meeting :)
If I have missed anything else or you have any other thoughts, just shout it out xD

I will add a comment to the existing rule 941380 once this PR is ready.

@franbuehler

I will add a comment to the existing rule 941380 once this PR is ready.

I have added a comment already, did you check it?
https://github.com/coreruleset/coreruleset/pull/4145/files#diff-b43639c57f48880d643410a3d423a4fa3daf0681be74978afc9b1e4214122c0bR361

Linting fails because of trailing spaces in tests/regression/tests/REQUEST-934-APPLICATION-ATTACK-GENERIC/934180.yaml.

@TheRubick Can you fix the test's yaml?

@franbuehler @fzipi Done, waiting for your sweet lgtm :)
Sorry for late response as well but I was afk from my machine.

Linting is still complaining about a trailing space.

@franbuehler done
I have installed the yamllint in my ubuntu machine and I think it should be fine rn :)
Sorry for this confusion :)

@fzipi released 4.15.0 a few days ago. Now linting is complaining about a wrong ver tag.
Can you change the ver to the new version: ver:'OWASP_CRS/4.16.0-dev'.
Then it should be good.

I also commited @Xhoenix changes of the .* pattern.

Updated the action version, done :)

@fzipi I have added the new tag, hopefully so I have adjusted it appropriately :)