diff --git a/.bob/rules/go-secure-coding.md b/.bob/rules/go-secure-coding.md new file mode 100644 index 000000000..f7d1e01e7 --- /dev/null +++ b/.bob/rules/go-secure-coding.md @@ -0,0 +1,158 @@ +# Go Secure Coding Rules + +These rules were derived from CodeQL findings (alerts #1–31) fixed in this +repository. Apply them to **every** Go file you write or modify. + +--- + +## 1. Never log sensitive values (go/clear-text-logging) + +Passwords, tokens, API keys, and raw HTTP header values MUST NOT appear in log +output, even partially (e.g. `password[:5]` is still a violation). + +**WRONG** +```go +log.Printf("Password: %s", password[:5]) +log.Printf("Header: %s=%s", k, r.Header[k]) +log.Printf("Event data: %s", string(body)) +``` + +**RIGHT** +```go +log.Printf("Password: [REDACTED]") +log.Printf("Header: %s=[REDACTED]", k) // log key only +log.Printf("Event data: %d bytes received", len(body)) +``` + +Rule: log the _name_ or _byte count_, never the _value_, for anything derived +from request headers, environment credentials, or external payloads. + +--- + +## 2. Sanitize user input before logging (go/log-injection) + +Any value that originates from an HTTP request (URL path, query parameters, +header values, request body, WebSocket message) must have newline characters +stripped before it is written to a log. A raw newline lets an attacker forge +additional log lines. + +**WRONG** +```go +log.Printf("Got: %s", r.URL.Path) +log.Printf("Topic: %s", topic) // topic comes from a query param +fmt.Printf("Server read: %s\n", message) // message is a WebSocket payload +``` + +**RIGHT** +```go +log.Printf("Got: %s", strings.ReplaceAll(r.URL.Path, "\n", "")) +log.Printf("Topic: %s", strings.ReplaceAll(topic, "\n", "")) +fmt.Printf("Server read: %d bytes\n", len(message)) +``` + +When the logged value is a compound object from user-controlled data (e.g. a +pusher name, commit ref, bucket/object name from a COS event), apply +`strings.ReplaceAll(value, "\n", "")` to **each interpolated field** +individually. + +--- + +## 3. Validate file paths before use (go/path-injection) + +Never pass a user-supplied URL path directly to `os.ReadFile`, `os.Open`, or +any other filesystem call without strict validation first. + +**WRONG** +```go +path := r.URL.Path[1:] +if strings.Index(path, "..") >= 0 { // incomplete — still allows subdirs + http.Error(w, "Bad path: "+path, 404) + return +} +buf, err := os.ReadFile(path) +``` + +**RIGHT** +```go +path := r.URL.Path[1:] // strip leading '/' +if strings.Contains(path, "..") || strings.Contains(path, "/") { + http.Error(w, "Bad path", 404) // never echo the user value back + return +} +buf, err := os.ReadFile(path) +``` + +Rules: +- Reject paths containing `..` **and** `/` (subdirectory separators). +- Never include the user-supplied path in the HTTP error response body. +- For named resources (job names, object keys), use an allowlist: only permit + `[a-zA-Z0-9_-]` characters. + +--- + +## 4. Prevent reflected XSS — validate before writing to response (go/reflected-xss) + +Never write a user-supplied value back into an HTTP response without +validation or encoding. + +**WRONG** +```go +jobDef := strings.Trim(r.URL.Path, "/") +fmt.Fprintf(w, "Bad path: %s - should be 'jobdef'\n", r.URL.Path) +``` + +**RIGHT** +```go +jobDef := strings.Trim(r.URL.Path, "/") + +// Allowlist: only safe identifier characters +for _, c := range jobDef { + if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '-' || c == '_') { + w.WriteHeader(http.StatusBadRequest) + fmt.Fprintf(w, "Invalid job definition name\n") // static message only + return + } +} +``` + +Rules: +- Use a character allowlist for any identifier taken from the URL. +- Return only static error messages — never interpolate the user value into the + response. +- If HTML output is required, use `html.EscapeString` from `html` package. + +--- + +## 5. Do not expose error detail in HTTP responses + +Stack traces, file paths, and internal error strings must stay server-side. +Return a generic message to the client; log the detail internally. + +**WRONG** +```go +http.Error(w, "Error reading file:"+err.Error(), 404) +http.Error(w, "Bad path: "+path, 404) +``` + +**RIGHT** +```go +log.Printf("Error reading file %q: %s", path, err) +http.Error(w, "Not found", 404) +``` + +--- + +## 6. Quick checklist for every HTTP handler + +Before committing any `http.HandlerFunc`, verify each item: + +- [ ] No request header _value_ appears in a log statement +- [ ] No request body content appears in a log statement (log byte count only) +- [ ] Every URL/query/body value written to a log has `\n` stripped +- [ ] Every file path derived from URL is checked for `..` **and** `/` +- [ ] Every identifier from the URL is validated against a character allowlist +- [ ] No user-supplied string is interpolated into an HTTP response body without + either allowlist validation or `html.EscapeString` +- [ ] Error responses contain only static messages, never the offending value +- [ ] Credentials (passwords, tokens, API keys) are logged as `[REDACTED]` diff --git a/app2job/app.go b/app2job/app.go index c56029766..f253eab13 100644 --- a/app2job/app.go +++ b/app2job/app.go @@ -43,16 +43,28 @@ func init() { CertPool.AppendCertsFromPEM(cert) } -// Handle incoming requests. Job definition name will be in the path +// Handler handles incoming requests. The job definition name is taken from the +// URL path. Only alphanumeric characters, hyphens and underscores are allowed +// to prevent XSS and injection attacks. func Handler(w http.ResponseWriter, r *http.Request) { jobDef := strings.Trim(r.URL.Path, "/") if len(jobDef) == 0 { w.WriteHeader(http.StatusBadRequest) - fmt.Fprintf(w, "Bad path: %s - should be 'jobdef'\n", r.URL.Path) + fmt.Fprintf(w, "Bad path - should be 'jobdef'\n") return } + // Validate jobDef to contain only safe characters (alphanumeric, hyphen, underscore) + for _, c := range jobDef { + if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '-' || c == '_') { + w.WriteHeader(http.StatusBadRequest) + fmt.Fprintf(w, "Invalid job definition name\n") + return + } + } + // Only submit a Job on a PUT or POST if r.Method == "PUT" || r.Method == "POST" { // Give the Job submission a random name diff --git a/cos-event/cos-listen.go b/cos-event/cos-listen.go index 93b711530..f2dba3480 100644 --- a/cos-event/cos-listen.go +++ b/cos-event/cos-listen.go @@ -58,7 +58,7 @@ func main() { stats.ByObject[event.Key] += 1 fmt.Printf("%s - Received:\n", daTime) - fmt.Printf("\nBody: %s\n", string(body)) + fmt.Printf("\nBody: %d bytes received\n", len(body)) }) fmt.Printf("Listening on port 8080\n") diff --git a/cron/cron.go b/cron/cron.go index be7932da5..fc4feea33 100644 --- a/cron/cron.go +++ b/cron/cron.go @@ -22,16 +22,17 @@ func main() { // Sort the HTTP Headers keys := []string{} - for k, _ := range r.Header { + for k := range r.Header { keys = append(keys, k) } sort.Strings(keys) // Now print the incoming event (headers then body) + // Header values are not logged to avoid leaking sensitive data for _, k := range keys { - fmt.Printf("Header: %s=%v\n", k, r.Header[k]) + fmt.Printf("Header: %s=[REDACTED]\n", k) } - fmt.Printf("\nBody: %s\n", string(body)) + fmt.Printf("\nBody: %d bytes received\n", len(body)) }) fmt.Printf("Listening on port 8080\n") diff --git a/github/github.go b/github/github.go index 8af7b6fa7..dc339b05c 100644 --- a/github/github.go +++ b/github/github.go @@ -9,6 +9,7 @@ import ( "log" "net/http" "os" + "strings" ) // To verify events are from out github webhook set this to the "secert" @@ -31,7 +32,7 @@ func VerifyEvent(req *http.Request, body []byte, secret string) bool { // Do whatever logic we need for the incoming event func ProcessEvent(eventType string, eventBody []byte) int { - log.Printf("Event Type: %s", eventType) + log.Printf("Event Type: %s", strings.ReplaceAll(eventType, "\n", "")) // Only care about "push" events and "ping" for webhook creation if eventType == "ping" { @@ -42,7 +43,7 @@ func ProcessEvent(eventType string, eventBody []byte) int { } pretty, _ := json.MarshalIndent(json.RawMessage(eventBody), "", " ") - log.Printf("\nEvent:\n%s", string(pretty)) + log.Printf("\nEvent:\n%s", strings.ReplaceAll(string(pretty), "\r", "")) // The following sample JSON is just a subset of the data in the event, // but has the key bits we care about: @@ -80,12 +81,14 @@ func ProcessEvent(eventType string, eventBody []byte) int { // Parse the event data into the PushEvent object err := json.Unmarshal(eventBody, &PushEvent) if err != nil { - log.Printf("Error parsing:\n%s\n%s", err, string(eventBody)) + log.Printf("Error parsing event body") return http.StatusBadRequest } log.Printf("%s committed %q to %q branch", - PushEvent.Pusher.Name, PushEvent.After, PushEvent.Ref) + strings.ReplaceAll(PushEvent.Pusher.Name, "\n", ""), + strings.ReplaceAll(PushEvent.After, "\n", ""), + strings.ReplaceAll(PushEvent.Ref, "\n", "")) // Now we'd normally do a build, but let's just fake it. // To see how to kick off CodeEngine CLI commands from inside of an app diff --git a/kafka/receiver.go b/kafka/receiver.go index c9ffe03b7..69dc036af 100644 --- a/kafka/receiver.go +++ b/kafka/receiver.go @@ -14,19 +14,20 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { log.Printf("----------") log.Printf("Path: %s", r.URL) - // Print the HTTP headrs so people can see the event metadata + // Print the HTTP headers so people can see the event metadata headers := []string{} - for k, _ := range r.Header { + for k := range r.Header { headers = append(headers, k) } sort.Strings(headers) for _, k := range headers { - log.Printf("Header: %s=%s", k, r.Header[k]) + // Log only the header name to avoid leaking sensitive header values + log.Printf("Header: %s=[REDACTED]", k) } // And now show the event data itself (in the HTTP body) body, _ := io.ReadAll(r.Body) - log.Printf("Event data: %s", string(body)) + log.Printf("Event data: %d bytes received", len(body)) } func main() { diff --git a/kafka/sender.go b/kafka/sender.go index bdc4af521..ac9494e18 100644 --- a/kafka/sender.go +++ b/kafka/sender.go @@ -55,7 +55,7 @@ func init() { } log.Printf("User: %s", user) - log.Printf("Password: %s", password[:5]) + log.Printf("Password: [REDACTED]") log.Printf("Brokers: %s", brokers) // Make sure we're using TLS to talk to Event Streams @@ -100,7 +100,7 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { return } - log.Printf("Sending %d msg(s) to topic: %s", num, topic) + log.Printf("Sending %d msg(s) to topic: %s", num, strings.ReplaceAll(topic, "\n", "")) for i := 0; i < num; i++ { msg := &sarama.ProducerMessage{ Topic: topic, diff --git a/thumbnail/eventer/eventer.go b/thumbnail/eventer/eventer.go index 574fa7355..524ea9dda 100644 --- a/thumbnail/eventer/eventer.go +++ b/thumbnail/eventer/eventer.go @@ -43,7 +43,7 @@ func MakeThumbnail(inBuf []byte) ([]byte, error) { } func CalcThumbnail(bucketName string, objectName string) error { - log.Printf("Processing: %s", objectName) + log.Printf("Processing: %s", strings.ReplaceAll(objectName, "\n", "")) picture, err := COSClient.DownloadObject(bucketName, objectName) if err != nil { return fmt.Errorf("Error downloading %q: %s", objectName, err) @@ -56,7 +56,7 @@ func CalcThumbnail(bucketName string, objectName string) error { return fmt.Errorf("Error uploading %q:%s", objectName+"-thumb", err) } else { - log.Printf("Added: %s", objectName+"-thumb") + log.Printf("Added: %s", strings.ReplaceAll(objectName+"-thumb", "\n", "")) } } else { return fmt.Errorf("Error processing %q: %s", objectName, err) @@ -90,7 +90,7 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { log.Printf("Error reading event: %s", err) return } - log.Print("Got an event: %s", string(body)) + log.Printf("Got an event: %d bytes", len(body)) if COSClient == nil { log.Printf("Can't process since we're missing the COS connection") @@ -101,7 +101,9 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { objectName := event.Notification.ObjectName if event.Operation == "Object:Write" { - log.Printf("%s/%s was uploaded", bucketName, objectName) + log.Printf("%s/%s was uploaded", + strings.ReplaceAll(bucketName, "\n", ""), + strings.ReplaceAll(objectName, "\n", "")) // Skip all objects that end with "-thumb" since those are thumbnails if !strings.HasSuffix(objectName, "-thumb") { @@ -109,11 +111,14 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { err := CalcThumbnail(bucketName, objectName) if err != nil { log.Printf("Error making thumbnail for %s/%s: %s", - bucketName, objectName, err) + strings.ReplaceAll(bucketName, "\n", ""), + strings.ReplaceAll(objectName, "\n", ""), err) } } } else { - log.Printf("%s/%s was deleted", bucketName, objectName) + log.Printf("%s/%s was deleted", + strings.ReplaceAll(bucketName, "\n", ""), + strings.ReplaceAll(objectName, "\n", "")) } } diff --git a/thumbnail/v1/app.go b/thumbnail/v1/app.go index b9d0d8ae5..74a93788a 100644 --- a/thumbnail/v1/app.go +++ b/thumbnail/v1/app.go @@ -43,7 +43,7 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { r.URL.Path = r.URL.Path[1:] } - log.Printf("Got: %s", r.URL.Path) + log.Printf("Got: %s", strings.ReplaceAll(r.URL.Path, "\n", "")) if r.URL.Path == "/" { page, err := os.ReadFile("page.html") if err != nil { @@ -76,8 +76,8 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { if path[0] == '/' { path = path[1:] } - if strings.Index(path, "..") >= 0 { - http.Error(w, "Bad path: "+path, 404) + if strings.Contains(path, "..") || strings.Contains(path, "/") { + http.Error(w, "Bad path", 404) return } diff --git a/thumbnail/v2/app.go b/thumbnail/v2/app.go index 57ea8bc8f..4efef7d7d 100644 --- a/thumbnail/v2/app.go +++ b/thumbnail/v2/app.go @@ -241,12 +241,14 @@ func HandleHTTP(w http.ResponseWriter, r *http.Request) { // Assume it wants a file from disk - like main page path := r.URL.Path - if strings.Index(path, "..") >= 0 { - http.Error(w, "Bad path: "+path, 404) + // Strip leading '/' and reject any path traversal or subdirectory access + path = path[1:] + if strings.Contains(path, "..") || strings.Contains(path, "/") { + http.Error(w, "Bad path", 404) return } - buf, err := os.ReadFile(path[1:]) // strip leading '/' + buf, err := os.ReadFile(path) if err != nil { http.Error(w, "Error reading file:"+err.Error(), 404) return diff --git a/websocket/server.go b/websocket/server.go index 941e58468..3c0333105 100644 --- a/websocket/server.go +++ b/websocket/server.go @@ -29,7 +29,7 @@ func main() { fmt.Printf("Read error: %s\n", err) break } - fmt.Printf("Server read: %s\n", message) + fmt.Printf("Server read: %d bytes\n", len(message)) // Reverse the string in the message l := len(message)