Semgrep: Semgrep 0.15.0 performance issues

[ajinabraham]

Describe the bug

$  time semgrep -f ~/Code/njsscan/njsscan/rules/semantic_grep/ ~/Downloads/juice-shop-master/
running 105 rules...
 10%|███████████████▍                                                                                                                                   |11/105

More than 15 minutes and still waiting.

Rules: https://github.com/ajinabraham/njsscan/tree/master/njsscan/rules/semantic_grep
Source: https://github.com/bkimminich/juice-shop

Enhancement

It would be nice to add an end to end test suite with semgrep that runs before release updates on real apps to catch performance bugs.

Environment

$ semgrep --version
0.15.0

[ajinabraham]

Also is there a way to identify rules that are slow?

[ajinabraham]

It took ~1 minute to scan single file with 105 rules

time semgrep -f ~/Code/njsscan/njsscan/rules/semantic_grep/ ~/Downloads/dvna-master/server.js
running 105 rules...
 10%|███████████████▍
...SNIPPED...
real    0m53.124s
user    0m12.414s
sys 0m8.551s

[minusworld]

Also is there a way to identify rules that are slow?

Hey @ajinabraham, we are actually working on gathering performance stats for each rule right now. We also need to know which rules are fast and slow. This'll probably be done tomorrow or Monday and we'll be able to check out runtimes on all the rules together.

[minusworld]

@ajinabraham From the JS rules, it looks like audit.puppeteer-injection eats up a LOT of time. You can check out the runtimes of all rules in this chart:

[ajinabraham]

@minusworld Nice!
Can I run this benchmark locally? Also, can we know which file contributes to the most time?

[minusworld]

@ajinabraham
The benchmarks are now set to run on our semgrep-rules repo. It uses pytest and pytest-benchmark, so you should be able to run the benchmarks locally.

You can pass in rule-directory and a git-repo.

Something like this should work:

$ pytest --timeout=60 --rule-directory=c --git-repo=https://github.com/zlib-ng/zlib-ng tests/performance/test_public_repos.py

It doesn't drill down into specific files, but generally speaking, our observations are that really large files (for example, more than 10,000 lines) are the slowest for Semgrep. This frequently means minified JS files also cause a lot of issues.

[ajinabraham]

@minusworld, Looks like I am not able to run this test. It's extremely slow.

pytest --timeout=60 --rule-directory=../njsscan/njsscan/rules/semantic_grep/ --git-repo=https://github.com/ajinabraham/dvna.git  tests/performance/test_public_repos.py

I think there needs to be a feature with semgrep that allows timeout within the core to ignore rules if they go beyond a threshold. Or maybe ignore minified files. It might be sensible for nodejsscan to do that since we don't expect minified server side javascript. I am not sure about semgrep's client side javascript rules though.

[minusworld]

@ajinabraham I... meant to mention above that it takes about an hour to run. My bad! 🤗

We scheduled it to run periodically since the benchmark tests take a while. Here's the run that ran over the weekend on a fork that I made to test it out: https://github.com/minusworld/semgrep-rules/actions/runs/175034968

r2c has discussed a hard timeout or skipping minified files, and your experience is another data point that will probably push us in that direction. Thanks for being patient (heh, ironic considering we're talking about speed). I'll see what folks think.

[minusworld]

@ajinabraham

I removed rules that was slower than 1,000 lines of code per second from some of the rule packs we distribute (this one in particular)[https://semgrep.live/p/r2c-security-audit] and that brought things to a tolerable speed.

$ time semgrep -f https://semgrep.live/p/r2c-security-audit .
58.62s user 6.47s system 118% cpu 54.786 total

The list of those rule files is here:

javascript/express/security/express-jwt-hardcoded-secret.yaml
contrib/nodejsscan/nosql_injection.yaml
javascript/express/security/audit/res-render-injection.yaml
contrib/nodejsscan/archive_path_overwrite.yaml
python/lang/maintainability/useless-innerfunction.yaml
javascript/passport-jwt/security/passport-hardcode.yaml
javascript/express/security/require-request.yaml
contrib/nodejsscan/hardcoded_secrets.yaml
contrib/nodejsscan/xss_node.yaml
javascript/express/security/audit/express-xml2json-xxe-event.yaml
contrib/nodejsscan/header_cors_star.yaml
javascript/express/security/express-sandbox-injection.yaml
javascript/express/security/express-xml2json-xxe.yaml
javascript/jsonwebtoken/security/audit/jwt-exposed-data.yaml
python/django/security/injection/email/xss-send-mail-html-message.yaml
javascript/jsonwebtoken/security/audit/jwt-decode-without-verify.yaml
javascript/jose/security/audit/jose-exposed-data.yaml
javascript/vm2/security/audit/vm2-context-injection.yaml
python/django/best-practice/json_response.yaml
python/django/security/injection/command/command-injection-os-system.yaml
javascript/express/security/express-puppeteer-injection.yaml
javascript/express/security/audit/express-jwt-not-revoked.yaml
javascript/express/security/express-phantom-injection.yaml
javascript/express/security/express-expat-xxe.yaml
python/django/security/injection/sql/sql-injection-using-raw.yaml
python/django/security/injection/sql/sql-injection-rawsql.yaml
python/django/security/injection/sql/sql-injection-using-db-cursor-execute.yaml
contrib/dlint/dlint-equivalent.yaml
javascript/express/security/express-wkhtml-injection.yaml
javascript/express/security/express-vm2-injection.yaml
python/django/security/injection/sql/sql-injection-extra.yaml
javascript/lang/security/audit/vm-injection.yaml
javascript/express/security/express-vm-injection.yaml
javascript/express/security/audit/express-path-join-resolve-traversal.yaml
javascript/lang/security/audit/path-traversal/path-join-resolve-traversal.yaml
javascript/express/security/audit/express-cookie-settings.yaml
javascript/wkhtmltoimage/security/audit/wkhtmltoimage-injection.yaml
javascript/phantom/security/audit/phantom-injection.yaml
javascript/wkhtmltopdf/security/audit/wkhtmltopdf-injection.yaml
javascript/sandbox/security/audit/sandbox-code-injection.yaml
javascript/vm2/security/audit/vm2-code-injection.yaml
python/django/security/injection/path-traversal/path-traversal-join.yaml
python/django/security/injection/path-traversal/path-traversal-file-name.yaml
javascript/puppeteer/security/audit/puppeteer-injection.yaml

We made https://github.com/returntocorp/semgrep-rules/issues/736 for improving the performance of the rules, and are looking into other performance regressions possibilities too:

• https://github.com/returntocorp/semgrep/issues/1130
• https://github.com/returntocorp/semgrep/issues/1125

For the time being, you may have some success turning off any of our r2c rules that are in the above list while we sort things out. Sorry about the regression 😞

[aryx]

Yes @ajinabraham we definitely need to add a performance regression testing in our CI checking for regression in nodejjscan.
What is the command we need to run to check for those perf regressions?

[ajinabraham]

@aryx Not sure if I understand correctly, But there is nothing specific to nodejsscan. I would like to see these semgrep rules https://github.com/semgrep/rules-nodejsscan running against some vulnerable node.js apps like the OWASP juice shop for performance-related CI checks.

[brendongo]

Update on progress being made re: performance regressions

We have identified two potential sources of poor performance:

• https://github.com/returntocorp/semgrep/issues/1360
• https://github.com/returntocorp/semgrep/issues/1361

[aryx]

I think a big issue is that patterns like $X = {}; ...
matches a lot of elements in three.js because {} may match any records with any elements, and then it triggers too much the deep statement matching.
@ajinabraham do you expect $X = {}; to match code like foo.bar = { fld1: 1, fld2: 2};?

[aryx]

or do you want $X = {}; to just match empty objects?

[aryx]

@ajinabraham ?

[ajinabraham]

Apologies for replying late. I was not doing well.
@aryx Can you share an example? I can look into it and get back

[aryx]

No problem, I am actually not feeling well either today :) Got a nasty cold.
the main performance issue is that semgrep with your rules get stuck on three.js because one of the
pattern, $X = {} ... matches at too many places which then triggers the slow deep statement matching,
and on a big file like three.js it's really slow.
My question is that in the example below
https://semgrep.live/LjK7/
do your want $X = {} to match the code?

[aryx]

The problematic rule is node_nosqli_injection with for example this pattern:
- pattern: |
$QUERY = {};
...
$QUERY[$KEY] = <... $REQ.$FOO.$BAR ...>;
...
$OBJ.findOne($QUERY, ...);

The first $QUERY = {} matches too many places in three.js. Do you want $QUERY = {} to match all those places or when you wrote this pattern you just wanted $QUERY = {} to only match empty objects?

[ajinabraham]

This ($QUERY = {};) is not really required I think. I removed all such occurrences from all the rules and attempted to scan three.js on semgrep 0.16.0. and still, see the issues.
The updated rules I am using: https://github.com/ajinabraham/njsscan/tree/master/njsscan/rules/semantic_grep

[ajinabraham]

I am already testing --timeout in 0.17.0 and it looks great!

[ajinabraham]

@aryx I see the following with timeout enabled.

Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.ssrf.phantom_ssrf on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.vm_runincontext_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.xml.node_xpath_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.headers.cookie_session_no_domain on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.vm_code_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.ssrf.wkhtmltopdf_ssrf on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.xml.node_xxe on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.jwt.jwt_exposed_data on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.ssrf.puppeteer_ssrf on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.headers.cookie_session_no_path on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.database.node_sqli_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.server_side_template_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.vm2_code_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.redirect.express_open_redirect on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.dos.regex_dos on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.ssrf.wkhtmltoimage_ssrf on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.headers.cookie_session_no_expires on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.xss.xss_serialize_javascript on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.eval_require on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.traversal.join_resolve_path_traversal on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.headers.cookie_session_no_samesite on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.xml.xxe_expat on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.redirect.express_open_redirect2 on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.exec.generic_os_command_exec2 on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.vm_runinnewcontext_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.vm2_context_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.ssrf.node_ssrf on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.headers.cookie_session_no_secure on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.vm_compilefunction_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.xml.xxe_xml2json on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.exec.generic_os_command_exec on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.xss.express_xss on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.dos.regex_injection_dos on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.headers.cookie_session_default on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.database.node_nosqli_js_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.sandbox_code_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.traversal.generic_path_traversal on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.eval.eval_nodejs on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.headers.cookie_session_no_httponly on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.exec.shelljs_os_command_exec on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.
Warning: Semgrep exceeded timeout when running .njsscan.njsscan.rules.semantic_grep.database.node_nosqli_injection on /juice-shop-master/frontend/src/assets/private/three.js. See `--timeout` for more info.

Does it make sense to skip the problematic file(s) on maybe a 3rd timeout and show it in errors or a separate flag along with timeout to handle that? This will save time wasted on timeouts on that file per X no of rules.

[aryx]

Not sure exactly what you propose. Do you want that the first timeout on a file with a rule to automatically skip this file for the other rules?

[ajinabraham]

I mean if we find that a file is consistently timing out for more than ~3 rules. Remove that file from being scanned with other rules, if a flag like skip_on_timeout=3 is supplied along with timeout=6.

This should skip problematic files triggering timeouts with most other rules and increasing scan times. In this example, we could skip the file three.js on the third timeout.

[aryx]

@brendongo what do you think? sounds reasonable to me. Could be done on the python side only? just exclude the file from the target list file passed to semgrep-core?

[aryx]

@brendongo we could also aggregate the 50 errors with 1 error listing all the rules (or just the first few rules and a ...)

[aryx]

BTW was this ever fast with semgrep 0.9? I have a hard time to believe that semgrep 0.9 was working correctly and fast on the current juice-shop repo with the current njssscan rules.

[aryx]

some of the patterns are intrinsically exponential given how the matching engine currently works (we can fix that, but it's a big change on how the matching engine work).

[ajinabraham]

The issue was reported on 0.8.0 with around 70 rules, fixed in 0.9.0 and got reintroduced from 0.10.0

[brendongo]

@ajinabraham skip_on_timeout sounds like a great idea
@aryx i'll make a ticket on aggregating errors like this

[brendongo]

Created https://github.com/returntocorp/semgrep/issues/1430 and https://github.com/returntocorp/semgrep/issues/1431 for aggregating timeout error message and for skipping a file after X timeouts.

[aryx]

Closing this, I think we've did many things to improve the performance.
We can create another issue if needed to check the performance of 0.20 and more recent semgrep versions.