Skip to content

Commit 8b116da

Browse files
fix(security): force --only-binary on python package fetch — never build/execute sdist (MCP-2391)
A plain 'pip download' / 'uv pip download' of an sdist invokes the package's PEP 517 build backend (setup.py egg_info) to resolve metadata, which executes code from the package being scanned — violating the download-and-unpack-NEVER-execute invariant of the source fetch. - Add --only-binary=:all: to uvDownloadArgs and pipDownloadArgs so only a prebuilt wheel is ever fetched; a package with no wheel fails the fetch and falls back to tool-definitions-only. - Refuse a non-wheel archive in fetchPythonPackage (defense-in-depth); drop the sdist extraction branch. - Add positive tests asserting --only-binary=:all: on both Python builders (the prior NoExecution tests only asserted absence of verbs). - Align comments + docs (security-scanner-plugins.md, swagger.yaml). Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 parent b3ab6e5 commit 8b116da

6 files changed

Lines changed: 78 additions & 46 deletions

File tree

docs/features/security-scanner-plugins.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ Package-runner servers (`npx`, `uvx`) are the **primary** quarantine/scan target
254254
**The source is fetched but never executed.** A scanner must not run the untrusted code it is scanning. The fetch only ever downloads and unpacks archives:
255255

256256
- **npm (`npx`)**`npm pack <pkg>@<version> --ignore-scripts` downloads the published tarball without running any lifecycle scripts (`install`/`postinstall`), then it is extracted (`source_method=npm_pack`).
257-
- **PyPI (`uvx`)**`uv pip download <pkg>==<version> --no-deps` (falling back to `pip download`) fetches the wheel or sdist; wheels (preferred) and sdists are unpacked **without building** or running `setup.py` (`source_method=pip_download`).
257+
- **PyPI (`uvx`)**`uv pip download <pkg>==<version> --no-deps --only-binary=:all:` (falling back to `pip download`) fetches **only a prebuilt wheel**, which is unpacked without building or running `setup.py` (`source_method=pip_download`). `--only-binary=:all:` is mandatory: downloading an sdist would invoke the package's PEP 517 build backend (`setup.py egg_info`) to resolve metadata, executing the untrusted code. A package that ships **no wheel** therefore fails the fetch and falls back to tool definitions only — sdists are never built or extracted.
258258

259259
Extraction is hardened against path traversal (zip-slip), symlink escape, and decompression bombs (bounded file count and total size). If the toolchain is missing, the host is offline, or the fetch fails, resolution falls through to **tool definitions only** with no regression.
260260

internal/config/config.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1777,11 +1777,14 @@ type SecurityConfig struct {
17771777
// the scan degrades to tool-definitions-only (no real source-level
17781778
// analysis). See MCP-2206.
17791779
//
1780-
// Fetching uses `npm pack` (npm) and `uv pip download` / `pip download`
1781-
// (Python), which only download + unpack archives and NEVER run install,
1782-
// build, or setup.py — a scanner must not execute the untrusted code it is
1783-
// scanning. Extraction is hardened against path traversal and
1784-
// decompression bombs.
1780+
// Fetching uses `npm pack --ignore-scripts` (npm) and `uv pip download` /
1781+
// `pip download` with `--only-binary=:all:` (Python), which only download +
1782+
// unpack archives and NEVER run install, build, or setup.py — a scanner must
1783+
// not execute the untrusted code it is scanning. The Python
1784+
// `--only-binary=:all:` flag is required because downloading an sdist would
1785+
// invoke its build backend (setup.py); packages with no wheel fall back to
1786+
// tool-definitions-only instead. Extraction is hardened against path
1787+
// traversal and decompression bombs.
17851788
//
17861789
// Default (nil) is ENABLED. Set to false on air-gapped deployments to
17871790
// forbid the scanner's network egress; such servers then fall back to the

internal/security/scanner/package_fetch.go

Lines changed: 39 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,14 @@ import (
2626
//
2727
// Security crux: a scanner must never execute the untrusted code it is about to
2828
// scan. We therefore only ever DOWNLOAD (npm pack / uv pip download / pip
29-
// download) and EXTRACT archives. We never install, build, or run setup.py, and
30-
// we pass --ignore-scripts / --no-deps / --only-binary to be explicit. Archive
31-
// extraction is hardened against path traversal (zip-slip), symlink escape, and
32-
// decompression bombs (bounded file count + total size).
29+
// download) and EXTRACT archives. We never install, build, or run setup.py. npm
30+
// pack uses --ignore-scripts; the Python path passes --no-deps AND
31+
// --only-binary=:all: so pip/uv only ever fetch a prebuilt wheel — downloading
32+
// an sdist would invoke its PEP 517 build backend (setup.py egg_info) to resolve
33+
// metadata, executing the package's code. A package with no wheel therefore
34+
// fails the fetch and falls back to tool-definitions-only. Archive extraction is
35+
// hardened against path traversal (zip-slip), symlink escape, and decompression
36+
// bombs (bounded file count + total size).
3337

3438
const (
3539
// fetchMaxFiles caps the number of files extracted from a fetched package.
@@ -97,21 +101,28 @@ func npmPackArgs(spec, destDir string) []string {
97101
}
98102

99103
// uvDownloadArgs builds the `uv pip download` argument list. --no-deps avoids
100-
// pulling the dependency tree (we only scan the target's own source);
101-
// extraction of the downloaded wheel/sdist runs no code.
104+
// pulling the dependency tree (we only scan the target's own source).
105+
// --only-binary=:all: forces a wheel and NEVER builds an sdist: a plain
106+
// `pip/uv download` of an sdist invokes the package's PEP 517 build backend
107+
// (setup.py egg_info) to resolve metadata, which would EXECUTE the untrusted
108+
// code we are about to scan. With this flag, a package that ships no wheel makes
109+
// the download fail, and the caller falls back to tool-definitions-only.
102110
func uvDownloadArgs(spec, destDir string) []string {
103-
return []string{"pip", "download", spec, "--no-deps", "-d", destDir}
111+
return []string{"pip", "download", spec, "--no-deps", "--only-binary=:all:", "-d", destDir}
104112
}
105113

106114
// pipDownloadArgs builds the `pip download` argument list (fallback when uv is
107-
// unavailable). --no-deps for the same reason as uv.
115+
// unavailable). --no-deps and --only-binary=:all: for the same reasons as uv —
116+
// the latter guarantees we never build (and thus never execute) an sdist.
108117
func pipDownloadArgs(spec, destDir string) []string {
109-
return []string{"download", spec, "--no-deps", "-d", destDir}
118+
return []string{"download", spec, "--no-deps", "--only-binary=:all:", "-d", destDir}
110119
}
111120

112121
// findDownloadedArchive locates the archive produced by a download command in
113-
// dir. Wheels (.whl) are preferred over sdists (.tar.gz) because extracting a
114-
// wheel never touches setup.py. npm pack writes a single .tgz.
122+
// dir. A wheel (.whl) is returned immediately. npm pack writes a single .tgz.
123+
// A bare .tar.gz (a Python sdist) is reported as a last resort so the caller can
124+
// detect and REFUSE it — the Python path only accepts wheels (--only-binary=:all:),
125+
// because building/extracting an sdist would execute the package's code.
115126
func findDownloadedArchive(dir string) (path string, kind archiveKind, err error) {
116127
entries, err := os.ReadDir(dir)
117128
if err != nil {
@@ -381,9 +392,8 @@ func (r *SourceResolver) fetchPythonPackage(ctx context.Context, info ServerInfo
381392
}
382393
cleanup := func() { os.RemoveAll(dlDir) }
383394

384-
// Prefer uv, fall back to pip. uv prefers a wheel via --only-binary when
385-
// possible; we leave format selection to the resolver and pick the wheel at
386-
// extraction time.
395+
// Prefer uv, fall back to pip. Both pass --only-binary=:all: so only a wheel
396+
// is ever downloaded — building an sdist would execute the package's code.
387397
if err := r.runPythonDownload(ctx, dlDir, spec); err != nil {
388398
cleanup()
389399
return nil, err
@@ -394,24 +404,17 @@ func (r *SourceResolver) fetchPythonPackage(ctx context.Context, info ServerInfo
394404
cleanup()
395405
return nil, fmt.Errorf("python download produced no archive: %w", err)
396406
}
397-
switch kind {
398-
case archiveZip:
399-
if err := extractZip(archive, srcDir, fetchMaxFiles, fetchMaxTotalBytes); err != nil {
400-
cleanup()
401-
return nil, fmt.Errorf("extract wheel: %w", err)
402-
}
403-
case archiveTarGz:
404-
f, oerr := os.Open(archive)
405-
if oerr != nil {
406-
cleanup()
407-
return nil, oerr
408-
}
409-
err := extractTarballGz(f, srcDir, fetchMaxFiles, fetchMaxTotalBytes)
410-
f.Close()
411-
if err != nil {
412-
cleanup()
413-
return nil, fmt.Errorf("extract sdist: %w", err)
414-
}
407+
// Wheel-only: --only-binary=:all: guarantees a wheel, but we refuse anything
408+
// else as defense-in-depth — extracting (or building) an sdist would run the
409+
// untrusted package's setup.py / PEP 517 backend. A package that ships no
410+
// wheel falls back to tool-definitions-only.
411+
if kind != archiveZip {
412+
cleanup()
413+
return nil, fmt.Errorf("python download produced a non-wheel archive; refusing to build/extract an sdist (would execute untrusted code)")
414+
}
415+
if err := extractZip(archive, srcDir, fetchMaxFiles, fetchMaxTotalBytes); err != nil {
416+
cleanup()
417+
return nil, fmt.Errorf("extract wheel: %w", err)
415418
}
416419

417420
r.logger.Info("Fetched published Python package source for scan",
@@ -422,8 +425,10 @@ func (r *SourceResolver) fetchPythonPackage(ctx context.Context, info ServerInfo
422425
return &ResolvedSource{SourceDir: srcDir, Method: "pip_download", Cleanup: cleanup}, nil
423426
}
424427

425-
// runPythonDownload tries `uv pip download` first, then `pip download`. Neither
426-
// executes the package (no build/setup.py); wheel/sdist are just downloaded.
428+
// runPythonDownload tries `uv pip download` first, then `pip download`. Both
429+
// pass --only-binary=:all:, so only a prebuilt wheel is fetched and no code runs
430+
// (a sdist download would build the package). If the package ships no wheel the
431+
// command fails and the caller falls back to tool-definitions-only.
427432
func (r *SourceResolver) runPythonDownload(ctx context.Context, dlDir, spec string) error {
428433
if uvBin, err := exec.LookPath("uv"); err == nil {
429434
cmd := exec.CommandContext(ctx, uvBin, uvDownloadArgs(spec, dlDir)...)

internal/security/scanner/package_fetch_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,27 @@ func TestPipDownloadArgs_NoExecution(t *testing.T) {
248248
}
249249
}
250250

251+
// MCP-2391: a `pip download` / `uv pip download` of an sdist runs the package's
252+
// PEP 517 build backend (setup.py egg_info) to resolve metadata — i.e. it
253+
// EXECUTES code from the package being scanned. --only-binary=:all: forces a
254+
// wheel and makes the download FAIL (no extraction) when only an sdist exists,
255+
// instead of silently building it. These positive assertions guard the flag so
256+
// it cannot be dropped without a red test (TestUv/PipDownloadArgs_NoExecution
257+
// only asserted ABSENCE of verbs, which let the original BLOCKER through).
258+
func TestUvDownloadArgs_OnlyBinary(t *testing.T) {
259+
args := uvDownloadArgs("flights==0.2.4", "/tmp/dest")
260+
if !hasExactArg(args, "--only-binary=:all:") {
261+
t.Errorf("uv download MUST pass --only-binary=:all: to never build an sdist (code execution): %v", args)
262+
}
263+
}
264+
265+
func TestPipDownloadArgs_OnlyBinary(t *testing.T) {
266+
args := pipDownloadArgs("flights==0.2.4", "/tmp/dest")
267+
if !hasExactArg(args, "--only-binary=:all:") {
268+
t.Errorf("pip download MUST pass --only-binary=:all: to never build an sdist (code execution): %v", args)
269+
}
270+
}
271+
251272
func hasExactArg(args []string, want string) bool {
252273
for _, a := range args {
253274
if a == want {

oas/docs.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

oas/swagger.yaml

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -577,11 +577,14 @@ components:
577577
the scan degrades to tool-definitions-only (no real source-level
578578
analysis). See MCP-2206.
579579
580-
Fetching uses `npm pack` (npm) and `uv pip download` / `pip download`
581-
(Python), which only download + unpack archives and NEVER run install,
582-
build, or setup.py — a scanner must not execute the untrusted code it is
583-
scanning. Extraction is hardened against path traversal and
584-
decompression bombs.
580+
Fetching uses `npm pack --ignore-scripts` (npm) and `uv pip download` /
581+
`pip download` with `--only-binary=:all:` (Python), which only download +
582+
unpack archives and NEVER run install, build, or setup.py — a scanner must
583+
not execute the untrusted code it is scanning. The Python
584+
`--only-binary=:all:` flag is required because downloading an sdist would
585+
invoke its build backend (setup.py); packages with no wheel fall back to
586+
tool-definitions-only instead. Extraction is hardened against path
587+
traversal and decompression bombs.
585588
586589
Default (nil) is ENABLED. Set to false on air-gapped deployments to
587590
forbid the scanner's network egress; such servers then fall back to the

0 commit comments

Comments
 (0)