Skip to content

Numeric tool parameters are evaluated as arithmetic with no validation at the comparison sites #3

Description

Four comparisons evaluate tool-supplied values as arithmetic expressions and validate nothing themselves:

Site Value
mcp-server-gh/lib/common.sh:295 grep_before, from grep_context_before
mcp-server-gh/lib/common.sh:296 grep_after, from grep_context_after
mcp-server-gh/lib/common.sh:301 max_lines
mcp-server-gh/lib/common.sh:305 tail_lines
mcp-server-gh/lib/release.sh:76 per_page, from release_list's limit

(The common.sh numbers move with any edit above them. The sites are the four -gt comparisons in _gh_post_process.)

[[ x -gt y ]] evaluates its operands as arithmetic expressions, and bash evaluates a command substitution written inside an array subscript in that context. A value such as PATH[$(command)0] reaching any of these lines runs command in the server process, which holds the user's gh credentials and filesystem access. A payload naming an unset variable (x[$(...)0]) aborts under the server's set -u before the substitution runs, so the payload names a variable that exists.

The other -gt comparisons in release.sh (lines 48, 63, 150) take jq-computed counts rather than tool input and are not affected.

What holds today

Two gates upstream of these sites, both in the protocol layer rather than in the code that does the comparing:

  1. Every dispatchable tool is declared. _gh_unset_undeclared_tools (lib/common.sh) drops every sourced tool_* function the running server's tools list does not name, so nothing reaches dispatch without a schema.
  2. Every declared parameter that feeds these sites is integer-typed. 45 such declarations in tools-read.json and 2 in tools-write.json; validate_tool_arguments enforces the declared type before dispatch.

A crafted call is refused before it reaches a tool function:

{"jsonrpc":"2.0","id":2,"result":{"content":[{"type":"text","text":"Invalid type(s): max_lines expected integer, got string (\"PATH[$(touch /tmp/marker)0]\")."}],"isError":true}}

There is no route through the MCP protocol that delivers a non-integer value to these comparisons.

Why this is still open

The gates are upstream, implicit, and in different files from the hazard. Nothing at the comparison sites depends on them, and nothing re-checks them.

  • A schema edit that drops or misspells one type restores command execution, silently. The 47 declarations are the whole guarantee, and no test asserts that they exist.
  • A caller that does not go through validate_tool_arguments is unguarded. _gh_post_process is an internal helper, and a new tool function calling it directly inherits no protection:
source plugins/github-mcp/mcp-server-gh/lib/common.sh
log() { :; }
_gh_post_process "data" "" "" 0 0 false false 'PATH[$(touch /tmp/marker)0]' ""

/tmp/marker exists afterwards.

This is a hardening item, not a live vulnerability. It is not reachable by any client today, and it does not gate a release.

Fix

Validate before comparing, at each of the five sites. _gh_validate_number in lib/common.sh:8 already has the right shape, or inline:

if [[ "${max_lines}" =~ ^[0-9]+$ ]] && [[ "${max_lines}" -gt 0 ]]; then

A regex match is not an arithmetic context, so the guard runs first and the comparison never sees an unvalidated value. Each site gets a test asserting that a non-numeric value is rejected rather than evaluated.

A test asserting that every parameter feeding these sites declares "type": "integer" is worth adding either way: it turns the second gate from a convention into something CI enforces, and it is useful even after the sites validate for themselves.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions