Skip to content

Fix false negatives in Python MCP and LLM exec rules - #4128

Open
Ayush-M99 wants to merge 1 commit into
semgrep:developfrom
Ayush-M99:fix-mcp-llm-exec-false-negatives
Open

Ayush-M99 wants to merge 1 commit into
semgrep:developfrom
Ayush-M99:fix-mcp-llm-exec-false-negatives

Conversation

@Ayush-M99

Copy link
Copy Markdown

Fixes #4127.

Changes

  • mcp-command-injection-python, mcp-ssrf-python, mcp-unsanitized-return-python, mcp-credential-in-response-python: match @$SERVER.tool(...), so tools registered with arguments are covered. mcp-typosquatted-tool-name-python already handles this form.
  • mcp-command-injection-python: add os.popen, subprocess.check_call, subprocess.check_output, subprocess.getoutput, subprocess.getstatusoutput and asyncio.create_subprocess_shell.
  • llm-output-to-exec-python: allow keywords after shell=True, and add subprocess.check_call, subprocess.check_output and os.popen.
  • Add # ruleid: and # ok: test cases for each change.

Testing

On Semgrep 1.181.0 in the playground, with message and metadata omitted:

  • mcp-command-injection-python: 16/16 assertions passed on the extended test file.
  • llm-output-to-exec-python: 13/13 assertions passed on the extended test file.
  • The other three rules flagged both the bare and the argument decorator cases. Reduced versions of mcp-ssrf-python and mcp-unsanitized-return-python were used.

semgrep test has not been run on a checkout. The new cases in the other three rules' test files have not been run as written.

This touches the same files as #4094, #4102 and #4112, though not the same lines in the rule files. Test file additions may need a small rebase.

Prepared with AI assistance (Claude), which wrote the patch and ran the tests above.

Match MCP tools registered with decorator arguments, allow keywords after
shell=True, and add missing shell sinks. Fixes semgrep#4127.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.


@mcp.tool(description="Fetch a page")
def fetch_with_description() -> str:
response = requests.get("https://example.com")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High severity and reachable blocking issue identified in your code:
Line 33 has a potentially vulnerable usage of urllib3, possibly introducing a high severity vulnerability.

ℹ️ Why this is reachable

A reachable issue is a possible security risk because your project actually executes the vulnerable code. This issue is reachable because your code calls vulnerable code in this version of urllib3.
Affected versions of urllib3 are vulnerable to Allocation of Resources Without Limits or Throttling. urllib3's streaming chunked-transfer-encoding read path (HTTPResponse.read_chunked(), reached via HTTPResponse.stream() and response iteration) reads each chunk-size line without a length limit. A malicious server can send 'Transfer-Encoding: chunked' followed by a very long chunk-size line with no newline, forcing the client to buffer unbounded amounts of memory and causing a denial of service.

References: https://euvd.enisa.europa.eu/vulnerability/EUVD-2026-89017, GHSA, CVE

To resolve this comment:
Upgrade this dependency to at least version 2.8.0 at Pipfile.lock.

💬 Ignore this finding

To ignore this, reply with:

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

You can view more details on this finding in the Semgrep AppSec Platform here.

@mcp.tool(description="Fetch a URL")
def fetch_with_description(url: str) -> str:
# ruleid: mcp-ssrf-python
response = requests.get(url)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High severity and reachable blocking issue identified in your code:
Line 42 has a potentially vulnerable usage of urllib3, possibly introducing a high severity vulnerability.

ℹ️ Why this is reachable

A reachable issue is a possible security risk because your project actually executes the vulnerable code. This issue is reachable because your code calls vulnerable code in this version of urllib3.
Affected versions of urllib3 are vulnerable to Allocation of Resources Without Limits or Throttling. urllib3's streaming chunked-transfer-encoding read path (HTTPResponse.read_chunked(), reached via HTTPResponse.stream() and response iteration) reads each chunk-size line without a length limit. A malicious server can send 'Transfer-Encoding: chunked' followed by a very long chunk-size line with no newline, forcing the client to buffer unbounded amounts of memory and causing a denial of service.

References: https://euvd.enisa.europa.eu/vulnerability/EUVD-2026-89017, GHSA, CVE

To resolve this comment:
Upgrade this dependency to at least version 2.8.0 at Pipfile.lock.

💬 Ignore this finding

To ignore this, reply with:

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

You can view more details on this finding in the Semgrep AppSec Platform here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python MCP and LLM exec rules miss decorated tools, keywords after shell=True, and common shell sinks

2 participants