registry icon indicating copy to clipboard operation
registry copied to clipboard

Provide clear guidance on avoiding security problems with `template` type package arguments

Open tadasant opened this issue 8 months ago • 2 comments

@alexhancock brought this up https://github.com/modelcontextprotocol/registry/pull/33#issuecomment-2876633662:

what validations would we imagine for template strings? Both for validity but also command injection. For example, what would we do if someone submitted a server with:

"template": “;rm -rf ~/Development”

@connor4312 said:

In general we have to assume that clients do the right shell escaping for arguments (or don't run the in shell, e.g. child_process.spawn). Users can do very valid things like put in quotation/punctuation marks and spaces into arguments, and if the client runs them in a shell it takes on the responsibility of escaping those.

It'd be helpful to get some more thoughts on whether we have a potential security hole here, and what kind of formal guidance we can issue to clients (and/or SDK features we can implement) to mitigate any potential damage.

tadasant avatar May 13 '25 18:05 tadasant

I think there are (at least) two questions to consider

  1. Should the registry do any form of scanning/sanitizing of the input if we do choose to enable the template field?
  2. What should we document about the risks client developers face when constructing commands from the template value and through what documentation channel?

@connor4312 per the original exchange in #33, I agree about shell escaping being a client responsibility before running, but I worry more about malicious commands being included, like the example:

"template": “;rm -rf ~/Development”

Are you thinking about this in a different way? Is there something I am missing in the design of the feature that makes this not a risk?

alexhancock avatar Jun 05 '25 14:06 alexhancock

Should the registry do any form of scanning/sanitizing of the input if we do choose to enable the template field?

This is nice but not possible to do with complete accuracy -- this is the halting problem. And folks could always just have a command that gets a shell script from the web and install that way, which is a popular approach for some packages in general, though we could have some LM analysis step as part of submission to provide a rough safeguard (provided the false positive rate is acceptable.)

What should we document about the risks client developers face when constructing commands from the template value and through what documentation channel?

I'm not sure where such instructions should exist, but I think we can recommend that "Clients MUST display the command(s) use to execute an MCP server to users and obtain their consent before executing commands from new servers or new server versions."

Is there something I am missing in the design of the feature that makes this not a risk?

Notably, MCP server commands don't have to be executed in a shell. And I think spawning in a shell is actually pretty rare because clients need direct stdin/stdout access. If clients used posix_spawn or equivalents then there is no risk of injection.

connor4312 avatar Jun 05 '25 17:06 connor4312

To check my understanding, this is basically wanting to warn registry clients that running arbitrary commands can be dangerous, and runtime_arguments can be a vector for injecting harmful commands if not careful. So in general they should take precautions like escaping things, spawning things in a shell, and showing commands that might be run to the user.

If that's the case, is this good to move to implementation work for someone to write docs on?

@claude can you tell me where my understanding is wrong? Make sure to read this thread, plus PR #33, for context.

domdomegg avatar Aug 22 '25 03:08 domdomegg

Claude finished @domdomegg's task —— View job


Your understanding is mostly correct with some important nuances.

Todo List

  • [x] Read PR #33 for additional context on template arguments
  • [x] Analyze the security concerns and proposed solutions
  • [x] Clarify the user's understanding
  • [x] Provide recommendations on next steps

Summary

Your understanding is mostly correct but has one key inaccuracy about shell execution.

Where your understanding needs clarification

Correction: You mentioned "escaping things, spawning things in a shell" - but connor4312 actually emphasized that MCP servers don't have to be executed in a shell at all. The security risk is primarily when clients DO use shell execution.

Key points from the discussion:

  1. The Risk: Template arguments (like runtime_arguments with type: template) could contain malicious commands like ;rm -rf ~/Development

  2. Why it's actually lower risk than it appears:

    • MCP servers typically need direct stdin/stdout access
    • Shell execution is "actually pretty rare" for MCP clients
    • Using posix_spawn or equivalents eliminates injection risk entirely
  3. Connor's proposed mitigation strategy:

    • Clients MUST display the command(s) to users
    • Clients MUST obtain consent before executing commands from new servers/versions
    • Use non-shell execution when possible
  4. The template feature: From PR #33, this allows complex argument templating like:

    - type: template
      template: "--mount=type=bind,src={host_path},dst={container_path}"
      properties:
        host_path: { ... }
        container_path: { ... }
    

Ready for implementation: Yes, this appears ready for documentation work. The consensus is:

  • Registry-side validation isn't practical (halting problem)
  • Client-side responsibility with clear security guidance
  • Focus on user consent and non-shell execution patterns

Recommended next steps: Create documentation that warns clients about:

  1. Command injection risks with shell execution
  2. Preference for non-shell execution methods
  3. Mandatory user consent for command execution
  4. Proper argument escaping when shell execution is necessary

claude[bot] avatar Aug 22 '25 03:08 claude[bot]

I'm a little confused because now I can't actually find where we accept "template" style arguments while working on #89

Going to return to this later.

domdomegg avatar Sep 05 '25 18:09 domdomegg

Here:

https://github.com/modelcontextprotocol/registry/blob/e4002fd19d42b324f4a19db0f1b84d19296650e1/docs/server-registry-api/openapi.yaml#L366-L375

connor4312 avatar Sep 05 '25 19:09 connor4312