diff --git a/skills/golang-security/SKILL.md b/skills/golang-security/SKILL.md index 5f13b13..36b7941 100644 --- a/skills/golang-security/SKILL.md +++ b/skills/golang-security/SKILL.md @@ -82,7 +82,7 @@ For the full methodology with Go examples, DFD trust boundaries, DREAD scoring, | Critical | SQL Injection | Parameterized queries separate data from code | `database/sql` with `?` placeholders | | Critical | Command Injection | Pass args separately, never via shell concatenation | `exec.Command` with separate args | | High | XSS | Auto-escaping renders user data as text, not HTML/JS | `html/template`, `text/template` | -| High | Path Traversal | Scope file access to a root, prevent `../` escapes | `os.Root` (Go 1.24+), `filepath.Clean` | +| High | Path Traversal | Scope untrusted file access to an allowed root | Go 1.24+: use `os.Root`. Pre-Go 1.24: use `filepath.IsLocal` + `filepath.Rel` + separator-aware checks; never rely on `filepath.Clean` + `strings.HasPrefix` alone. | | Medium | Timing Attacks | Constant-time comparison avoids byte-by-byte leaks | `crypto/subtle.ConstantTimeCompare` | | High | Crypto Issues | Use vetted algorithms; never roll your own | `crypto/aes`, `crypto/rand` | | Medium | HTTP Security | TLS + security headers prevent downgrade attacks | `net/http`, configure TLSConfig | @@ -120,12 +120,12 @@ For deeper security-specific analysis: ```bash # Go security checker (SAST) -go install github.com/securego/gosec/v2/cmd/gosec@latest -gosec ./... +go get -tool github.com/securego/gosec/v2/cmd/gosec@latest +go tool gosec ./... # Vulnerability scanner — see golang-dependency-management for full govulncheck usage -go install golang.org/x/vuln/cmd/govulncheck@latest -govulncheck ./... +go get -tool golang.org/x/vuln/cmd/govulncheck@latest +go tool govulncheck ./... ``` ### Security Testing diff --git a/skills/golang-security/evals/evals.json b/skills/golang-security/evals/evals.json index 7fce10c..3865605 100644 --- a/skills/golang-security/evals/evals.json +++ b/skills/golang-security/evals/evals.json @@ -17,16 +17,16 @@ }, { "id": 2, - "name": "bcrypt-72-byte-truncation", - "description": "Tests awareness of bcrypt's silent 72-byte password truncation — a Go-specific gotcha for long passwords", + "name": "bcrypt-72-byte-limit", + "description": "Tests awareness of bcrypt's 72-byte password limit in Go and correct long-password support", "prompt": "Write Go functions HashPassword and VerifyPassword using bcrypt. The system must support passwords up to 1000 characters long.", - "trap": "Model might use bcrypt directly without pre-hashing. bcrypt silently truncates input at 72 bytes, so passwords 'abc...73chars' and 'abc...73chars+anything' hash identically — a security hole for long passwords. The Go-idiomatic fix is to SHA-256 pre-hash before bcrypt (or use Argon2id).", + "trap": "Model might use bcrypt directly without handling long passwords. Go's bcrypt returns an error for passwords over 72 bytes, so a system that must support 1000-character passwords needs Argon2id/scrypt or a deliberate pre-hashing design before bcrypt.", "assertions": [ - {"id": "2.1", "text": "Addresses the bcrypt 72-byte truncation limit explicitly (either via comment, pre-hashing, or choosing Argon2id instead)"}, + {"id": "2.1", "text": "Addresses bcrypt's 72-byte password limit explicitly (either via comment, pre-hashing, or choosing Argon2id/scrypt instead)"}, {"id": "2.2", "text": "Either pre-hashes the password with SHA-256/SHA-512 before bcrypt, OR uses Argon2id/scrypt that have no such truncation limit"}, - {"id": "2.3", "text": "If using bcrypt directly without pre-hashing, warns that passwords longer than 72 bytes will be silently truncated"}, + {"id": "2.3", "text": "If using bcrypt directly without pre-hashing, handles the Go bcrypt error for passwords longer than 72 bytes"}, {"id": "2.4", "text": "Uses constant-time comparison — either bcrypt.CompareHashAndPassword or an equivalent that does not short-circuit"}, - {"id": "2.5", "text": "Does NOT add a manual length check like 'if len(password) > 72 { return error }' as a workaround (this reveals the truncation boundary to attackers via timing)"} + {"id": "2.5", "text": "Does NOT claim Go bcrypt silently truncates passwords; it either supports long passwords deliberately or returns a clear policy error"} ] }, { @@ -47,9 +47,9 @@ "id": 4, "name": "path-traversal-file-serving", "prompt": "Write a Go HTTP handler that serves user-uploaded files from a /var/www/uploads directory. The filename comes from the URL path parameter. Use Go 1.24+.", - "expected_output": "Uses os.Root for scoped file access, or validates path with filepath.Clean + prefix check. Prevents ../../../etc/passwd.", + "expected_output": "Uses os.Root for scoped file access. If Go <1.24 compatibility is required, uses filepath.IsLocal plus filepath.Rel with separator-aware checks; does not rely on Clean+HasPrefix.", "assertions": [ - {"id": "4.1", "text": "Uses os.OpenRoot to scope file access to /var/www/uploads (Go 1.24+ preferred), OR validates resolved path stays within uploads directory"}, + {"id": "4.1", "text": "Uses os.OpenRoot to scope file access to /var/www/uploads (Go 1.24+ preferred), OR for older Go uses filepath.IsLocal plus filepath.Rel with separator-aware checks"}, {"id": "4.2", "text": "Prevents path traversal via ../ sequences — does NOT just use filepath.Join without additional validation"}, {"id": "4.3", "text": "Does NOT leak system file paths in error responses to the client"}, {"id": "4.4", "text": "Returns appropriate HTTP status codes (404 for not found, 403 for traversal attempts)"}, @@ -142,10 +142,10 @@ "id": 11, "name": "zipslip-extraction", "prompt": "Write a Go function that extracts a ZIP archive uploaded by a user to a specified target directory. Use Go 1.24+.", - "expected_output": "Validates zip entry paths against traversal (ZipSlip). Uses os.Root or prefix check. Limits decompression size.", + "expected_output": "Validates zip entry paths against traversal (ZipSlip). Uses os.Root or a filepath.IsLocal/Rel fallback. Limits decompression size.", "assertions": [ - {"id": "11.1", "text": "Checks for path traversal in zip entry names (rejects entries containing .. or absolute paths)"}, - {"id": "11.2", "text": "Uses os.OpenRoot to scope extraction to target directory, OR validates extracted paths stay within target"}, + {"id": "11.1", "text": "Checks for path traversal in zip entry names with filepath.IsLocal or os.Root confinement"}, + {"id": "11.2", "text": "Uses os.OpenRoot to scope extraction to target directory, OR validates extracted paths with filepath.Rel and separator-aware checks"}, {"id": "11.3", "text": "Limits total decompression size or individual file size to prevent decompression bombs"}, {"id": "11.4", "text": "Does NOT just use filepath.Join(dest, file.Name) without validation"}, {"id": "11.5", "text": "Handles errors during extraction (corrupted entries, permission issues) without crashing"} diff --git a/skills/golang-security/references/cryptography.md b/skills/golang-security/references/cryptography.md index a3f34b0..99304c2 100644 --- a/skills/golang-security/references/cryptography.md +++ b/skills/golang-security/references/cryptography.md @@ -111,9 +111,14 @@ block, _ := aes.NewCipher(key) // Using block.Encrypt directly = ECB mode // Good — GCM provides authenticated encryption -aead, _ := cipher.NewGCM(block) // randomized, authenticated +aead, err := cipher.NewGCM(block) // randomized, authenticated +if err != nil { + return nil, err +} nonce := make([]byte, aead.NonceSize()) -rand.Read(nonce) +if _, err := rand.Read(nonce); err != nil { + return nil, err +} ciphertext := aead.Seal(nonce, nonce, plaintext, nil) ``` @@ -127,7 +132,9 @@ nonce := []byte("fixed_nonce!") // catastrophic with GCM // Good — random nonce per encryption nonce := make([]byte, 12) // 96-bit for GCM -rand.Read(nonce) +if _, err := rand.Read(nonce); err != nil { + return nil, err +} ``` ### Mistake 3: Non-constant-time comparison for secrets — Medium @@ -228,7 +235,10 @@ hash := argon2.IDKey([]byte(pw), salt, 3, 64*1024, 4, 32) // Or bcrypt (simpler API, no salt management): import "golang.org/x/crypto/bcrypt" -hash, _ := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.DefaultCost) +hash, err := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.DefaultCost) +if err != nil { + return nil, err +} // For general-purpose hashing (not passwords): import "crypto/sha256" @@ -380,17 +390,25 @@ key := argon2.IDKey([]byte(password), salt, 3, 64*1024, 4, 32) // Or bcrypt (simpler API, widely supported): import "golang.org/x/crypto/bcrypt" -hash, _ := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.DefaultCost) +hash, err := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.DefaultCost) +if err != nil { + return nil, err +} -// Or PBKDF2 with 600,000+ iterations: -import "golang.org/x/crypto/pbkdf2" -key := pbkdf2.Key([]byte(password), salt, 600000, 32, sha512.New) +// Or PBKDF2 with 600,000+ iterations (Go 1.24+ stdlib): +import "crypto/pbkdf2" +key, err := pbkdf2.Key(sha512.New, password, salt, 600_000, 32) +if err != nil { + return err +} // Or scrypt: import "golang.org/x/crypto/scrypt" key := scrypt.Key([]byte(password), salt, 32768, 8, 1, 32) ``` +For Go 1.24+, prefer stdlib `crypto/hkdf`, `crypto/pbkdf2`, and `crypto/sha3`. Use `golang.org/x/crypto/...` fallbacks only for modules targeting older Go versions or for algorithms still outside the standard library. + --- ## CWE References diff --git a/skills/golang-security/references/filesystem.md b/skills/golang-security/references/filesystem.md index 256c03a..5ad2d2b 100644 --- a/skills/golang-security/references/filesystem.md +++ b/skills/golang-security/references/filesystem.md @@ -4,7 +4,7 @@ Filesystem vulnerabilities can lead to unauthorized file access, data leakage, a **Rules:** -1. File paths MUST be sanitized against traversal (`../`). +1. User-controlled file paths MUST be confined to an allowed root. 2. `os.Root` SHOULD be used for scoped file access (Go 1.24+). 3. Zip extraction MUST check for ZipSlip path traversal. 4. Temporary files MUST use `os.CreateTemp` — NEVER predictable names. @@ -32,10 +32,34 @@ defer root.Close() f, err := root.Open(filename) // cannot escape root directory ``` -`os.Root` prevents path traversal at the OS level — no manual path validation needed. All operations (`Open`, `Create`, `Stat`, `OpenFile`, etc.) are confined to the root directory. Symlinks that resolve outside the root are rejected. +`os.Root` prevents ordinary path traversal at the OS level. All operations (`Open`, `Create`, `Stat`, `OpenFile`, etc.) are confined to the root directory, and symlinks that resolve outside the root are rejected. It is not a full sandbox: it does not by itself block bind mounts, special device files, or all `/proc`-style filesystem behavior. For archive extraction and uploads, still reject special files and choose a root without attacker-controlled mounts. **Good (pre-Go 1.24 fallback):** +```go +func safeJoin(baseDir, userPath string) (string, error) { + if userPath == "" || filepath.IsAbs(userPath) || !filepath.IsLocal(userPath) { + return "", errors.New("invalid relative path") + } + + full := filepath.Join(baseDir, userPath) + + rel, err := filepath.Rel(baseDir, full) + if err != nil { + return "", fmt.Errorf("checking path: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", errors.New("path escapes base directory") + } + + return full, nil +} +``` + +This lexical fallback is not a full symlink-resistant substitute for `os.Root`. + +**Bad:** + ```go fullPath := filepath.Join(baseDir, filename) if !strings.HasPrefix(filepath.Clean(fullPath), filepath.Clean(baseDir)) { @@ -76,13 +100,17 @@ for _, file := range reader.File { ```go for _, file := range reader.File { - if strings.Contains(file.Name, "..") || strings.HasPrefix(file.Name, "/") { - return errors.New("invalid path") + if !filepath.IsLocal(file.Name) { + return fmt.Errorf("unsafe archive path: %q", file.Name) } - targetPath := filepath.Join(dest, file.Name) - if !strings.HasPrefix(filepath.Clean(targetPath), filepath.Clean(dest)) { - return errors.New("path traversal attempt") + + targetPath, err := safeJoin(dest, file.Name) + if err != nil { + return err } + + // create parent directories, then write targetPath + _ = targetPath } ``` @@ -234,12 +262,9 @@ func readFile(filename string) ([]byte, error) { const allowedDir = "/var/www/public/" func readFile(filename string) ([]byte, error) { - if strings.Contains(filename, "..") { - return nil, errors.New("invalid filename") - } - fullPath := filepath.Join(allowedDir, filename) - if !strings.HasPrefix(filepath.Clean(fullPath), filepath.Clean(allowedDir)) { - return nil, errors.New("access denied") + fullPath, err := safeJoin(allowedDir, filename) + if err != nil { + return nil, err } return os.ReadFile(fullPath) } diff --git a/skills/golang-security/references/injection.md b/skills/golang-security/references/injection.md index f49bbd2..8dcca58 100644 --- a/skills/golang-security/references/injection.md +++ b/skills/golang-security/references/injection.md @@ -282,12 +282,12 @@ if strings.Contains(u.Hostname(), "metadata.") { ## Unsafe Deserialization — Critical -Deserializing untrusted input can lead to RCE. +Deserializing untrusted input can lead to resource exhaustion, type confusion, or unsafe object construction. **Bad:** ```go -dec := gob.NewDecoder(r.Body) // DON'T: gob can execute code +dec := gob.NewDecoder(r.Body) // DON'T: gob is not hardened for adversarial input var user interface{} dec.Decode(&user) ``` diff --git a/skills/golang-security/references/network.md b/skills/golang-security/references/network.md index 8da59c7..35b1be3 100644 --- a/skills/golang-security/references/network.md +++ b/skills/golang-security/references/network.md @@ -135,8 +135,11 @@ func checkPassword(input, secret string) bool { ```go import "crypto/subtle" -// For comparing tokens, MACs, or hashes (same-length values): +// For comparing fixed-length tokens or hashes: func checkToken(input, expected string) bool { + if len(input) != len(expected) { + return false + } return subtle.ConstantTimeCompare([]byte(input), []byte(expected)) == 1 } diff --git a/skills/golang-security/references/secrets.md b/skills/golang-security/references/secrets.md index b2e2896..5926749 100644 --- a/skills/golang-security/references/secrets.md +++ b/skills/golang-security/references/secrets.md @@ -80,11 +80,11 @@ func connectMySQL() (*sql.DB, error) { if password == "" { return nil, errors.New("DB_PASSWORD required") } - dsn := fmt.Sprintf("%s:%s@tcp(%s:%s)/%s", - user, password, - getEnvWithDefault("DB_HOST", "localhost"), - getEnvWithDefault("DB_PORT", "3306"), - getEnvWithDefault("DB_NAME", "mydb")) + host := getEnvWithDefault("DB_HOST", "localhost") + port := getEnvWithDefault("DB_PORT", "3306") + addr := net.JoinHostPort(host, port) + dsn := fmt.Sprintf("%s:%s@tcp(%s)/%s", + user, password, addr, getEnvWithDefault("DB_NAME", "mydb")) return sql.Open("mysql", dsn) }