diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 61f66292..52abf85e 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -62,6 +62,13 @@ func (l *Logger) getWriter(path string) (*bufio.Writer, error) { return w, nil } + // belt-and-suspenders: logPath already flattens '/' out of the filename + // so path's parent is always dir itself, but a future caller could still + // hand getWriter a nested path directly, so make sure it exists too. + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + return nil, err + } + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) if err != nil { return nil, err @@ -122,13 +129,43 @@ func (l *Logger) Close() error { return firstErr } -// CreateFile initializes a log file for the given URL and writes the header. -func CreateFile(logFiles *[]string, url string, dir string) error { +// flattenPath folds every '/' and '\\' run in s into a single '_' and trims +// them from the ends. a target's path segments (or a stray backslash) would +// otherwise be read as an implied (and almost always missing) subdirectory +// tree; a URL maps to exactly one flat log file, never a directory tree. +func flattenPath(s string) string { + var b strings.Builder + b.Grow(len(s)) + prevSep := false + for i := 0; i < len(s); i++ { + c := s[i] + if c == '/' || c == '\\' { + if !prevSep { + b.WriteByte('_') + prevSep = true + } + continue + } + b.WriteByte(c) + prevSep = false + } + return strings.Trim(b.String(), "_") +} + +// logPath builds the flattened log-file path for a URL under dir. CreateFile +// and Write both go through it so the header and every later line for a +// target always land in the same file. +func logPath(dir, url string) string { sanitizedURL := url if _, after, ok := strings.Cut(url, "://"); ok { sanitizedURL = after } - path := filepath.Join(dir, sanitizedURL+".log") + return filepath.Join(dir, flattenPath(sanitizedURL)+".log") +} + +// CreateFile initializes a log file for the given URL and writes the header. +func CreateFile(logFiles *[]string, url string, dir string) error { + path := logPath(dir, url) header := fmt.Sprintf(" _____________\n__________(_)__ __/\n__ ___/_ /__ /_ \n_(__ )_ / _ __/ \n/____/ /_/ /_/ \n\nsif log file for %s\nhttps://sif.sh\n\n", url) @@ -142,7 +179,7 @@ func CreateFile(logFiles *[]string, url string, dir string) error { // Write appends text to the log file for the given URL. func Write(url string, dir string, text string) error { - path := filepath.Join(dir, url+".log") + path := logPath(dir, url) return defaultLogger.write(path, text) } diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go index a8fd36fe..803a54cf 100644 --- a/internal/logger/logger_test.go +++ b/internal/logger/logger_test.go @@ -116,6 +116,112 @@ func TestCreateFile(t *testing.T) { Close() } +// TestCreateFileURLWithPathDoesNotFail proves a target with a URL path (e.g. +// http://127.0.0.1:8931/) gets a writable log file. Before the fix, the +// scheme-stripped sanitized URL kept the '/' verbatim, so CreateFile tried to +// open "/127.0.0.1:8931/.log", a path whose parent directory was never +// created, and OpenFile failed, aborting the whole target's scan. +func TestCreateFileURLWithPathDoesNotFail(t *testing.T) { + tmpDir := t.TempDir() + + var logFiles []string + url := "http://127.0.0.1:8931/" + if err := CreateFile(&logFiles, url, tmpDir); err != nil { + t.Fatalf("CreateFile with path-bearing url: %v", err) + } + + if err := Flush(); err != nil { + t.Fatalf("Flush failed: %v", err) + } + + if len(logFiles) != 1 { + t.Fatalf("expected 1 log file, got %d", len(logFiles)) + } + + if filepath.Dir(logFiles[0]) != tmpDir { + t.Fatalf("expected log file directly under %q, got %q", tmpDir, logFiles[0]) + } + + content, err := os.ReadFile(logFiles[0]) + if err != nil { + t.Fatalf("log file not readable: %v", err) + } + if !strings.Contains(string(content), "sif log file for "+url) { + t.Errorf("expected header content, got %q", content) + } + + Close() +} + +// TestCreateFileURLWithMultiSlashAndQueryDoesNotFail covers a messier target: +// multiple path segments (and a doubled slash) plus query characters. None of +// it should ever produce a nested directory or a failed OpenFile. +func TestCreateFileURLWithMultiSlashAndQueryDoesNotFail(t *testing.T) { + tmpDir := t.TempDir() + + var logFiles []string + url := "https://example.com:8443//a/b/c?x=1&y=2" + if err := CreateFile(&logFiles, url, tmpDir); err != nil { + t.Fatalf("CreateFile with multi-slash/query url: %v", err) + } + + if err := Flush(); err != nil { + t.Fatalf("Flush failed: %v", err) + } + + if len(logFiles) != 1 { + t.Fatalf("expected 1 log file, got %d", len(logFiles)) + } + if filepath.Dir(logFiles[0]) != tmpDir { + t.Fatalf("expected log file directly under %q, got %q", tmpDir, logFiles[0]) + } + if _, err := os.Stat(logFiles[0]); err != nil { + t.Fatalf("log file not created: %v", err) + } + + Close() +} + +// TestWriteURLWithPathUsesSameFlatFile proves Write (used for every line after +// the header) resolves to the exact same path CreateFile wrote the header to, +// for a URL with a path component. Before the fix, both functions kept the raw +// '/' verbatim and would have hit the same missing-subdirectory error. +func TestWriteURLWithPathUsesSameFlatFile(t *testing.T) { + tmpDir := t.TempDir() + sanitizedURL := "127.0.0.1:8931/some/path" + + if err := WriteHeader(sanitizedURL, tmpDir, "test"); err != nil { + t.Fatalf("WriteHeader with path-bearing url: %v", err) + } + if err := Write(sanitizedURL, tmpDir, "line two\n"); err != nil { + t.Fatalf("Write with path-bearing url: %v", err) + } + if err := Flush(); err != nil { + t.Fatalf("Flush failed: %v", err) + } + + entries, err := os.ReadDir(tmpDir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + if len(entries) != 1 { + t.Fatalf("expected exactly 1 flat log file under %q, got %d entries", tmpDir, len(entries)) + } + if entries[0].IsDir() { + t.Fatalf("expected a flat file, got a directory: %s", entries[0].Name()) + } + + content, err := os.ReadFile(filepath.Join(tmpDir, entries[0].Name())) + if err != nil { + t.Fatalf("reading log file: %v", err) + } + if !strings.Contains(string(content), "line two") { + t.Errorf("Write did not land in the same file WriteHeader created: %q", content) + } + + Close() +} + func TestConcurrentWrites(t *testing.T) { tmpDir := t.TempDir() diff --git a/internal/scan/cloudstorage.go b/internal/scan/cloudstorage.go index e22a6007..349e8f0c 100644 --- a/internal/scan/cloudstorage.go +++ b/internal/scan/cloudstorage.go @@ -23,6 +23,7 @@ import ( "github.com/charmbracelet/log" "github.com/vmfunc/sif/internal/httpx" "github.com/vmfunc/sif/internal/logger" + "github.com/vmfunc/sif/internal/output" "github.com/vmfunc/sif/internal/styles" ) @@ -36,7 +37,7 @@ type CloudStorageResult struct { } func CloudStorage(url string, timeout time.Duration, logdir string) ([]CloudStorageResult, error) { - fmt.Println(styles.Separator.Render("Starting " + styles.Status.Render("Cloud Storage Misconfiguration Scan") + "...")) + output.ScanStart("Cloud Storage Misconfiguration Scan") sanitizedURL := stripScheme(url) diff --git a/internal/scan/js/frameworks/next.go b/internal/scan/js/frameworks/next.go index 86e76009..b8f775e4 100644 --- a/internal/scan/js/frameworks/next.go +++ b/internal/scan/js/frameworks/next.go @@ -24,7 +24,6 @@ package frameworks import ( "context" - "fmt" "io" "net/http" "regexp" @@ -32,6 +31,7 @@ import ( urlutil "github.com/projectdiscovery/utils/url" "github.com/vmfunc/sif/internal/httpx" + "github.com/vmfunc/sif/internal/output" ) // nextPagesRegex matches JavaScript file references in Next.js build manifest. @@ -49,7 +49,7 @@ func GetPagesRouterScripts(scriptUrl string) ([]string, error) { req, err := http.NewRequestWithContext(context.TODO(), http.MethodGet, scriptUrl, http.NoBody) if err != nil { - fmt.Println(err) + output.Error("%v", err) return nil, err } @@ -57,14 +57,14 @@ func GetPagesRouterScripts(scriptUrl string) ([]string, error) { // while still routing through the shared transport (proxy/headers/rate-limit). resp, err := httpx.Client(0).Do(req) if err != nil { - fmt.Println(err) + output.Error("%v", err) return nil, err } defer resp.Body.Close() body, err := io.ReadAll(io.LimitReader(resp.Body, maxManifestSize)) if err != nil { - fmt.Println(err) + output.Error("%v", err) return nil, err } // the manifest ships minified on one line; strip line breaks so the regex diff --git a/internal/scan/js/frameworks/stdout_bypass_test.go b/internal/scan/js/frameworks/stdout_bypass_test.go new file mode 100644 index 00000000..d8978fa5 --- /dev/null +++ b/internal/scan/js/frameworks/stdout_bypass_test.go @@ -0,0 +1,36 @@ +/* +·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━· +: : +: █▀ █ █▀▀ · Blazing-fast pentesting suite : +: ▄█ █ █▀ · BSD 3-Clause License : +: : +: (c) 2022-2026 vmfunc, xyzeva, : +: lunchcat alumni & contributors : +: : +·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━· +*/ + +package frameworks + +import ( + "os" + "strings" + "testing" +) + +// TestNextDoesNotPrintDirectlyToStdout mirrors internal/scan's guard: next.go +// used to fmt.Println(err) straight to os.Stdout, which can interleave/tear +// under -concurrency>1 since only the output sink is lock-wrapped there. +func TestNextDoesNotPrintDirectlyToStdout(t *testing.T) { + data, err := os.ReadFile("next.go") + if err != nil { + t.Fatalf("reading next.go: %v", err) + } + src := string(data) + if strings.Contains(src, "fmt.Println(") { + t.Error("next.go still calls fmt.Println directly, want output.Error instead") + } + if strings.Contains(src, "os.Stdout") { + t.Error("next.go references os.Stdout directly, want the output sink instead") + } +} diff --git a/internal/scan/stdout_bypass_test.go b/internal/scan/stdout_bypass_test.go new file mode 100644 index 00000000..508dd3f8 --- /dev/null +++ b/internal/scan/stdout_bypass_test.go @@ -0,0 +1,44 @@ +/* +·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━· +: : +: █▀ █ █▀▀ · Blazing-fast pentesting suite : +: ▄█ █ █▀ · BSD 3-Clause License : +: : +: (c) 2022-2026 vmfunc, xyzeva, : +: lunchcat alumni & contributors : +: : +·━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━· +*/ + +package scan + +import ( + "os" + "strings" + "testing" +) + +// TestModulesDoNotPrintDirectlyToStdout guards against status/error lines that +// bypass the output sink via a raw fmt.Println(os.Stdout, ...): under +// -concurrency>1 those interleave/tear with lines the rest of the run writes +// through output.Info/Error, since only the sink is wrapped with a lock. every +// module should route its chrome through the output package instead. +func TestModulesDoNotPrintDirectlyToStdout(t *testing.T) { + files := []string{ + "cloudstorage.go", + "subdomaintakeover.go", + } + for _, f := range files { + data, err := os.ReadFile(f) + if err != nil { + t.Fatalf("reading %s: %v", f, err) + } + src := string(data) + if strings.Contains(src, "fmt.Println(") { + t.Errorf("%s still calls fmt.Println directly, want output.Info/Error/ScanStart instead", f) + } + if strings.Contains(src, "os.Stdout") { + t.Errorf("%s references os.Stdout directly, want the output sink instead", f) + } + } +} diff --git a/internal/scan/subdomaintakeover.go b/internal/scan/subdomaintakeover.go index 884cc938..e1989584 100644 --- a/internal/scan/subdomaintakeover.go +++ b/internal/scan/subdomaintakeover.go @@ -25,6 +25,7 @@ import ( "github.com/charmbracelet/log" "github.com/vmfunc/sif/internal/httpx" "github.com/vmfunc/sif/internal/logger" + "github.com/vmfunc/sif/internal/output" "github.com/vmfunc/sif/internal/pool" "github.com/vmfunc/sif/internal/styles" ) @@ -70,7 +71,7 @@ var takeoverProviders = map[string]string{ // SubdomainTakeover checks dnsResults for dangling subdomains pointing at // unclaimed third-party services. func SubdomainTakeover(url string, dnsResults []string, timeout time.Duration, threads int, logdir string) ([]SubdomainTakeoverResult, error) { - fmt.Println(styles.Separator.Render("Starting " + styles.Status.Render("Subdomain Takeover Vulnerability Check") + "...")) + output.ScanStart("Subdomain Takeover Vulnerability Check") sanitizedURL := stripScheme(url) diff --git a/internal/store/store.go b/internal/store/store.go index af2cd885..6dfaa7fd 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -18,6 +18,8 @@ package store import ( + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "os" @@ -95,11 +97,26 @@ func sanitize(target string) string { return name } +// targetHashLen is how many hex characters of the target's sha256 are kept in +// the snapshot filename. 16 hex chars is 64 bits of the hash, astronomically +// collision-free for the number of targets a single run ever has, while +// keeping the filename short and stable. +const targetHashLen = 16 + // pathFor builds the absolute snapshot path for a target under dir. kept private -// so the sanitized-filename invariant lives in one place; Save and Load both go -// through it so a target always maps to the same file. +// so the filename invariant lives in one place; Save and Load both go through +// it so a target always maps to the same file. +// +// sanitize alone is lossy: it folds every separator run (and a literal '_') +// to one '_', so distinct targets like "https://a.com/x" and "https://a.com//x" +// (or "host:8443/path" and "host_8443_path") produce the identical string and +// would silently share, and clobber, one snapshot. appending a hash of the +// full, un-sanitized target makes the path injective for distinct targets +// while keeping the sanitized prefix for a human skimming the state dir. func pathFor(dir, target string) string { - return filepath.Join(dir, sanitize(target)+snapshotExt) + sum := sha256.Sum256([]byte(target)) + suffix := hex.EncodeToString(sum[:])[:targetHashLen] + return filepath.Join(dir, sanitize(target)+"-"+suffix+snapshotExt) } // Save writes the run's findings for target as a json snapshot under dir, @@ -126,12 +143,44 @@ func Save(dir, target string, findings []finding.Finding) error { } path := pathFor(dir, target) - if err := os.WriteFile(path, data, snapshotFileMode); err != nil { + if err := writeFileAtomic(dir, path, data); err != nil { return fmt.Errorf("writing snapshot %q: %w", path, err) } return nil } +// writeFileAtomic writes data to path without ever exposing a reader to a +// partially-written file: it writes to a temp file in the same directory +// (so the later rename is same-filesystem and atomic) and only renames it +// onto path once the write and close both succeed. under -concurrency>1, two +// targets whose sanitized names previously collided could otherwise race two +// os.WriteFile calls on the same path and interleave their writes; a rename +// is atomic, so a concurrent reader always sees one complete snapshot or the +// other, never a mix. +func writeFileAtomic(dir, path string, data []byte) error { + tmp, err := os.CreateTemp(dir, ".snapshot-*.tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + // on any early return the temp file must not linger; once the rename + // below succeeds this is a no-op (the path is already gone) and its + // error is deliberately discarded. + defer func() { _ = os.Remove(tmpPath) }() + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chmod(tmpPath, snapshotFileMode); err != nil { + return err + } + return os.Rename(tmpPath, path) +} + // Load reads the previously saved snapshot for target under dir. a missing // snapshot is not an error - it's the first run for that target, so an empty // slice comes back and the caller treats every current finding as new. a present diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 478b28c2..f53a2cb3 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -127,7 +127,7 @@ func TestLoadCorruptSnapshotErrors(t *testing.T) { // would silently re-flag every finding as new on every run. dir := t.TempDir() const target = "https://corrupt.test" - path := filepath.Join(dir, sanitize(target)+snapshotExt) + path := pathFor(dir, target) if err := os.WriteFile(path, []byte("{not json"), snapshotFileMode); err != nil { t.Fatalf("seeding corrupt snapshot: %v", err) } @@ -136,6 +136,96 @@ func TestLoadCorruptSnapshotErrors(t *testing.T) { } } +func TestPathForDistinctTargetsNeverCollide(t *testing.T) { + // see pathFor's doc comment (store.go) for why this used to collide. + dir := t.TempDir() + tests := [][2]string{ + {"https://a.com/x", "https://a.com//x"}, + {"https://a.com/x", "https://a_com/x"}, + {"host:8443/path", "host_8443_path"}, + } + for _, tt := range tests { + a, b := tt[0], tt[1] + if sanitize(a) != sanitize(b) { + t.Fatalf("test premise broken: sanitize(%q)=%q != sanitize(%q)=%q, pick a colliding pair", a, sanitize(a), b, sanitize(b)) + } + pa, pb := pathFor(dir, a), pathFor(dir, b) + if pa == pb { + t.Errorf("pathFor collided for distinct targets %q and %q: both %q", a, b, pa) + } + } +} + +func TestSaveDistinctCollidingTargetsRoundTripIndependently(t *testing.T) { + dir := t.TempDir() + a, b := "https://a.com/x", "https://a.com//x" + if sanitize(a) != sanitize(b) { + t.Fatalf("test premise broken: sanitize no longer collides for %q and %q", a, b) + } + + findingsA := []finding.Finding{{Target: a, Module: "headers", Severity: finding.SeverityInfo, Key: "a", Title: "a"}} + findingsB := []finding.Finding{{Target: b, Module: "headers", Severity: finding.SeverityInfo, Key: "b", Title: "b"}} + + if err := Save(dir, a, findingsA); err != nil { + t.Fatalf("Save a: %v", err) + } + if err := Save(dir, b, findingsB); err != nil { + t.Fatalf("Save b: %v", err) + } + + gotA, err := Load(dir, a) + if err != nil { + t.Fatalf("Load a: %v", err) + } + gotB, err := Load(dir, b) + if err != nil { + t.Fatalf("Load b: %v", err) + } + if !reflect.DeepEqual(gotA, findingsA) { + t.Errorf("Load(a) = %#v, want %#v (b's save must not have clobbered a)", gotA, findingsA) + } + if !reflect.DeepEqual(gotB, findingsB) { + t.Errorf("Load(b) = %#v, want %#v", gotB, findingsB) + } +} + +func TestSaveWritesAtomicallyViaTempAndRename(t *testing.T) { + // Save must never leave a stray temp file behind, and a reader must never + // observe a partially-written snapshot: it either sees the old file (not + // yet renamed) or the fully-written new one, never a half-written one at + // the final path. + dir := t.TempDir() + const target = "https://atomic.test" + + if err := Save(dir, target, sampleFindings()); err != nil { + t.Fatalf("Save: %v", err) + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + if len(entries) != 1 { + t.Fatalf("dir has %d entries after Save, want exactly 1 (no leftover temp file): %v", len(entries), entries) + } + if entries[0].Name() != filepath.Base(pathFor(dir, target)) { + t.Fatalf("unexpected file left in state dir: %q", entries[0].Name()) + } + + // a second Save (overwrite path) must also leave exactly one file, proving + // the temp file it wrote for the update got renamed/cleaned up too. + if err := Save(dir, target, nil); err != nil { + t.Fatalf("second Save: %v", err) + } + entries, err = os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir after second Save: %v", err) + } + if len(entries) != 1 { + t.Fatalf("dir has %d entries after overwrite, want exactly 1: %v", len(entries), entries) + } +} + func TestDiffAddedAndRemoved(t *testing.T) { base := sampleFindings()