tighten filesystem and crypto guidance (#39)

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

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"}

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

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)
 }

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)
 ```

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
 }
 

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)
 }