From 08d595769062ac675c3625bf76edfebee9290a17 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Thu, 5 Mar 2026 12:53:30 -0600 Subject: [PATCH] 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 --- agent.go | 9 ++++++++- checks.go | 9 +++++++++ hostkeys.go | 6 +++++- mikrotik.go | 24 ++++++++++++++++++++---- ping.go | 6 ++++-- update.go | 2 +- websocket.go | 5 +++++ 7 files changed, 52 insertions(+), 9 deletions(-) diff --git a/agent.go b/agent.go index 0c8cd3b..62c64f6 100644 --- a/agent.go +++ b/agent.go @@ -150,6 +150,9 @@ func runSession(ctx context.Context, baseURL, token string) error { return } 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)] zeroBytes(full) *bp = full[:0] @@ -391,6 +394,10 @@ func handleMessage( slog.Error("decode check_jobs payload", "error", err) 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) if err != nil { slog.Error("decode check_jobs base64", "error", err) @@ -419,7 +426,7 @@ func handleMessage( slog.Error("invalid update payload") 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 { slog.Error("self-update failed", "error", err) } diff --git a/checks.go b/checks.go index 4b21e76..5ecb850 100644 --- a/checks.go +++ b/checks.go @@ -66,6 +66,9 @@ func ExecuteCheck(ctx context.Context, check *pb.Check) *pb.CheckResult { // executeHTTPCheck performs an HTTP/HTTPS check func executeHTTPCheck(ctx context.Context, config *pb.HttpCheckConfig, timeoutMs uint32) (uint32, string, float64) { + if timeoutMs == 0 { + timeoutMs = 10000 + } timeout := time.Duration(timeoutMs) * time.Millisecond // 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 func executeTCPCheck(ctx context.Context, config *pb.TcpCheckConfig, timeoutMs uint32) (uint32, string, float64) { + if timeoutMs == 0 { + timeoutMs = 10000 + } timeout := time.Duration(timeoutMs) * time.Millisecond 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 func executeDNSCheck(ctx context.Context, config *pb.DnsCheckConfig, timeoutMs uint32) (uint32, string, float64) { + if timeoutMs == 0 { + timeoutMs = 10000 + } timeout := time.Duration(timeoutMs) * time.Millisecond resolver := &net.Resolver{} diff --git a/hostkeys.go b/hostkeys.go index 80c32dd..4470022 100644 --- a/hostkeys.go +++ b/hostkeys.go @@ -48,7 +48,11 @@ func (s *hostKeyStore) save() error { if err != nil { 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. diff --git a/mikrotik.go b/mikrotik.go index 3c54a38..91b88b9 100644 --- a/mikrotik.go +++ b/mikrotik.go @@ -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) if err != nil { - resultCh <- &pb.MikrotikResult{ + select { + case resultCh <- &pb.MikrotikResult{ DeviceId: job.DeviceId, JobId: job.JobId, Error: fmt.Sprintf("connection failed: %v", err), Timestamp: timestamp, + }: + default: + slog.Warn("result channel full", "job_id", job.JobId) } 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, JobId: job.JobId, Sentences: allSentences, Error: errorMessage, 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) if err != nil { - resultCh <- &pb.MikrotikResult{ + select { + case resultCh <- &pb.MikrotikResult{ DeviceId: job.DeviceId, JobId: job.JobId, Error: fmt.Sprintf("SSH backup failed: %v", err), Timestamp: timestamp, + }: + default: + slog.Warn("result channel full", "job_id", job.JobId) } return } - resultCh <- &pb.MikrotikResult{ + select { + case resultCh <- &pb.MikrotikResult{ DeviceId: job.DeviceId, JobId: job.JobId, Sentences: []*pb.MikrotikSentence{ {Attributes: map[string]string{"config": config}}, }, Timestamp: timestamp, + }: + default: + slog.Warn("result channel full", "job_id", job.JobId) } } diff --git a/ping.go b/ping.go index 9431f6b..8024cb1 100644 --- a/ping.go +++ b/ping.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "math/rand/v2" "net" "os" "os/exec" @@ -72,12 +73,13 @@ func doICMPPing(ip net.IP, network string, isIPv4 bool, timeoutMs int) (float64, } id := os.Getpid() & 0xffff + seq := int(rand.Uint32() & 0xffff) msg := icmp.Message{ Type: msgType, Code: 0, Body: &icmp.Echo{ ID: id, - Seq: 1, + Seq: seq, Data: []byte("towerops"), }, } @@ -121,7 +123,7 @@ func doICMPPing(ip net.IP, network string, isIPv4 bool, timeoutMs int) (float64, switch rm.Type { case ipv4.ICMPTypeEchoReply, ipv6.ICMPTypeEchoReply: echo, ok := rm.Body.(*icmp.Echo) - if !ok || echo.ID != id { + if !ok || echo.ID != id || echo.Seq != seq { continue } return float64(elapsed.Microseconds()) / 1000.0, nil diff --git a/update.go b/update.go index bbf4d32..e8a176b 100644 --- a/update.go +++ b/update.go @@ -34,7 +34,7 @@ func selfUpdate(downloadURL, expectedChecksum string) error { return fmt.Errorf("checksum required for update") } - slog.Info("downloading update", "url", downloadURL) + slog.Info("downloading update", "url", sanitizeURL(downloadURL)) resp, err := httpGet(downloadURL) if err != nil { diff --git a/websocket.go b/websocket.go index 18bb216..235cfcc 100644 --- a/websocket.go +++ b/websocket.go @@ -199,6 +199,7 @@ func (ws *WSConn) Close() error { } const wsReadTimeout = 90 * time.Second +const wsWriteTimeout = 30 * time.Second func (ws *WSConn) readFrame() (opcode int, payload []byte, err error) { // 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() defer ws.mu.Unlock() + if nc, ok := ws.conn.(net.Conn); ok { + _ = nc.SetWriteDeadline(time.Now().Add(wsWriteTimeout)) + } + length := len(payload) // Single buffer: max header (2+8+4=14) + payload buf := make([]byte, 0, 14+length)