fix(plugins): honor PATHEXT for provider lookup on Windows - #14159
Conversation
The PATH fallback of provider discovery appended a hardcoded ".exe" on Windows, restricting providers to compiled binaries. exec.LookPath already implements Windows executable resolution (PATHEXT), which finds .exe but also .com/.bat/.cmd scripts — the suffix only narrowed the lookup. Drop the executable() indirection and look up the bare name. Note this only affects the PATH fallback: the docker CLI plugin channel (docker-<name>) keeps requiring .exe on Windows, per docker/cli plugin candidate rules. Fixes docker#14157 Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The PR correctly drops the executable() helper and lets exec.LookPath use Windows PATHEXT semantics. Unix behavior is unchanged. One medium-severity security finding: PATHEXT resolution now allows .bat/.cmd scripts to match before .exe binaries when an earlier PATH directory contains a same-named script — the old .exe-only lookup blocked this. The Go runtime's BatBadBut mitigation covers argument-injection escaping, not script discovery.
| // (PATHEXT: .exe, but also .com/.bat/.cmd), so provider executables | ||
| // need not be compiled binaries. Callers must not append a hardcoded | ||
| // ".exe": it restricts the lookup instead of helping it. | ||
| path, err = exec.LookPath(provider) |
There was a problem hiding this comment.
[medium/security] PATHEXT-resolved .bat/.cmd scripts can be executed on Windows instead of binaries
The change from exec.LookPath(executable(provider)) (which appended .exe on Windows) to exec.LookPath(provider) means Windows PATHEXT resolution now applies. On Windows, exec.LookPath searches PATH directories against the full PATHEXT list (.COM;.EXE;.BAT;.CMD). If an attacker — or an inadvertent script — places a provider.bat or provider.cmd in a PATH directory that appears before the directory containing provider.exe, the batch file will be executed instead of the intended binary.
The PR description notes that the Go runtime mitigates BatBadBut/CVE-2024-3566 (argument-injection in .bat/.cmd files). That mitigation covers argument escaping, not discovery: the batch file is still found and launched; the Go runtime simply refuses the call if it cannot safely escape the arguments. In practice, getPluginBinaryPath is called with no arguments to the provider itself at LookPath time, so the escape guard may not trigger at all — leaving the wrong binary executing silently.
The old .exe-only lookup fully prevented .bat/.cmd execution. If the intent is to support .bat/.cmd providers intentionally, consider documenting the accepted risk and the PATHEXT ordering that a well-configured system would need. If only compiled binaries should run, filter by extension after LookPath:
if ext := strings.ToLower(filepath.Ext(path)); ext == ".bat" || ext == ".cmd" {
return "", fmt.Errorf("provider %q resolved to a script (%s); only .exe providers are supported", provider, ext)
}| Confidence | Score |
|---|---|
| 🟢 strong | 97/100 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The PATH fallback of provider discovery appended a hardcoded
.exeon Windows, restricting providers to compiled binaries.exec.LookPathalready implements Windows executable resolution (PATHEXT), which finds.exebut also.com/.bat/.cmdscripts — the hardcoded suffix only narrowed the lookup. This drops theexecutable()indirection and looks up the bare provider name.Only the PATH fallback changes: the docker CLI plugin channel (
docker-<name>) keeps requiring.exeon Windows, per docker/cli plugin candidate rules. Note that scripts relying on file associations (e.g..py) still won't launch:CreateProcessdoes not honor them — PATHEXT support effectively covers what Windows can execute directly.Argument-injection concerns for batch files (BatBadBut, CVE-2024-3566) are mitigated by the Go runtime, which refuses to run
.bat/.cmdwith arguments it cannot safely escape.Fixes #14157
🤖 Generated with Claude Code