From 9f94dded3f83fee6faf4fa48af986be5dadfcf90 Mon Sep 17 00:00:00 2001 From: Wasabules <39313803+Wasabules@users.noreply.github.com> Date: Sun, 6 Sep 2026 19:44:10 +0200 Subject: [PATCH] fix(security): address CodeQL and Dependabot findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL reported five alerts and Dependabot one advisory on the freshly enabled scanners. Each is handled below with what it does and does not mean. Dependabot: labstack/echo GHSA-vfp3-v2gw-7wfq (high) ----------------------------------------------------- Bumped 4.13.3 -> 4.15.3, an indirect dependency pulled in by Wails. Scope, measured rather than assumed: echo reaches this module only through wailsapp/wails/v2/internal/frontend/devserver, which is behind `//go:build dev`. `go list -deps ./...` links zero echo packages, `go list -tags dev -deps ./...` links one, and `go version -m` on the published v1.3.0 Windows binary finds no echo at all. So shipped builds never contained the vulnerable code and users were not exposed; the reachable surface was the developer machine during `wails dev`. Worth fixing, not worth an emergency release. CodeQL go/allocation-size-overflow (high) — crypto.go ----------------------------------------------------- EncryptBytes sizes its output buffer as headerLen + len(plaintext) + the GCM tag. That is an int addition, so on a 32-bit build a plaintext near 2 GiB would wrap and under-allocate before the copy ever ran. Its only caller passes a CA bundle of a few kilobytes, so this is not reachable today — but it is an exported entry point, and nothing in the signature says otherwise. Bounded at 64 MiB with an explicit error steering callers with real volume to EncryptFile, which streams instead of holding the whole payload. CodeQL go/unhandled-writable-file-close (4 warnings) — crypto.go, config.go ---------------------------------------------------------------------------- All four sit on error paths that close a file after a failed write and return the original error. Discarding the Close error is defensible there, but not free: on a buffered filesystem a deferred write failure surfaces only at Close, so the discarded error can be the one that says the file is incomplete. The temp file is removed straight after, which limits this to losing a diagnosis rather than corrupting anything. They now join both errors, so whichever one carries the useful detail survives. Also fixes the one real gofmt deviation in the tree (a double space before an inline comment in logstore.go). Note that `gofmt -l` flags three further files locally, which is a Windows CRLF artefact, not a formatting problem — the repo stores LF via .gitattributes, and a Linux checkout reports them clean. --- go.mod | 6 +++--- go.sum | 14 ++++++-------- internal/storage/config.go | 7 +++---- internal/storage/crypto.go | 21 ++++++++++++++++----- internal/storage/crypto_test.go | 33 +++++++++++++++++++++++++++++++++ internal/storage/logstore.go | 2 +- 6 files changed, 62 insertions(+), 21 deletions(-) diff --git a/go.mod b/go.mod index d097b36..0f8448b 100644 --- a/go.mod +++ b/go.mod @@ -21,13 +21,13 @@ require ( github.com/godbus/dbus/v5 v5.1.0 // indirect github.com/gorilla/websocket v1.5.3 // indirect github.com/jchv/go-winloader v0.0.0-20210711035445-715c2860da7e // indirect - github.com/labstack/echo/v4 v4.13.3 // indirect - github.com/labstack/gommon v0.4.2 // indirect + github.com/labstack/echo/v4 v4.15.3 // indirect + github.com/labstack/gommon v0.5.0 // indirect github.com/leaanthony/go-ansi-parser v1.6.1 // indirect github.com/leaanthony/gosod v1.0.4 // indirect github.com/leaanthony/slicer v1.6.0 // indirect github.com/leaanthony/u v1.1.1 // indirect - github.com/mattn/go-colorable v0.1.13 // indirect + github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.24 // indirect github.com/ncruces/go-strftime v1.0.0 // indirect github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c // indirect diff --git a/go.sum b/go.sum index 0f6018f..a86ad67 100644 --- a/go.sum +++ b/go.sum @@ -22,10 +22,10 @@ github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= github.com/jchv/go-winloader v0.0.0-20210711035445-715c2860da7e h1:Q3+PugElBCf4PFpxhErSzU3/PY5sFL5Z6rfv4AbGAck= github.com/jchv/go-winloader v0.0.0-20210711035445-715c2860da7e/go.mod h1:alcuEEnZsY1WQsagKhZDsoPCRoOijYqhZvPwLG0kzVs= -github.com/labstack/echo/v4 v4.13.3 h1:pwhpCPrTl5qry5HRdM5FwdXnhXSLSY+WE+YQSeCaafY= -github.com/labstack/echo/v4 v4.13.3/go.mod h1:o90YNEeQWjDozo584l7AwhJMHN0bOC4tAfg+Xox9q5g= -github.com/labstack/gommon v0.4.2 h1:F8qTUNXgG1+6WQmqoUWnz8WiEU60mXVVw0P4ht1WRA0= -github.com/labstack/gommon v0.4.2/go.mod h1:QlUFxVM+SNXhDL/Z7YhocGIBYOiwB0mXm1+1bAPHPyU= +github.com/labstack/echo/v4 v4.15.3 h1:lIdG4kK5RdMyhwCwSc4AmSQsLBb3AVwok6S8PX/9kwQ= +github.com/labstack/echo/v4 v4.15.3/go.mod h1:Xzp1Ns1RA2c9fY7nSgUJkpkUZGNbEIVHZbtbOMPktBI= +github.com/labstack/gommon v0.5.0 h1:6VSQ2NOzsnEJ5W6+84E0RbcaDDmgB6NIAzWCczTEe6c= +github.com/labstack/gommon v0.5.0/go.mod h1:Rzlg7HHy1maLfzBYGg9NZcVuz1sA68HHhLjhcEllYE0= github.com/leaanthony/debme v1.2.1 h1:9Tgwf+kjcrbMQ4WnPcEIUcQuIZYqdWftzZkBr+i/oOc= github.com/leaanthony/debme v1.2.1/go.mod h1:3V+sCm5tYAgQymvSOfYQ5Xx2JCr+OXiD9Jkw3otUjiA= github.com/leaanthony/go-ansi-parser v1.6.1 h1:xd8bzARK3dErqkPFtoF9F3/HgN8UQk0ed1YDKpEz01A= @@ -39,9 +39,8 @@ github.com/leaanthony/u v1.1.1/go.mod h1:9+o6hejoRljvZ3BzdYlVL0JYCwtnAsVuN9pVTQc github.com/matryer/is v1.4.0/go.mod h1:8I/i5uYgLzgsgEloJE1U6xx5HkBQpAZvepWuujKwMRU= github.com/matryer/is v1.4.1 h1:55ehd8zaGABKLXQUe2awZ99BD/PTc2ls+KV/dXphgEQ= github.com/matryer/is v1.4.1/go.mod h1:8I/i5uYgLzgsgEloJE1U6xx5HkBQpAZvepWuujKwMRU= -github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= -github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= -github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= +github.com/mattn/go-colorable v0.1.14 h1:9A9LHSqF/7dyVVX6g0U9cwm9pG3kP9gSzcuIPHPsaIE= +github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= github.com/mattn/go-isatty v0.0.24 h1:tGZZoVgT/KiqK1c8ocVLeDS8BSWMRd47J3Lbz7vsReI= github.com/mattn/go-isatty v0.0.24/go.mod h1:nMCL3Zebbrt45jsMDgnfIwz6ydEQApk5oEI3HqDio6A= github.com/minio/selfupdate v0.6.0 h1:i76PgT0K5xO9+hjzKcacQtO7+MjJ4JKA8Ak8XQ9DDwU= @@ -96,7 +95,6 @@ golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210228012217-479acdf4ea46/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= diff --git a/internal/storage/config.go b/internal/storage/config.go index f6ca138..5d45f8e 100644 --- a/internal/storage/config.go +++ b/internal/storage/config.go @@ -2,6 +2,7 @@ package storage import ( "encoding/json" + "errors" "log/slog" "os" "path/filepath" @@ -145,12 +146,10 @@ func writeFileSync(path string, data []byte, perm os.FileMode) error { return err } if _, err := f.Write(data); err != nil { - f.Close() - return err + return errors.Join(err, f.Close()) } if err := f.Sync(); err != nil { - f.Close() - return err + return errors.Join(err, f.Close()) } return f.Close() } diff --git a/internal/storage/crypto.go b/internal/storage/crypto.go index 0f5a21c..a27a1cb 100644 --- a/internal/storage/crypto.go +++ b/internal/storage/crypto.go @@ -112,7 +112,7 @@ func EncryptFileWithProgress(srcPath, dstPath, password string, progress Progres return fmt.Errorf("create temp file: %w", err) } if _, err := f.Write(header); err != nil { - f.Close() + err = errors.Join(err, f.Close()) os.Remove(tmpPath) return fmt.Errorf("write header: %w", err) } @@ -120,7 +120,7 @@ func EncryptFileWithProgress(srcPath, dstPath, password string, progress Progres emit(progress, "writing", 75+pct*25, sizeMB) }) if err != nil { - f.Close() + err = errors.Join(err, f.Close()) os.Remove(tmpPath) return fmt.Errorf("write ciphertext: %w", err) } @@ -129,7 +129,7 @@ func EncryptFileWithProgress(srcPath, dstPath, password string, progress Progres // leave a renamed-but-unflushed (zero/partial) .enc — that would lose every // stored log irreversibly. if err := f.Sync(); err != nil { - f.Close() + err = errors.Join(err, f.Close()) os.Remove(tmpPath) return fmt.Errorf("sync temp file: %w", err) } @@ -215,12 +215,12 @@ func DecryptFileWithProgress(srcPath, dstPath, password string, progress Progres emit(progress, "writing", 75+pct*25, sizeMB) }) if err != nil { - f.Close() + err = errors.Join(err, f.Close()) os.Remove(tmpPath) return fmt.Errorf("write decrypted file: %w", err) } if err := f.Sync(); err != nil { - f.Close() + err = errors.Join(err, f.Close()) os.Remove(tmpPath) return fmt.Errorf("sync temp file: %w", err) } @@ -292,10 +292,21 @@ func writeChunked(f *os.File, data []byte, onProgress func(pct float64)) error { return nil } +// maxInMemoryPlaintext bounds what EncryptBytes will take. Its only caller +// passes a CA bundle of a few kilobytes, but it is an exported entry point, and +// the capacity computed below (headerLen + len + tag) is an int addition that +// would wrap on a 32-bit build before the allocation ever happened. Anything +// approaching this size belongs in EncryptFile, which streams. +const maxInMemoryPlaintext = 64 << 20 // 64 MiB + // EncryptBytes encrypts plaintext with AES-256-GCM using a key derived // from password via Argon2id. Output format matches the file format: // [1 byte version][16 bytes salt][12 bytes nonce][ciphertext + GCM tag]. func EncryptBytes(plaintext []byte, password string) ([]byte, error) { + if len(plaintext) > maxInMemoryPlaintext { + return nil, fmt.Errorf("plaintext of %d bytes exceeds the %d byte in-memory limit; use EncryptFile", + len(plaintext), maxInMemoryPlaintext) + } salt := make([]byte, saltLen) if _, err := io.ReadFull(rand.Reader, salt); err != nil { return nil, fmt.Errorf("generate salt: %w", err) diff --git a/internal/storage/crypto_test.go b/internal/storage/crypto_test.go index 3b5a72d..a753c95 100644 --- a/internal/storage/crypto_test.go +++ b/internal/storage/crypto_test.go @@ -165,3 +165,36 @@ func TestEncryptEmptyFile(t *testing.T) { t.Error("decrypted empty file should be empty") } } + +func TestEncryptBytes_RejectsOversizedPlaintext(t *testing.T) { + // The capacity computed inside EncryptBytes is headerLen + len(plaintext) + + // the GCM tag, an int addition that would wrap on a 32-bit build before the + // allocation happened. The bound makes that unreachable and steers callers + // with real volume to EncryptFile, which streams. + oversized := make([]byte, maxInMemoryPlaintext+1) + if _, err := EncryptBytes(oversized, "password"); err == nil { + t.Fatal("EncryptBytes accepted a plaintext above the in-memory limit") + } + + // The bound itself must still be usable, and must round-trip. + atLimit := make([]byte, 4096) + for i := range atLimit { + atLimit[i] = byte(i) + } + enc, err := EncryptBytes(atLimit, "password") + if err != nil { + t.Fatalf("EncryptBytes on a normal payload: %v", err) + } + dec, err := DecryptBytes(enc, "password") + if err != nil { + t.Fatalf("DecryptBytes: %v", err) + } + if len(dec) != len(atLimit) { + t.Fatalf("round-tripped %d bytes, want %d", len(dec), len(atLimit)) + } + for i := range dec { + if dec[i] != atLimit[i] { + t.Fatalf("byte %d differs after round-trip", i) + } + } +} diff --git a/internal/storage/logstore.go b/internal/storage/logstore.go index 3126a2a..537b088 100644 --- a/internal/storage/logstore.go +++ b/internal/storage/logstore.go @@ -83,7 +83,7 @@ func openDatabase(dbPath string, config models.StorageConfig, emitter event.Even "?_pragma=journal_mode(wal)"+ "&_pragma=synchronous(normal)"+ "&_pragma=busy_timeout(5000)"+ - "&_pragma=cache_size(-64000)"+ // 64 MB cache + "&_pragma=cache_size(-64000)"+ // 64 MB cache "&_pragma=mmap_size(268435456)"+ // 256 MB mmap "&_pragma=temp_store(memory)") if err != nil {