From df0683b60759df6e4fdbe0fb4e44fe68ae18d8db Mon Sep 17 00:00:00 2001 From: j3ssie Date: Sat, 8 Aug 2026 14:10:26 +0800 Subject: [PATCH] fix(api): don't run validate commands from a caller-supplied registry_url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #311 let callers point GET /osm/api/registry-info at any registry via the registry_url query param. Both modes then called IsBinaryInstalled() on every entry, which runs `sh -c ` — so a GET with a hostile registry executed arbitrary shell on the server. With SameSite=Lax session cookies and reflect-all CORS, that was reachable by CSRF from any page an operator visits. - add installer.IsBinaryInstalledNoExec() and use it whenever registry_url is set; only the embedded registry is trusted to run validate commands - match isGitHubURL() on the parsed hostname so a lookalike host such as evil.tld/?x=github.com no longer receives the GitHub token - cap remote registry reads at 32MB instead of an unbounded io.ReadAll - surface LoadRegistry errors in nix-build mode rather than returning a success response with all metadata silently missing - report the same registry_url semantics in both modes, and document the no-exec behaviour in docs/api/install.mdx --- docs/api/install.mdx | 6 +- internal/installer/binary.go | 44 +++++++++-- internal/installer/download.go | 19 ++++- internal/installer/installer_test.go | 50 +++++++++++- pkg/server/handlers/install.go | 43 +++++++++- pkg/server/handlers/install_test.go | 113 +++++++++++++++++++++++++++ 6 files changed, 258 insertions(+), 17 deletions(-) create mode 100644 pkg/server/handlers/install_test.go diff --git a/docs/api/install.mdx b/docs/api/install.mdx index 60c242d..47693c6 100644 --- a/docs/api/install.mdx +++ b/docs/api/install.mdx @@ -16,7 +16,9 @@ Fetch binary registry metadata with installation status. Supports two modes: | Parameter | Type | Default | Description | |-----------|------|---------|-------------| | `registry_mode` | string | `direct-fetch` | Registry mode: `direct-fetch` or `nix-build` | -| `registry_url` | string | _(embedded)_ | Custom registry URL or local file path. **Direct-fetch mode**: source for the full binary list. **Nix-build mode**: metadata overlay (desc, tags, version). Accepts HTTPS URLs or absolute file paths. | +| `registry_url` | string | _(embedded)_ | Custom registry URL or local file path. **Direct-fetch mode**: source for the full binary list. **Nix-build mode**: metadata overlay (desc, tags, version). Accepts `http(s)://` URLs or filesystem paths. | + +**Note on custom registries:** when `registry_url` is supplied, the `valide-command` of each entry is **not executed** — `installed` falls back to a `PATH` lookup. Only the embedded registry is trusted to run validation commands, because that field is run as shell and a registry author controls it. In nix-build mode, a `registry_url` that cannot be loaded returns `500` instead of silently dropping the metadata. --- @@ -163,7 +165,7 @@ curl "http://localhost:8002/osm/api/registry-info?registry_mode=nix-build®ist | Field | Type | Description | |-------|------|-------------| | `registry_mode` | string | Always `"nix-build"` | -| `registry_url` | string | Registry URL or file path used for metadata (empty string = embedded) | +| `registry_url` | string | Registry URL or file path used for metadata (the default registry URL when the embedded registry was used) | | `nix_installed` | boolean | Whether Nix package manager is installed | | `categories` | array | List of tool categories from flake.nix | | `categories[].name` | string | Category name (e.g., "Subdomain", "Vuln") | diff --git a/internal/installer/binary.go b/internal/installer/binary.go index bd6244b..a7d4ea0 100644 --- a/internal/installer/binary.go +++ b/internal/installer/binary.go @@ -22,6 +22,9 @@ import ( // DefaultRegistryURL is the default URL for the binary registry const DefaultRegistryURL = "https://raw.githubusercontent.com/osmedeus/osmedeus-base/main/registry-metadata.json" +// maxRegistrySize bounds how much registry JSON is read from a remote source +const maxRegistrySize = 32 << 20 // 32MB + // BinaryEntry represents a single binary's download/install information // Supports both download URLs and commands per OS/architecture type BinaryEntry struct { @@ -146,7 +149,16 @@ func fetchURL(url string, customHeaders map[string]string) ([]byte, error) { return nil, fmt.Errorf("HTTP status %d", resp.StatusCode) } - return io.ReadAll(resp.Body) + // Cap the read: the registry URL can come from an API caller, and an endpoint that + // streams endlessly would otherwise exhaust memory. Real registries are ~30KB. + data, err := io.ReadAll(io.LimitReader(resp.Body, maxRegistrySize+1)) + if err != nil { + return nil, err + } + if int64(len(data)) > maxRegistrySize { + return nil, fmt.Errorf("registry exceeds the %d byte limit", maxRegistrySize) + } + return data, nil } // DetectPackageManager returns the system's package manager @@ -292,7 +304,23 @@ func IsBinaryInPath(name string) bool { // IsBinaryInstalled checks if a binary is installed using validate command or PATH lookup // If entry has a ValidateCommand, run it and check exit code (0 = installed) // If ValidateCommand is empty, fall back to checking if binary name is in PATH +// +// Only call this for registries from a trusted source (embedded or operator-configured): +// ValidateCommand is executed as shell. For a registry loaded from a caller-supplied +// path or URL, use IsBinaryInstalledNoExec instead. func IsBinaryInstalled(name string, entry *BinaryEntry) bool { + return isBinaryInstalled(name, entry, true) +} + +// IsBinaryInstalledNoExec is IsBinaryInstalled without the shell execution: entries +// whose ValidateCommand is a shell command are checked against PATH instead of being +// run. Use it whenever the registry came from an untrusted source, since a registry +// author controls ValidateCommand and could otherwise run arbitrary code. +func IsBinaryInstalledNoExec(name string, entry *BinaryEntry) bool { + return isBinaryInstalled(name, entry, false) +} + +func isBinaryInstalled(name string, entry *BinaryEntry, allowExec bool) bool { // If validate command is provided and not empty, use it if entry != nil && entry.ValidateCommand != "" { vc := entry.ValidateCommand @@ -302,11 +330,15 @@ func IsBinaryInstalled(name string, entry *BinaryEntry) bool { if !strings.ContainsAny(vc, " \t|;&") { return IsBinaryInPath(vc) } - // @NOTE: This is intentional - ValidateCommand comes from the binary registry - // configuration which is a trusted source for installation validation commands. - cmd := exec.Command("sh", "-c", vc) - err := cmd.Run() - return err == nil // exit code 0 means installed + if allowExec { + // @NOTE: This is intentional - ValidateCommand comes from the binary registry + // configuration which is a trusted source for installation validation commands. + cmd := exec.Command("sh", "-c", vc) + err := cmd.Run() + return err == nil // exit code 0 means installed + } + // Untrusted registry: fall through to the PATH check below rather than + // executing a command the caller supplied. } // Fall back to default PATH check if IsBinaryInPath(name) { diff --git a/internal/installer/download.go b/internal/installer/download.go index a70544c..c990a54 100644 --- a/internal/installer/download.go +++ b/internal/installer/download.go @@ -83,9 +83,22 @@ func IsZipFile(source string) bool { return st == SourceTypeLocalZip || st == SourceTypeZipURL } -// isGitHubURL checks if a URL is a GitHub URL that can benefit from authentication -func isGitHubURL(url string) bool { - return strings.Contains(url, "github.com") || strings.Contains(url, "raw.githubusercontent.com") +// isGitHubURL checks if a URL is a GitHub URL that can benefit from authentication. +// Matches on the parsed hostname, not a substring: the caller uses this to decide +// whether to attach the GitHub token, and a lookalike host such as +// "evil.tld/?x=github.com" or "github.com.evil.tld" must never receive it. +func isGitHubURL(rawURL string) bool { + u, err := url.Parse(rawURL) + if err != nil { + return false + } + host := strings.ToLower(u.Hostname()) + for _, domain := range []string{"github.com", "githubusercontent.com"} { + if host == domain || strings.HasSuffix(host, "."+domain) { + return true + } + } + return false } // downloadMaxRetries is the number of retry attempts for transient download failures diff --git a/internal/installer/installer_test.go b/internal/installer/installer_test.go index 20d4eb5..037361a 100644 --- a/internal/installer/installer_test.go +++ b/internal/installer/installer_test.go @@ -1,6 +1,8 @@ package installer import ( + "os" + "path/filepath" "runtime" "testing" @@ -81,9 +83,9 @@ func TestMaybePrependSudo(t *testing.T) { } tests := []struct { - name string - input string - expect string + name string + input string + expect string }{ {"apt install", "apt install coreutils", "sudo apt install coreutils"}, {"apt-get install", "apt-get install -y curl", "sudo apt-get install -y curl"}, @@ -104,3 +106,45 @@ func TestMaybePrependSudo(t *testing.T) { }) } } + +func TestIsGitHubURL(t *testing.T) { + tests := []struct { + name string + url string + expect bool + }{ + {"github repo", "https://github.com/owner/repo", true}, + {"raw content", "https://raw.githubusercontent.com/owner/repo/main/f.json", true}, + {"api subdomain", "https://api.github.com/repos/owner/repo/releases", true}, + {"release objects", "https://objects.githubusercontent.com/foo", true}, + {"uppercase host", "https://GitHub.com/owner/repo", true}, + {"with port", "https://github.com:443/owner/repo", true}, + {"lookalike suffix host", "https://github.com.evil.tld/owner/repo", false}, + {"host as query param", "https://evil.tld/?x=github.com", false}, + {"host in path", "https://evil.tld/github.com/owner/repo", false}, + {"userinfo trick", "https://github.com@evil.tld/repo", false}, + {"unrelated host", "https://gitlab.com/owner/repo", false}, + {"empty", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expect, isGitHubURL(tt.url), + "the GitHub token is attached based on this check") + }) + } +} + +func TestIsBinaryInstalledNoExec(t *testing.T) { + marker := filepath.Join(t.TempDir(), "executed.txt") + entry := &BinaryEntry{ValidateCommand: "touch " + marker} + + assert.False(t, IsBinaryInstalledNoExec("definitely-not-a-real-binary-xyz", entry)) + _, err := os.Stat(marker) + assert.True(t, os.IsNotExist(err), "no-exec variant must not run the validate command") + + // The trusted variant still executes it + assert.True(t, IsBinaryInstalled("definitely-not-a-real-binary-xyz", entry)) + _, err = os.Stat(marker) + assert.NoError(t, err, "trusted variant should still run the validate command") +} diff --git a/pkg/server/handlers/install.go b/pkg/server/handlers/install.go index ba4d347..f5f4a09 100644 --- a/pkg/server/handlers/install.go +++ b/pkg/server/handlers/install.go @@ -36,9 +36,27 @@ func GetRegistryInfo(cfg *config.Config) fiber.Handler { } } +// isTrustedRegistry reports whether the registry source is one the server controls. +// An empty registry_url means the embedded registry; anything else was supplied by the +// caller and its entries must never be executed (see IsBinaryInstalledNoExec). +func isTrustedRegistry(registryPathOrURL string) bool { + return registryPathOrURL == "" +} + +// registryInstalledStatus reports install status for one entry, shelling out to its +// valide-command only when the registry came from a trusted source. +func registryInstalledStatus(name string, entry *installer.BinaryEntry, trusted bool) bool { + if trusted { + return installer.IsBinaryInstalled(name, entry) + } + return installer.IsBinaryInstalledNoExec(name, entry) +} + // getDirectFetchRegistry returns the direct-fetch registry (existing behavior) func getDirectFetchRegistry(c *fiber.Ctx) error { registryPathOrURL := c.Query("registry_url", "") + trusted := isTrustedRegistry(registryPathOrURL) + registry, err := installer.LoadRegistry(registryPathOrURL, nil) if err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ @@ -70,7 +88,7 @@ func getDirectFetchRegistry(c *fiber.Ctx) error { CommandDual: entry.CommandDual, MultiCommandsLinux: entry.MultiCommandsLinux, MultiCommandsDarwin: entry.MultiCommandsDarwin, - Installed: installer.IsBinaryInstalled(name, &entry), + Installed: registryInstalledStatus(name, &entry, trusted), Path: path, Optional: containsOptionalTag(entry.Tags), } @@ -104,7 +122,26 @@ func getNixBuildRegistry(c *fiber.Ctx) error { // Load registry for metadata (desc, tags) - use custom registry_url if provided registryPathOrURL := c.Query("registry_url", "") - registry, _ := installer.LoadRegistry(registryPathOrURL, nil) + trusted := isTrustedRegistry(registryPathOrURL) + + registry, err := installer.LoadRegistry(registryPathOrURL, nil) + if err != nil { + // Metadata is optional for the embedded registry, but a caller-supplied source + // that fails to load is a mistake the caller needs to see rather than a silent + // response with every desc/tags field missing. + if !trusted { + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ + "error": true, + "message": "Failed to load registry: " + err.Error(), + }) + } + registry = nil + } + + // Report the concrete source used, matching direct-fetch mode + if registryPathOrURL == "" { + registryPathOrURL = installer.DefaultRegistryURL + } // Build response with categories and tool metadata categoriesData := make([]map[string]interface{}, 0) @@ -121,7 +158,7 @@ func getNixBuildRegistry(c *fiber.Ctx) error { toolData := map[string]interface{}{ "name": tool, - "installed": installer.IsBinaryInstalled(tool, entryPtr), + "installed": registryInstalledStatus(tool, entryPtr, trusted), } if entryPtr != nil { toolData["desc"] = entryPtr.Desc diff --git a/pkg/server/handlers/install_test.go b/pkg/server/handlers/install_test.go new file mode 100644 index 0000000..6d8a2e5 --- /dev/null +++ b/pkg/server/handlers/install_test.go @@ -0,0 +1,113 @@ +package handlers + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/gofiber/fiber/v2" + "github.com/j3ssie/osmedeus/v5/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// writeRegistryFile writes a registry JSON file and returns its path +func writeRegistryFile(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "registry.json") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + return path +} + +func callRegistryInfo(t *testing.T, query string) (*http.Response, map[string]interface{}) { + t.Helper() + app := fiber.New() + app.Get("/osm/api/registry-info", GetRegistryInfo(&config.Config{})) + + resp, err := app.Test(httptest.NewRequest("GET", "/osm/api/registry-info"+query, nil), 10000) + require.NoError(t, err) + + raw, err := io.ReadAll(resp.Body) + require.NoError(t, err) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(raw, &body), "response: %s", raw) + return resp, body +} + +// TestGetRegistryInfo_CustomRegistryDoesNotExecuteValidateCommand guards the endpoint +// against command execution: valide-command is run as shell, so a registry supplied +// through the registry_url query param must never have it executed. "amass" is used as +// the entry name because nix-build mode only reads metadata for tools listed in flake.nix. +func TestGetRegistryInfo_CustomRegistryDoesNotExecuteValidateCommand(t *testing.T) { + marker := filepath.Join(t.TempDir(), "executed.txt") + registryPath := writeRegistryFile(t, `{ + "amass": {"desc": "hi", "valide-command": "touch `+marker+`"} + }`) + + for _, mode := range []string{"direct-fetch", "nix-build"} { + t.Run(mode, func(t *testing.T) { + resp, _ := callRegistryInfo(t, "?registry_mode="+mode+"®istry_url="+registryPath) + assert.Equal(t, 200, resp.StatusCode) + + _, statErr := os.Stat(marker) + assert.True(t, os.IsNotExist(statErr), + "valide-command from a caller-supplied registry must not be executed") + }) + } +} + +// TestGetRegistryInfo_EmbeddedRegistry checks the default response shape is unchanged. +func TestGetRegistryInfo_EmbeddedRegistry(t *testing.T) { + resp, body := callRegistryInfo(t, "") + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, "direct-fetch", body["registry_mode"]) + assert.NotEmpty(t, body["registry_url"], "embedded registry should report a concrete source") + assert.NotEmpty(t, body["binaries"]) +} + +// TestGetRegistryInfo_CustomRegistryReturnsItsBinaries checks the feature itself still +// works: a caller-supplied registry replaces the binary list in direct-fetch mode. +func TestGetRegistryInfo_CustomRegistryReturnsItsBinaries(t *testing.T) { + registryPath := writeRegistryFile(t, `{"only-tool": {"desc": "the only one"}}`) + + resp, body := callRegistryInfo(t, "?registry_url="+registryPath) + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, registryPath, body["registry_url"]) + + binaries, ok := body["binaries"].(map[string]interface{}) + require.True(t, ok) + assert.Contains(t, binaries, "only-tool") + assert.Len(t, binaries, 1) +} + +// TestGetRegistryInfo_NixBuildReportsRegistrySource checks both modes name a concrete +// source, so callers can tell which registry produced the metadata. +func TestGetRegistryInfo_NixBuildReportsRegistrySource(t *testing.T) { + registryPath := writeRegistryFile(t, `{"amass": {"desc": "custom desc"}}`) + + resp, body := callRegistryInfo(t, "?registry_mode=nix-build®istry_url="+registryPath) + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, "nix-build", body["registry_mode"]) + assert.Equal(t, registryPath, body["registry_url"]) + + _, defaultBody := callRegistryInfo(t, "?registry_mode=nix-build") + assert.NotEmpty(t, defaultBody["registry_url"], "embedded metadata source should be reported too") +} + +// TestGetRegistryInfo_BadCustomRegistryIsReported ensures a registry_url that cannot be +// loaded surfaces an error rather than a success response with the metadata silently gone. +func TestGetRegistryInfo_BadCustomRegistryIsReported(t *testing.T) { + missing := filepath.Join(t.TempDir(), "does-not-exist.json") + + for _, mode := range []string{"direct-fetch", "nix-build"} { + t.Run(mode, func(t *testing.T) { + resp, body := callRegistryInfo(t, "?registry_mode="+mode+"®istry_url="+missing) + assert.Equal(t, 500, resp.StatusCode) + assert.Equal(t, true, body["error"]) + }) + } +}