Skip to content

Numeric tool parameters reach bash arithmetic evaluation without validation #3

Description

Four numeric comparisons run on values taken straight from tool arguments, with no validation of their own:

  • mcp-server-gh/lib/common.sh:203max_lines
  • mcp-server-gh/lib/common.sh:207tail_lines
  • mcp-server-gh/lib/common.sh:197-198grep_before / grep_after (the grep_context_before / grep_context_after parameters)
  • mcp-server-gh/lib/release.sh:76per_page, from release_list's limit

[[ x -gt y ]] evaluates its operands as arithmetic expressions, and bash evaluates a command substitution written inside an array subscript there. A value such as PATH[$(command)0] reaching any of these lines runs command inside the MCP server process, which holds the user's gh credentials and filesystem access.

Nothing at these sites prevents that. What prevents it today is the protocol layer: validate_tool_arguments in the vendored shared/mcpserver_core.sh enforces a property's declared type, and every parameter feeding these comparisons declares "type": "integer"max_lines (18 tools), tail_lines (7), grep_context_before and grep_context_after (5 each), limit (10, including release_list). Every tool in tools-read.json and tools-write.json carries an inputSchema, so no tool is dispatched unvalidated. A crafted call is refused before dispatch:

{"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}}

That leaves the guarantee resting entirely on 45 schema declarations that nothing downstream re-checks. A dropped or mistyped type in a future schema edit re-opens command execution silently, and any call that does not pass through the protocol layer is unguarded:

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. A payload naming a variable that is unset (x[$(...)0]) aborts under the server's set -u with unbound variable before the substitution runs, which is why the payload has to name a variable that exists.

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

Fix

Validate before comparing, at each of the four 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 comes first and the numeric comparison never runs on an unvalidated value. Each site needs a test asserting that a non-numeric value is rejected rather than evaluated.

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