fix: address bugs and security gaps found in code audit round 2

- Fix blocking MikroTik channel sends that could leak goroutines
- Default to 10s timeout when check TimeoutMs is 0
- Add payload size limit to check_jobs event
- Add 30s write deadline to WebSocket writes
- Use random ICMP sequence numbers to prevent ping ID collisions
- Atomic host key file writes via temp+rename
- Sanitize update URLs in log output
- Zero correct buffer in sendBinaryResult after MarshalAppend
This commit is contained in:
Graham McIntire 2026-03-05 12:53:30 -06:00
parent b4f3dabe8d
commit 08d5957690
No known key found for this signature in database
7 changed files with 52 additions and 9 deletions

View file

@ -150,6 +150,9 @@ func runSession(ctx context.Context, baseURL, token string) error {
return return
} }
encoded := base64.StdEncoding.EncodeToString(bin) encoded := base64.StdEncoding.EncodeToString(bin)
// Zero the serialized protobuf data (may contain credentials).
// MarshalAppend may return a new backing array, so zero both.
zeroBytes(bin)
full := (*bp)[:cap(*bp)] full := (*bp)[:cap(*bp)]
zeroBytes(full) zeroBytes(full)
*bp = full[:0] *bp = full[:0]
@ -391,6 +394,10 @@ func handleMessage(
slog.Error("decode check_jobs payload", "error", err) slog.Error("decode check_jobs payload", "error", err)
return false return false
} }
if len(payload.Binary) > maxJobPayloadBytes {
slog.Error("check_jobs payload too large", "size", len(payload.Binary), "max", maxJobPayloadBytes)
return false
}
bin, err := base64.StdEncoding.DecodeString(payload.Binary) bin, err := base64.StdEncoding.DecodeString(payload.Binary)
if err != nil { if err != nil {
slog.Error("decode check_jobs base64", "error", err) slog.Error("decode check_jobs base64", "error", err)
@ -419,7 +426,7 @@ func handleMessage(
slog.Error("invalid update payload") slog.Error("invalid update payload")
return false return false
} }
slog.Info("update requested", "url", payload.URL) slog.Info("update requested", "url", sanitizeURL(payload.URL))
if err := doSelfUpdate(payload.URL, payload.Checksum); err != nil { if err := doSelfUpdate(payload.URL, payload.Checksum); err != nil {
slog.Error("self-update failed", "error", err) slog.Error("self-update failed", "error", err)
} }

View file

@ -66,6 +66,9 @@ func ExecuteCheck(ctx context.Context, check *pb.Check) *pb.CheckResult {
// executeHTTPCheck performs an HTTP/HTTPS check // executeHTTPCheck performs an HTTP/HTTPS check
func executeHTTPCheck(ctx context.Context, config *pb.HttpCheckConfig, timeoutMs uint32) (uint32, string, float64) { func executeHTTPCheck(ctx context.Context, config *pb.HttpCheckConfig, timeoutMs uint32) (uint32, string, float64) {
if timeoutMs == 0 {
timeoutMs = 10000
}
timeout := time.Duration(timeoutMs) * time.Millisecond timeout := time.Duration(timeoutMs) * time.Millisecond
// Create HTTP client with timeout and TLS config // Create HTTP client with timeout and TLS config
@ -141,6 +144,9 @@ func executeHTTPCheck(ctx context.Context, config *pb.HttpCheckConfig, timeoutMs
// executeTCPCheck performs a TCP port connectivity check // executeTCPCheck performs a TCP port connectivity check
func executeTCPCheck(ctx context.Context, config *pb.TcpCheckConfig, timeoutMs uint32) (uint32, string, float64) { func executeTCPCheck(ctx context.Context, config *pb.TcpCheckConfig, timeoutMs uint32) (uint32, string, float64) {
if timeoutMs == 0 {
timeoutMs = 10000
}
timeout := time.Duration(timeoutMs) * time.Millisecond timeout := time.Duration(timeoutMs) * time.Millisecond
address := net.JoinHostPort(config.Host, strconv.Itoa(int(config.Port))) address := net.JoinHostPort(config.Host, strconv.Itoa(int(config.Port)))
@ -181,6 +187,9 @@ func executeTCPCheck(ctx context.Context, config *pb.TcpCheckConfig, timeoutMs u
// executeDNSCheck performs a DNS resolution check // executeDNSCheck performs a DNS resolution check
func executeDNSCheck(ctx context.Context, config *pb.DnsCheckConfig, timeoutMs uint32) (uint32, string, float64) { func executeDNSCheck(ctx context.Context, config *pb.DnsCheckConfig, timeoutMs uint32) (uint32, string, float64) {
if timeoutMs == 0 {
timeoutMs = 10000
}
timeout := time.Duration(timeoutMs) * time.Millisecond timeout := time.Duration(timeoutMs) * time.Millisecond
resolver := &net.Resolver{} resolver := &net.Resolver{}

View file

@ -48,7 +48,11 @@ func (s *hostKeyStore) save() error {
if err != nil { if err != nil {
return err return err
} }
return os.WriteFile(s.path, data, 0600) tmp := s.path + ".tmp"
if err := os.WriteFile(tmp, data, 0600); err != nil {
return err
}
return os.Rename(tmp, s.path)
} }
// verify checks a fingerprint for host. Returns nil on match or first-use, error on mismatch. // verify checks a fingerprint for host. Returns nil on match or first-use, error on mismatch.

View file

@ -281,11 +281,15 @@ func executeMikrotikJob(ctx context.Context, job *pb.AgentJob, resultCh chan<- *
client, err := mikrotikDial(dev.Ip, dev.Port, dev.Username, dev.Password, dev.UseSsl) client, err := mikrotikDial(dev.Ip, dev.Port, dev.Username, dev.Password, dev.UseSsl)
if err != nil { if err != nil {
resultCh <- &pb.MikrotikResult{ select {
case resultCh <- &pb.MikrotikResult{
DeviceId: job.DeviceId, DeviceId: job.DeviceId,
JobId: job.JobId, JobId: job.JobId,
Error: fmt.Sprintf("connection failed: %v", err), Error: fmt.Sprintf("connection failed: %v", err),
Timestamp: timestamp, Timestamp: timestamp,
}:
default:
slog.Warn("result channel full", "job_id", job.JobId)
} }
return return
} }
@ -314,12 +318,16 @@ func executeMikrotikJob(ctx context.Context, job *pb.AgentJob, resultCh chan<- *
} }
} }
resultCh <- &pb.MikrotikResult{ select {
case resultCh <- &pb.MikrotikResult{
DeviceId: job.DeviceId, DeviceId: job.DeviceId,
JobId: job.JobId, JobId: job.JobId,
Sentences: allSentences, Sentences: allSentences,
Error: errorMessage, Error: errorMessage,
Timestamp: timestamp, Timestamp: timestamp,
}:
default:
slog.Warn("result channel full", "job_id", job.JobId)
} }
} }
@ -329,21 +337,29 @@ func executeMikrotikBackupViaSSH(job *pb.AgentJob, dev *pb.MikrotikDevice, resul
config, err := sshBackup(dev.Ip, uint16(dev.SshPort), dev.Username, dev.Password) config, err := sshBackup(dev.Ip, uint16(dev.SshPort), dev.Username, dev.Password)
if err != nil { if err != nil {
resultCh <- &pb.MikrotikResult{ select {
case resultCh <- &pb.MikrotikResult{
DeviceId: job.DeviceId, DeviceId: job.DeviceId,
JobId: job.JobId, JobId: job.JobId,
Error: fmt.Sprintf("SSH backup failed: %v", err), Error: fmt.Sprintf("SSH backup failed: %v", err),
Timestamp: timestamp, Timestamp: timestamp,
}:
default:
slog.Warn("result channel full", "job_id", job.JobId)
} }
return return
} }
resultCh <- &pb.MikrotikResult{ select {
case resultCh <- &pb.MikrotikResult{
DeviceId: job.DeviceId, DeviceId: job.DeviceId,
JobId: job.JobId, JobId: job.JobId,
Sentences: []*pb.MikrotikSentence{ Sentences: []*pb.MikrotikSentence{
{Attributes: map[string]string{"config": config}}, {Attributes: map[string]string{"config": config}},
}, },
Timestamp: timestamp, Timestamp: timestamp,
}:
default:
slog.Warn("result channel full", "job_id", job.JobId)
} }
} }

View file

@ -3,6 +3,7 @@ package main
import ( import (
"context" "context"
"fmt" "fmt"
"math/rand/v2"
"net" "net"
"os" "os"
"os/exec" "os/exec"
@ -72,12 +73,13 @@ func doICMPPing(ip net.IP, network string, isIPv4 bool, timeoutMs int) (float64,
} }
id := os.Getpid() & 0xffff id := os.Getpid() & 0xffff
seq := int(rand.Uint32() & 0xffff)
msg := icmp.Message{ msg := icmp.Message{
Type: msgType, Type: msgType,
Code: 0, Code: 0,
Body: &icmp.Echo{ Body: &icmp.Echo{
ID: id, ID: id,
Seq: 1, Seq: seq,
Data: []byte("towerops"), Data: []byte("towerops"),
}, },
} }
@ -121,7 +123,7 @@ func doICMPPing(ip net.IP, network string, isIPv4 bool, timeoutMs int) (float64,
switch rm.Type { switch rm.Type {
case ipv4.ICMPTypeEchoReply, ipv6.ICMPTypeEchoReply: case ipv4.ICMPTypeEchoReply, ipv6.ICMPTypeEchoReply:
echo, ok := rm.Body.(*icmp.Echo) echo, ok := rm.Body.(*icmp.Echo)
if !ok || echo.ID != id { if !ok || echo.ID != id || echo.Seq != seq {
continue continue
} }
return float64(elapsed.Microseconds()) / 1000.0, nil return float64(elapsed.Microseconds()) / 1000.0, nil

View file

@ -34,7 +34,7 @@ func selfUpdate(downloadURL, expectedChecksum string) error {
return fmt.Errorf("checksum required for update") return fmt.Errorf("checksum required for update")
} }
slog.Info("downloading update", "url", downloadURL) slog.Info("downloading update", "url", sanitizeURL(downloadURL))
resp, err := httpGet(downloadURL) resp, err := httpGet(downloadURL)
if err != nil { if err != nil {

View file

@ -199,6 +199,7 @@ func (ws *WSConn) Close() error {
} }
const wsReadTimeout = 90 * time.Second const wsReadTimeout = 90 * time.Second
const wsWriteTimeout = 30 * time.Second
func (ws *WSConn) readFrame() (opcode int, payload []byte, err error) { func (ws *WSConn) readFrame() (opcode int, payload []byte, err error) {
// Set read deadline to detect dead connections (MEDIUM-4) // Set read deadline to detect dead connections (MEDIUM-4)
@ -261,6 +262,10 @@ func (ws *WSConn) writeFrame(opcode int, payload []byte) error {
ws.mu.Lock() ws.mu.Lock()
defer ws.mu.Unlock() defer ws.mu.Unlock()
if nc, ok := ws.conn.(net.Conn); ok {
_ = nc.SetWriteDeadline(time.Now().Add(wsWriteTimeout))
}
length := len(payload) length := len(payload)
// Single buffer: max header (2+8+4=14) + payload // Single buffer: max header (2+8+4=14) + payload
buf := make([]byte, 0, 14+length) buf := make([]byte, 0, 14+length)