From cdb820dce2d22fd075613ad37afd807bb34c397b Mon Sep 17 00:00:00 2001 From: Bakir Gracic Date: Sat, 17 Oct 2026 23:57:25 +0200 Subject: [PATCH] chore: resolve the OpenSSF Scorecard alerts Fuzzing: add Go native fuzz targets for relayPorts, validateThrottle, proxyURL, classify and splitPaths. Fix the relayPorts range check, which overflowed for huge BasePort or Transfers values and let them through (or panicked in make); both extremes are now fuzz seeds. Vulnerabilities: ignore GO-2026-5932 (x/crypto/openpgp, never imported) and GHSA-vfj7-8cjw-p6xm (braces, build-time only), neither of which has a fixed release, in osv-scanner.toml next to each manifest. Record the four dismissed alerts in docs/known-issues.md. --- crocmobile/errors_test.go | 40 ++++++++++++++ crocmobile/network.go | 2 +- crocmobile/network_test.go | 104 ++++++++++++++++++++++++++++++++++++ crocmobile/osv-scanner.toml | 5 ++ crocmobile/session_test.go | 26 +++++++++ docs/known-issues.md | 4 +- web/docs/osv-scanner.toml | 5 ++ 7 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 crocmobile/osv-scanner.toml create mode 100644 web/docs/osv-scanner.toml diff --git a/crocmobile/errors_test.go b/crocmobile/errors_test.go index 054ff97c..fc6c7be9 100644 --- a/crocmobile/errors_test.go +++ b/crocmobile/errors_test.go @@ -9,6 +9,7 @@ import ( "net" "os" "path/filepath" + "slices" "strconv" "strings" "sync" @@ -256,3 +257,42 @@ func TestStartErrorsCarryPayload(t *testing.T) { t.Error("startError(nil) != nil") } } + +func FuzzClassify(f *testing.F) { + for _, s := range []string{ + "pake not successful: peer uses unsupported PAKE protocol version 0", + "peer error: refusing files", "refused files", "room full", "bad password", + "could not connect to relay", "transfer disconnected", "unexpected EOF", "", + } { + f.Add(s) + } + codes := []string{ + codePeerVersion, codeRelayUnreachable, codeNoPublicRelay, codeDeclined, codeRefused, + codeRoomFull, codeBadPassword, codeWrongCode, codeCancelled, codeConnectionLost, + codeStoreLimit, codeStoreExpired, codeInvalidSetting, codeOther, + } + f.Fuzz(func(t *testing.T, msg string) { + err := errors.New(msg) + got := classify(err) + if !slices.Contains(codes, got) { + t.Fatalf("classify(%q) = %q, not a known failure code", msg, got) + } + if classify(err) != got { + t.Fatalf("classify(%q) is not deterministic", msg) + } + // Wrapping must not change the code: the app relies on errors.As/Is and + // on text matching seeing through croc's own wrapping. + if w := classify(fmt.Errorf("wrapped: %w", err)); w != got && w != codeCancelled { + t.Fatalf("classify(wrapped %q) = %s, want %s", msg, w, got) + } + // A coded error carries its code regardless of its text. + if c := classify(&codedError{codeInvalidSetting, err}); c != codeInvalidSetting { + t.Fatalf("classify(coded, %q) = %s, want %s", msg, c, codeInvalidSetting) + } + // The failure payload must round-trip through JSON for any message. + var f failure + if jerr := json.Unmarshal([]byte(errorJSON(err)), &f); jerr != nil || f.Code != got { + t.Fatalf("errorJSON(%q) decoded to %+v, %v; want code %s", msg, f, jerr, got) + } + }) +} diff --git a/crocmobile/network.go b/crocmobile/network.go index 575d7bd8..384ab159 100644 --- a/crocmobile/network.go +++ b/crocmobile/network.go @@ -32,7 +32,7 @@ func relayPorts(o *Options) ([]string, error) { if n == 0 { n = DefaultTransfers } - if base < 1 || n < 1 || base+n > 65535 { + if base < 1 || n < 1 || base > 65535-n { return nil, invalidSetting("ports %d to %d are out of range", base, base+n) } ports := make([]string, n+1) diff --git a/crocmobile/network_test.go b/crocmobile/network_test.go index f5d80a2a..a1e68e33 100644 --- a/crocmobile/network_test.go +++ b/crocmobile/network_test.go @@ -3,10 +3,14 @@ package crocmobile import ( "encoding/json" "io" + "math" "net" "net/http" "path/filepath" "reflect" + "regexp" + "slices" + "strconv" "strings" "sync/atomic" "testing" @@ -283,3 +287,103 @@ func codeOf(err error) string { } return classify(err) } + +func FuzzRelayPorts(f *testing.F) { + f.Add(0, 0) + f.Add(9100, 2) + f.Add(50000, 1) + f.Add(65533, 2) + f.Add(65535, 0) + f.Add(-1, 0) + f.Add(0, -1) + f.Add(math.MaxInt, 1) + f.Add(1, math.MaxInt) + f.Fuzz(func(t *testing.T, base, transfers int) { + o := Options{BasePort: base, Transfers: transfers} + ports, err := relayPorts(&o) + if err != nil { + if codeOf(err) != codeInvalidSetting { + t.Fatalf("relayPorts(%+v) err = %v, want %s", o, err, codeInvalidSetting) + } + return + } + if base == 0 { + base = DefaultBasePort + } + if transfers == 0 { + transfers = DefaultTransfers + } + if len(ports) != transfers+1 { + t.Fatalf("relayPorts(%+v) gave %d ports, want %d", o, len(ports), transfers+1) + } + for i, p := range ports { + n, err := strconv.Atoi(p) + if err != nil || n != base+i || n < 1 || n > 65535 { + t.Fatalf("relayPorts(%+v)[%d] = %q, want %d within 1-65535", o, i, p, base+i) + } + } + }) +} + +var throttleSpeed = regexp.MustCompile(`^[0-9]+[kKmMgG]?$`) + +func FuzzValidateThrottle(f *testing.F) { + for _, s := range []string{"", "500k", " 2M ", "1g", "12", "5", "0k", "k", "kk", "-5k", "+5k", "5kb", "1.5m"} { + f.Add(s) + } + f.Fuzz(func(t *testing.T, value string) { + err := validateThrottle(value) + if err != nil { + if codeOf(err) != codeInvalidSetting { + t.Fatalf("validateThrottle(%q) err = %v, want %s", value, err, codeInvalidSetting) + } + return + } + v := strings.TrimSpace(value) + if v == "" { + return + } + // croc panics on anything but digits plus one optional unit letter, on + // zero, and ignores a one-character value. + if len(v) < 2 || !throttleSpeed.MatchString(v) || strings.Trim(v, "0kKmMgG") == "" { + t.Fatalf("validateThrottle(%q) accepted a value croc cannot parse", value) + } + }) +} + +func FuzzProxyURL(f *testing.F) { + for _, s := range []string{ + "", "127.0.0.1:1080", "proxy.example:3128", "socks5://user:secret@proxy.example:1080", + "socks5h://proxy.example:1080", "http://user:secret@proxy.example:3128", " https://proxy.example:443 ", + "proxy.example", "socks5://:1080", "user:secret@proxy.example", "proxy.example:0", "proxy.example:70000", + } { + f.Add(s) + } + f.Fuzz(func(t *testing.T, value string) { + for _, c := range []struct { + scheme string + allowed []string + }{ + {"socks5", []string{"socks5", "socks5h"}}, + {"http", []string{"http", "https"}}, + } { + u, err := proxyURL(value, c.scheme, c.allowed...) + if err != nil { + if u != nil || codeOf(err) != codeInvalidSetting { + t.Fatalf("proxyURL(%q, %s) = %v, %v; want nil and %s", value, c.scheme, u, err, codeInvalidSetting) + } + continue + } + if u == nil { + if strings.TrimSpace(value) != "" { + t.Fatalf("proxyURL(%q, %s) = nil, nil for a non-empty value", value, c.scheme) + } + continue + } + port, perr := strconv.Atoi(u.Port()) + if !slices.Contains(c.allowed, u.Scheme) || u.Hostname() == "" || perr != nil || port < 1 || port > 65535 { + t.Fatalf("proxyURL(%q, %s) accepted %q: scheme, host or port invalid", value, c.scheme, u) + } + } + }) +} diff --git a/crocmobile/osv-scanner.toml b/crocmobile/osv-scanner.toml new file mode 100644 index 00000000..703c91c9 --- /dev/null +++ b/crocmobile/osv-scanner.toml @@ -0,0 +1,5 @@ +# Read by OpenSSF Scorecard's Vulnerabilities check. Keep in step with docs/known-issues.md. + +[[IgnoredVulns]] +id = "GO-2026-5932" +reason = "golang.org/x/crypto/openpgp is never imported: croc only uses x/crypto/argon2, and govulncheck reports the vulnerability as not called. No fixed x/crypto version exists." diff --git a/crocmobile/session_test.go b/crocmobile/session_test.go index 2d179f34..6657eff2 100644 --- a/crocmobile/session_test.go +++ b/crocmobile/session_test.go @@ -845,3 +845,29 @@ func TestPollReportsFileListOfTransferEndedWithinATick(t *testing.T) { t.Fatalf("OnFileList called %d times, want 1", len(d.lists)) } } + +func FuzzSplitPaths(f *testing.F) { + for _, s := range []string{"", "/d/report.pdf \n\n /d/x\n", "a\nb\nc", "\n\n", " a \n b"} { + f.Add(s) + } + f.Fuzz(func(t *testing.T, joined string) { + got := splitPaths(joined) + var want []string + for _, line := range strings.Split(joined, "\n") { + if line != "" { + want = append(want, line) + } + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("splitPaths(%q) = %q, want every non-empty line verbatim: %q", joined, got, want) + } + for _, p := range got { + if p == "" || strings.Contains(p, "\n") { + t.Fatalf("splitPaths(%q) returned entry %q", joined, p) + } + } + if again := splitPaths(strings.Join(got, "\n")); !reflect.DeepEqual(again, got) { + t.Fatalf("splitPaths does not round-trip: %q then %q", got, again) + } + }) +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 7530b4e7..36bd58aa 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -72,7 +72,9 @@ None. - **The trust badge names the configured route, not the path that carried the data.** `TrustBadge` reads the relay setting, so a transfer that croc moved over the LAN, or over Tailcat under Connection = Automatic, still shows "Via the public croc relay" (or the custom relay). croc does not report the winning path to the engine. Accepted: the badge's claim that the server sees only encrypted data holds on every path. - **Receive list identity is by file name.** Duplicate names within one transfer are a croc-level concern, not a UI one. - **Gesture-level flows are not machine-verified.** Drag-and-drop, camera QR, and taps have no XCUITest coverage, by decision: unit tests cover non-UI logic (Go `go test -race`, Swift Testing in `CrocKitTests` and `CrocAppTests`, all in CI), transfers are covered by the harnesses, and UI flows are verified by hand. The rule is in `CONTRIBUTING.md` (Verification, Tests). -- **`braces` <= 3.0.3 (GHSA-vfj7-8cjw-p6xm) has no patched release.** Stack exhaustion on deeply nested brace patterns; it is reached only at build time through `chokidar` and `fast-glob` over our own docs paths, never from user input, and no override can lift it. `pnpm audit` lists it, and any Dependabot alert for it is dismissed with this reason. Remove this item when `braces` publishes a fix. +- **`braces` <= 3.0.3 (GHSA-vfj7-8cjw-p6xm) has no patched release.** Stack exhaustion on deeply nested brace patterns; it is reached only at build time through `chokidar` and `fast-glob` over our own docs paths, never from user input, and no override can lift it. `pnpm audit` lists it, and any Dependabot alert for it is dismissed with this reason. `web/docs/osv-scanner.toml` ignores it so OpenSSF Scorecard's Vulnerabilities check does not count it. Remove this item, and that entry, when `braces` publishes a fix. +- **`golang.org/x/crypto` carries GO-2026-5932 (the unmaintained `openpgp` package) with no fixed version.** The advisory covers the whole module, but nothing imports `openpgp`: croc uses only `x/crypto/argon2`, and `go tool govulncheck ./...` in `crocmobile/` reports it as required but not called. `crocmobile/osv-scanner.toml` ignores it for Scorecard. Remove both if x/crypto drops `openpgp` or a dependency starts importing it. +- **Four OpenSSF Scorecard alerts are dismissed as "won't fix".** Scorecard opens a code-scanning alert for every check below 10, and these cannot reach 10 with one maintainer: Branch-Protection (#1; 10 needs two approving reviewers with admins included, so nothing could merge; `main` keeps PRs, the `ci pass` check, linear history, and no force push or deletion), Code-Review (#5; needs approvals from someone other than the author), Maintained (#4; fires only while the repo is under 90 days old, created 2026-07-21), and CII-Best-Practices (#6; only the gold badge scores 10, and gold needs several developers). Dismissed 2026-10-17. Revisit Branch-Protection and Code-Review if a second maintainer joins. - **`xfer()` has no watchdog.** If croc ever blocks past context cancel and prompt-pipe close, `activeMu` is held for the process's lifetime and every later transfer returns "another transfer is active". Accepted: forcing the lock open while croc is still running would let two sessions mutate the same process-global state (`os.Chdir`, stdin/stdout swaps). - **`poll()` reads `croc.Client` scalar and slice fields with no synchronization** (croc's own goroutines write them unguarded). Degrades to inconsistent progress numbers; bounds-checked (`session.go`'s `progressSnapshot`) so it cannot panic. Accepted: the writes are croc's, so a fix needs upstream locking, and the worst case is a progress number that is briefly wrong. - **The harnesses fail at random against the public relays when they rate-limit.** Accepted: the limit is relay policy, and CI does not depend on it. A croc v11 relay admits 30 connections a minute from one address (`DEFAULT_SOURCE_JOIN_LIMIT`, `DEFAULT_JOIN_LIMIT_WINDOW`), and a multiplexed transfer opens 9 per peer (1 control, 8 data), so with both peers on one machine two transfers on the same pool relay inside a minute can be refused with `relay admission rate limited`. `verify-interop.sh`, `verify-app-mac.sh` and `verify-share-sim.sh` then fail at whichever scenario hits it; on 2026-10-12 back-to-back `verify-interop.sh` runs failed at scenarios 6, 7 and 10 (then the pool scenario, now 11) this way, and the pre-v11 runs saw the same thing from `croc.schollz.com`. `verify-interop.sh` scenario 11 waits 60 s and turns multiplexing off for that reason. CI avoids this: its `interop (local relay)` job runs `verify-interop.sh` with `RELAY` set to a local `croc relay` with raised admission limits (`docs/BUILDING.md`, section 5). The other harnesses have no local-relay mode yet, so use a local relay for a deterministic result, leave a minute between public-relay runs, and treat a failure as a regression only after finding the cause in its log. diff --git a/web/docs/osv-scanner.toml b/web/docs/osv-scanner.toml new file mode 100644 index 00000000..6640c5a2 --- /dev/null +++ b/web/docs/osv-scanner.toml @@ -0,0 +1,5 @@ +# Read by OpenSSF Scorecard's Vulnerabilities check. Keep in step with docs/known-issues.md. + +[[IgnoredVulns]] +id = "GHSA-vfj7-8cjw-p6xm" +reason = "braces <= 3.0.3 has no patched release. Reached only at docs build time through chokidar, micromatch and fast-glob over our own paths, never from user input."