diff --git a/scripts/install.sh b/scripts/install.sh index 47b624af..8e14faa3 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -251,7 +251,12 @@ else To bypass (not recommended): MALT_ALLOW_UNVERIFIED=1 curl … | bash" fi - EXPECTED=$(grep "$ARCHIVE_NAME" "$TMPDIR/checksums.txt" | awk '{print $1}') + # Fixed-string, whole-field match on the two-space GoReleaser separator. + # An unanchored `grep "$ARCHIVE_NAME"` treats the name as a regex and matches + # anywhere on the line, so a longer filename containing this one would also + # hit and make EXPECTED multi-line. checksums.txt is cosign-verified above, + # so this was not exploitable — it just should not depend on that ordering. + EXPECTED=$(awk -v want=" ${ARCHIVE_NAME}" 'index($0, want) == 65 {print $1; exit}' "$TMPDIR/checksums.txt") [ -n "$EXPECTED" ] || error "Checksum for ${ARCHIVE_NAME} not listed in checksums.txt." ACTUAL=$(shasum -a 256 "$TMPDIR/$ARCHIVE_NAME" | awk '{print $1}') if [ "$EXPECTED" != "$ACTUAL" ]; then diff --git a/src/cli/version_update.zig b/src/cli/version_update.zig index 848f5bc5..376a7a72 100644 --- a/src/cli/version_update.zig +++ b/src/cli/version_update.zig @@ -16,6 +16,7 @@ const cleanup = @import("../update/cleanup.zig"); const origin = @import("../update/origin.zig"); const release = @import("../update/release.zig"); const verify = @import("../update/verify.zig"); +const atomic = @import("../fs/atomic.zig"); const swap = @import("../update/swap.zig"); const notifier = @import("../update/notifier.zig"); const AppCtx = @import("../app_ctx.zig").AppCtx; @@ -492,6 +493,10 @@ fn runVerification(rv: RunVerification) !void { output.info("Verifying cosign signature + SHA256 checksum...", .{}); verify.verifyAll(rv.ctx.io, rv.allocator, .{ + // The prefix is user-writable and on PATH, so a `cosign` resolving + // inside it is refused rather than trusted for its exit status. + .environ = rv.ctx.environ, + .prefix = atomic.maltPrefixOrAbort(), .tarball_path = rv.tarball_path, .checksums_path = rv.checksums_path, .sigstore_path = sigstore_path, @@ -509,6 +514,13 @@ fn runVerification(rv: RunVerification) !void { output.err("cosign signature verification failed for {s}", .{checksums_name}); return error.Aborted; }, + error.CosignUntrusted => { + output.err("Refusing to verify with a cosign found inside the malt prefix.", .{}); + output.info("`{s}` is writable by malt and by the packages it installs, so a", .{atomic.maltPrefixOrAbort()}); + output.info("cosign resolved from there could report success for any binary.", .{}); + output.info("Install cosign system-wide (e.g. `brew install cosign`) and retry.", .{}); + return error.Aborted; + }, error.ChecksumMissing => { output.err("Checksum for {s} not listed in {s}", .{ rv.archive_name, checksums_name }); return error.Aborted; diff --git a/src/net/client.zig b/src/net/client.zig index 6666a9f8..36f841af 100644 --- a/src/net/client.zig +++ b/src/net/client.zig @@ -28,6 +28,8 @@ pub const GetError = error{ OfflineRequired, RequestFailed, TlsDowngradeRefused, + /// A manifest handed us a cleartext (non-loopback) origin. + InsecureUrlScheme, TooManyHttpRedirects, HttpRedirectInvalid, ResponseTooLarge, @@ -442,11 +444,39 @@ pub const HttpClient = struct { return hostIsSameOrSubdomain(fh, th); } + /// True for the loopback names a local fixture server binds. Cleartext to + /// one of these never leaves the machine, so the https requirement below + /// does not apply — and the test suites that stand up a throwaway HTTP + /// server keep working without an escape hatch a real deployment could + /// trip over. + fn isLoopbackHost(host: []const u8) bool { + return std.ascii.eqlIgnoreCase(host, "127.0.0.1") or + std.ascii.eqlIgnoreCase(host, "localhost") or + std.ascii.eqlIgnoreCase(host, "::1") or + std.ascii.eqlIgnoreCase(host, "[::1]"); + } + + /// Refuse a cleartext origin. The redirect loop already declines an + /// https→http downgrade mid-chain, but the *initial* scheme came straight + /// from a manifest: a tap that writes `http://` for a formula or cask URL + /// got a plaintext fetch, and a cask may legitimately carry no `sha256` + /// digest, leaving nothing but the transport to substitute against. + /// + /// `cli/tap.zig` already refuses `http://` when registering a tap; this + /// applies the same rule to the URLs those taps hand back. + pub fn requireSecureOrigin(url: []const u8) !void { + if (std.mem.startsWith(u8, url, "https://")) return; + const host = urlHost(url) orelse return error.InsecureUrlScheme; + if (isLoopbackHost(host)) return; + return error.InsecureUrlScheme; + } + /// GET request; auto-injects the GitHub API token (`MALT_GITHUB_TOKEN`, /// then `HOMEBREW_GITHUB_API_TOKEN`) as Authorization for GitHub/Homebrew /// hosts. Caller owns the returned `Response`. pub fn get(self: *HttpClient, url: []const u8) !Response { if (self.offline) return error.OfflineRequired; + try requireSecureOrigin(url); if (try self.authHeaderValue(url)) |auth_value| { defer self.allocator.free(auth_value); const headers = [_]std.http.Header{ @@ -476,6 +506,7 @@ pub const HttpClient = struct { progress: ?ProgressCallback, ) !Response { if (self.offline) return error.OfflineRequired; + try requireSecureOrigin(url); return self.doGetWithRetry(url, extra_headers, max_blob_bytes, progress); } @@ -493,12 +524,14 @@ pub const HttpClient = struct { progress: ?ProgressCallback, ) GetError!u16 { if (self.offline) return error.OfflineRequired; + requireSecureOrigin(url) catch return error.InsecureUrlScheme; return self.doGetToWriterWithRetry(url, extra_headers, sink, progress); } /// Perform a HEAD request and return only the HTTP status code. pub fn head(self: *HttpClient, url: []const u8) !u16 { if (self.offline) return error.OfflineRequired; + try requireSecureOrigin(url); const uri = try std.Uri.parse(url); var req = try self.client.request(.HEAD, uri, .{ @@ -1397,6 +1430,40 @@ fn environFrom(entries: [:null]const ?[*:0]const u8) std.process.Environ { return .{ .block = .{ .slice = entries } }; } +test "requireSecureOrigin: refuses a cleartext manifest origin" { + // A tap controls the `url` field of a formula or cask. Cask hashes are + // optional upstream, so on the cleartext path there can be nothing but the + // transport standing between the manifest and an installed artifact. + const bad = [_][]const u8{ + "http://example.com/pkg.tar.gz", + "http://github.com/x/y", + "http://192.168.1.10/pkg.dmg", + "http://evil.tld/127.0.0.1/pkg.zip", // loopback in the *path*, not the host + "http://127.0.0.1.evil.tld/pkg.zip", // and not as a prefix of the host + "ftp://example.com/pkg.tar.gz", + "file:///etc/passwd", + "not-a-url", + "", + }; + for (bad) |u| try std.testing.expectError( + error.InsecureUrlScheme, + HttpClient.requireSecureOrigin(u), + ); +} + +test "requireSecureOrigin: allows https anywhere and cleartext only to loopback" { + // The loopback carve-out is what keeps the fixture-server tests honest + // without giving a real deployment a way to opt out. + const ok = [_][]const u8{ + "https://formulae.brew.sh/api/formula/jq.json", + "https://ghcr.io/v2/x/blobs/sha256:ab", + "http://127.0.0.1:8080/blob", + "http://localhost:1234/start", + "http://LOCALHOST:1/x", + }; + for (ok) |u| try HttpClient.requireSecureOrigin(u); +} + test "githubApiToken: MALT_GITHUB_TOKEN is preferred over HOMEBREW_GITHUB_API_TOKEN" { const entries = [_:null]?[*:0]const u8{ "MALT_GITHUB_TOKEN=malt-tok".ptr, diff --git a/src/net/ghcr.zig b/src/net/ghcr.zig index 43e7e48b..0259e98f 100644 --- a/src/net/ghcr.zig +++ b/src/net/ghcr.zig @@ -275,6 +275,10 @@ pub const GhcrClient = struct { error.OfflineRequired, error.RequestFailed, error.TlsDowngradeRefused, + // A cleartext origin is a manifest defect, not a transient + // fault; it collapses here with the rest because the outer + // loop's retry is harmless (it will fail identically). + error.InsecureUrlScheme, error.TooManyHttpRedirects, error.HttpRedirectInvalid, error.ResponseTooLarge, diff --git a/src/update/verify.zig b/src/update/verify.zig index 4e5a50e5..8f8ef9ea 100644 --- a/src/update/verify.zig +++ b/src/update/verify.zig @@ -58,11 +58,18 @@ pub const CosignError = error{ CosignNotFound, /// `cosign verify-blob` exited non-zero - signature did not verify. CosignVerifyFailed, + /// The `cosign` that would run resolves inside malt's own prefix, which + /// packages can write to. Refuse rather than trust its exit status. + CosignUntrusted, }; pub const CosignBlob = struct { /// Either `"cosign"` to resolve via PATH, or an absolute path (tests). cosign_bin: []const u8 = "cosign", + /// Process environment, for the PATH lookup that mirrors execvp. + environ: std.process.Environ = .empty, + /// malt's install prefix. A `cosign` resolving inside it is refused. + prefix: []const u8 = "", /// The blob whose signature is being checked (usually checksums.txt). blob_path: []const u8, /// Sigstore `.sigstore.json` bundle (cert + signature + rekor entry). @@ -76,6 +83,7 @@ pub const CosignBlob = struct { pub const VerifyError = error{ CosignNotFound, CosignVerifyFailed, + CosignUntrusted, /// `archive_name` has no matching line in `checksums.txt`. ChecksumMissing, ChecksumMismatch, @@ -88,6 +96,10 @@ pub const VerifyError = error{ pub const VerifyInputs = struct { /// `"cosign"` for PATH lookup, or an absolute path (tests). cosign_bin: []const u8 = "cosign", + /// Process environment, for the PATH lookup that mirrors execvp. + environ: std.process.Environ = .empty, + /// malt's install prefix. A `cosign` resolving inside it is refused. + prefix: []const u8 = "", tarball_path: []const u8, checksums_path: []const u8, sigstore_path: []const u8, @@ -104,6 +116,8 @@ pub const VerifyInputs = struct { pub fn verifyAll(io: std.Io, allocator: std.mem.Allocator, in: VerifyInputs) VerifyError!void { verifyCosignBlob(io, .{ .cosign_bin = in.cosign_bin, + .environ = in.environ, + .prefix = in.prefix, .blob_path = in.checksums_path, .bundle_path = in.sigstore_path, .cert_identity_regex = in.cert_identity_regex, @@ -126,9 +140,68 @@ pub fn verifyAll(io: std.Io, allocator: std.mem.Allocator, in: VerifyInputs) Ver if (!std.crypto.timing_safe.eql([32]u8, expected, actual)) return error.ChecksumMismatch; } +/// True when `path` sits inside malt's own install prefix. +/// +/// `cosign` is normally resolved off `PATH`, which includes `/bin` — +/// a directory malt itself installs packages into, and which any of the +/// package-controlled write paths can reach. A `cosign` shim landing there +/// would turn every later `mt version update` into a rubber stamp, since the +/// only signal this module reads is the child's exit status. Refuse to treat +/// a verifier that malt (or a package) could have written as trusted. +pub fn resolvedInsidePrefix(resolved: []const u8, prefix: []const u8) bool { + if (prefix.len == 0) return false; + if (!std.mem.startsWith(u8, resolved, prefix)) return false; + if (resolved.len == prefix.len) return true; + if (prefix[prefix.len - 1] == '/') return true; + return resolved[prefix.len] == '/'; +} + +/// Locate `bin` on `PATH` and reject it when it resolves inside `prefix`. +/// A bare name with no match is left to the spawn to fail as CosignNotFound. +fn rejectPrefixResidentTool( + io: std.Io, + bin: []const u8, + prefix: []const u8, + environ: std.process.Environ, +) CosignError!void { + if (prefix.len == 0) return; + var resolved_buf: [std.fs.max_path_bytes]u8 = undefined; + + // Resolve the prefix too, or the comparison is spelling-sensitive: on + // macOS `/tmp` is a symlink to `/private/tmp`, so a resolved binary path + // would never match a prefix given in the other form. Falls back to the + // literal when the prefix does not exist yet. + var prefix_buf: [std.fs.max_path_bytes]u8 = undefined; + const prefix_real = if (std.Io.Dir.cwd().realPathFile(io, prefix, &prefix_buf)) |pn| + prefix_buf[0..pn] + else |_| + prefix; + + // An explicit path is checked as given; a bare name is searched on PATH. + if (std.mem.indexOfScalar(u8, bin, '/') != null) { + const n = std.Io.Dir.cwd().realPathFile(io, bin, &resolved_buf) catch return; + if (resolvedInsidePrefix(resolved_buf[0..n], prefix_real)) return error.CosignUntrusted; + return; + } + + const path_env = std.process.Environ.getPosix(environ, "PATH") orelse return; + var it = std.mem.tokenizeScalar(u8, path_env, ':'); + var probe: [std.fs.max_path_bytes]u8 = undefined; + while (it.next()) |dir| { + const cand = std.fmt.bufPrint(&probe, "{s}/{s}", .{ dir, bin }) catch continue; + std.Io.Dir.cwd().access(io, cand, .{}) catch continue; + // First hit wins, exactly as execvp would resolve it. + const n = std.Io.Dir.cwd().realPathFile(io, cand, &resolved_buf) catch return; + if (resolvedInsidePrefix(resolved_buf[0..n], prefix_real)) return error.CosignUntrusted; + return; + } +} + /// Shell out to `cosign verify-blob` with the same flags `install.sh` uses. /// Exit 0 = verified. Any other outcome maps to a CosignError. pub fn verifyCosignBlob(io: std.Io, args: CosignBlob) CosignError!void { + try rejectPrefixResidentTool(io, args.cosign_bin, args.prefix, args.environ); + const argv = [_][]const u8{ args.cosign_bin, "verify-blob", @@ -152,3 +225,66 @@ pub fn verifyCosignBlob(io: std.Io, args: CosignBlob) CosignError!void { else => return error.CosignVerifyFailed, } } + +test "resolvedInsidePrefix: only a component-boundary match counts" { + try std.testing.expect(resolvedInsidePrefix("/opt/malt/bin/cosign", "/opt/malt")); + try std.testing.expect(resolvedInsidePrefix("/opt/malt", "/opt/malt")); + try std.testing.expect(resolvedInsidePrefix("/opt/malt/", "/opt/malt/")); + // A sibling that merely shares a textual prefix is not inside it. + try std.testing.expect(!resolvedInsidePrefix("/opt/malthack/bin/cosign", "/opt/malt")); + try std.testing.expect(!resolvedInsidePrefix("/usr/local/bin/cosign", "/opt/malt")); + try std.testing.expect(!resolvedInsidePrefix("/opt/homebrew/bin/cosign", "/opt/malt")); + // An empty prefix disables the check rather than matching everything. + try std.testing.expect(!resolvedInsidePrefix("/anything", "")); +} + +test "verifyCosignBlob refuses a cosign that lives inside the prefix" { + const io = std.Options.debug_io; + const a = std.testing.allocator; + + // Stand up a fake prefix with an executable `cosign` in its bin/, exactly + // where a package (or one of the path-traversal bugs) could drop one. + const prefix = try std.fmt.allocPrint(a, "/tmp/malt_cosign_trust_{d}", .{std.c.getpid()}); + defer a.free(prefix); + defer std.Io.Dir.cwd().deleteTree(io, prefix) catch {}; + const bin = try std.fmt.allocPrint(a, "{s}/bin", .{prefix}); + defer a.free(bin); + try std.Io.Dir.cwd().createDirPath(io, bin); + const shim = try std.fmt.allocPrint(a, "{s}/cosign", .{bin}); + defer a.free(shim); + { + const f = try std.Io.Dir.createFileAbsolute(io, shim, .{ .truncate = true }); + defer f.close(io); + // Contents are deliberately inert: the guard refuses on *location* + // before anything is spawned, so a shim that could actually run would + // add nothing to the test but a shell-invocation pattern under src/ + // (which `tests/spawn_invariant_test.zig` rightly rejects). + try f.writeStreamingAll(io, "placeholder\n"); + try f.setPermissions(io, std.Io.File.Permissions.fromMode(0o755)); + } + + // Explicit-path form: refused because it resolves inside the prefix. + try std.testing.expectError(error.CosignUntrusted, verifyCosignBlob(io, .{ + .cosign_bin = shim, + .prefix = prefix, + .blob_path = "/dev/null", + .bundle_path = "/dev/null", + .cert_identity_regex = "^x$", + .oidc_issuer = "https://example.invalid", + })); + + // PATH form: same shim, found by search, same refusal. + const path_val = try std.fmt.allocPrintSentinel(a, "PATH={s}", .{bin}, 0); + defer a.free(path_val); + const entries = [_:null]?[*:0]const u8{path_val.ptr}; + const environ: std.process.Environ = .{ .block = .{ .slice = &entries } }; + try std.testing.expectError(error.CosignUntrusted, verifyCosignBlob(io, .{ + .cosign_bin = "cosign", + .environ = environ, + .prefix = prefix, + .blob_path = "/dev/null", + .bundle_path = "/dev/null", + .cert_identity_regex = "^x$", + .oidc_issuer = "https://example.invalid", + })); +}