Barnard audit findings ====================== This list was assembled from static review of the maintained Go sources, including Barnard, gumble, gumbleopenal, the local OpenAL binding, recording, file playback, UI, and protocol code. Generated protobuf output and the vendored Mumble C++ source were not treated as files to modify. Priority 0: security and crashers --------------------------------- [x] 1. UDP crypto secrets are logged Files: gumble/gumble/udp15.go, gumble/gumble/crypt.go Both crypto setup paths log the AES key and IV/nonce material at debug level. Anyone who obtains debug logs and a packet capture can decrypt voice traffic. Remove all secret material from logs. At most log lengths, setup success, and a non-secret connection identifier. [x] 2. Native UDP races TCP state mutation Files: gumble/gumble/udp15.go, gumble/gumble/handlers.go, gumble/gumble/audiolisteners.go The UDP reader runs independently of the TCP read routine. It reads Client.Users, User.decoder/audio sequence state, and audio-listener stream maps while TCP handlers add/remove users and close stream channels. This can cause data races, concurrent map read/write panics, or sends to a closed stream channel. Establish one synchronization regime: protect client user/channel state and audio stream registration with locks, and ensure channel close/send are serialized. Do not rely on the TCP read routine being serialized with UDP. [x] 3. Context actions panic on receipt and on trigger Files: gumble/gumble/client.go, gumble/gumble/handlers.go, gumble/gumble/contextaction.go Client.ContextActions is never initialized, so the first ContextActionModify_Add writes to a nil map. Further, newly created ContextAction values do not receive client = c, so Trigger methods dereference nil. Initialize the map in DialWithDialer and assign its owning client when actions are created. Add handler tests for add/remove/trigger. [x] 4. Unknown ChannelId deadlocks the protocol reader File: gumble/gumble/handlers.go In handleUserState, the unknown ChannelId branch takes c.volatile.Lock() again instead of unlocking before returning. This leaves the mutex locked forever. Replace with one unlock (prefer defer after acquisition) and add a malformed/out-of-order channel test. [x] 5. OpenAL Buffer.Delete deletes a source, not a buffer File: gumble/go-openal/openal/buffer.go Buffer.Delete calls C.walDeleteSource. It must call C.walDeleteBuffer. The current code reports invalid source errors and leaks OpenAL buffers. Add a binding test that creates and deletes a single buffer and checks openal.Err(). [x] 6. UI writes are concurrent and termbox is not protected Files: ui.go, client.go, gumble/gumbleopenal/stream.go, uiterm/*.go Network callbacks, audio capture error callbacks, and reconnect goroutines directly update Ui, Textview, Tree, Label, and termbox while Ui.Run updates the same state. These types have no locks and termbox calls are not safe from arbitrary goroutines. Route UI work through a UI-owned event queue, or protect all state and ensure only the UI goroutine calls termbox. [x] 7. Terminal control sequences from server data are rendered Files: client.go, ui.go, ui_tree.go, admin.go HTML escaping does not remove terminal escape/control sequences. Server supplied messages, names, comments, and channel names are displayed in the terminal and can contain ANSI/OSC controls. Sanitize for terminal display: remove/control-escape C0, DEL, ESC, and dangerous Unicode controls before rendering or notifying. Priority 1: transport, lifecycle, and correctness --------------------------------------------------- [x] 8. TCP audio is discarded before UDP is proven usable Files: gumble/gumble/crypt.go, gumble/gumble/client.go, gumble/gumble/udp.go udpActive is set immediately after CryptSetup. The TCP read routine then discards UDPTunnel packets even if inbound UDP is blocked or NAT setup has failed. Mark UDP active only after an authenticated UDP response/audio packet (or retain TCP until confirmed), and define fallback/recovery rules. [x] 9. Stream capture shutdown/startup races OpenAL File: gumble/gumbleopenal/stream.go StopSource closes a channel but does not wait for sourceRoutine. Destroy immediately closes the capture device, so the routine can access a closed device. A quick stop/start can also run two capture routines at once. Use a cancellation context plus WaitGroup/done channel; serialize Start, Stop, reopen, and Destroy; wait before CaptureCloseDevice. [x] 10. Renderer can be used after it is closed Files: gumble/gumbleopenal/stream.go, gumble/gumble/audiolisteners.go Existing OnAudioStream goroutines can run cleanup after Destroy closes renderCh. Their final render call then panics sending on a closed channel. Stop and join all audio stream goroutines before renderer shutdown; make render reject work after shutdown without panicking. [x] 11. Reconnect leaks the old audio stream File: client.go OnDisconnect starts reconnecting but never destroys the existing Stream or stops its file player/capture routine. connect creates a new Stream and overwrites b.Stream. Destroy/stop the old resources before reconnecting; make disconnect cleanup idempotent. [x] 12. File player sessions race each other File: fileplayback/player.go readFileAudio repeatedly reads mutable Player stopChan/ctx/audioChan. Stop can return and a new PlayFile can replace them while the old ffmpeg goroutine is still running. Old audio can enter the new session and old workers can survive. Put per-playback state in a session object with local context, stop channel, output channel, and WaitGroup. Stop must cancel and join that session before another begins. [x] 13. Recorder Stop races the encoder writer File: recording/recorder.go Stop closes stdin while run may be writing. A normal stop can therefore record a closed-pipe error and be reported as failed. Have run own stdin closure: signal stop, wait for run to finish/close stdin and Wait ffmpeg, then return its result. Do not close stdin concurrently from Stop. [x] 14. Tone-test startup leaks transmission on output-file error File: client.go connect starts StartToneGenerator and sets Tx before NewAudioFileSaver. If output file creation fails, the tone goroutine continues. Create the saver first, or close/wait for the tone generator and reset Tx on every failure. [x] 15. Gumble ffmpeg Pause can block forever File: gumble/gumbleffmpeg/stream.go Pause checks StatePlaying, releases the lock, then sends on an unbuffered pause channel. If process exits in between, no receiver remains. Redesign around context cancellation/state guarded by a mutex and a per-run done channel. Also synchronize Volume, which is currently read and written without protection. [x] 16. Audio listener/event listener detach is not concurrency-safe Files: gumble/gumble/listeners.go, gumble/gumble/audiolisteners.go Event listener detach has no lock; audio detach removes streams without closing/joining them. Concurrent attach/detach/delivery can corrupt linked lists or strand goroutines. Use mutex-protected listener snapshots and an idempotent detach operation. [x] 17. Notification commands block callers and substitution is unsafe File: main.go Notify sends to an unbuffered channel. The one consumer waits for each shell command, so slow notification programs block UI/network callbacks. Use a bounded queue and define dropping/backpressure behavior. Placeholder replacement is sequential: a user-controlled value containing a later placeholder can be re-expanded inside prior substituted text. Build argv without a shell where possible, or perform non-recursive token expansion in one pass. Priority 2: protocol and data correctness ------------------------------------------ [x] 18. UserStats FromServer fields are copied from FromClient File: gumble/gumble/handlers.go In handleUserStats, FromServer.Good is correct but Late/Lost/Resync read packet.FromClient. Use packet.FromServer for all four fields and add a regression test with differing values. [x] 19. Full channel link updates leave stale reverse links File: gumble/gumble/handlers.go A ChannelState Links replacement assigns a new channel.Links map but does not remove channel from the Links maps of old peers. Remove reciprocal old links before replacement and add link add/remove/full-replacement tests. [x] 20. Malformed protobuf fields can panic handlers File: gumble/gumble/handlers.go Several optional proto fields are dereferenced without validation, notably ACL group.Name and UserList_User.UserId. Validate required fields before dereferencing and return errInvalidProtobuf for malformed server packets. Audit all packet pointer dereferences similarly. [x] 21. UDP protocol state has no complete interoperability test coverage Files: gumble/gumble/udp15.go, gumble/gumble/udp.go Tests are mostly local encrypt/decrypt round trips. Add captured/reference vectors from current Mumble for CryptSetup, encrypted audio, ping, packet loss, IV wrap, late/replayed packets, protobuf and legacy envelopes, frame terminators, positional data, and volume adjustment. Test real UDP fallback behavior too. [x] 22. Opus bitrate calculation assumes a 10 ms interval File: gumble/opus/opus.go bitrate is maxDataBytes * 8 * 100. For permitted 20/40/60 ms intervals it is 2x/4x/6x too high. Calculate bits per frame divided by the actual Config.AudioInterval, or set the bitrate once when configuration changes. [x] 23. AudioInterval accepts invalid values File: gumble/gumble/config.go AudioFrameSize truncates arbitrary intervals to a count of 10 ms frames, while the ticker still uses the original interval. Validate and reject values other than 10/20/40/60 ms (and validate AudioDataBytes/Buffers). [x] 24. Legacy/custom varint has a MinInt64 recursion failure File: gumble/gumble/varint/write.go Encoding math.MinInt64 evaluates -value to the same negative number and recursively encodes forever until panic. Handle MinInt64 explicitly or encode negatives using an unsigned magnitude without overflow. Validate output buffer capacity in the exported encoder too. [x] 25. Mumble version layout documentation is wrong File: gumble/gumble/version.go The comment says major uses bits 0-15, but SemanticVersion and ClientVersion use bits 16-31. Correct the documentation and add known version tests. Priority 3: configuration, UI, and binding hardening ------------------------------------------------------ [x] 26. Persisted microphone volume is unused and zero is impossible Files: config/user_config.go, ui.go, gumble/gumbleopenal/stream.go MicVolume is stored but never applied when a Stream is created; UI changes do not call SaveConfig. GetMicVolume treats stored zero bits as uninitialized and returns 1.0, so mute cannot persist. Initialize the atomic value to 1.0 in New, apply configured volume during connect, allow zero, and save volume changes. [x] 27. Config address and file error handling can panic File: config/user_config.go makeHostPort splits on ':' and panics for malformed addresses or IPv6. fileExists dereferences info after non-ENOENT Stat failures. Replace with net.SplitHostPort (with explicit default-port policy) and return/report errors from Stat rather than dereferencing nil. [x] 28. Config SaveConfig panics and is not robust File: config/user_config.go Configuration write/rename errors panic the client. Return errors to the caller, preserve the prior config on failure, and consider fsyncing the temporary file/directory before rename. Avoid broad unrelated formatting changes while fixing this. [x] 29. FIFO reader spins after an error File: main.go setup_fifo ignores all ReadBytes errors and retries immediately. It also never closes the FIFO descriptor. Exit the reader on terminal errors, close the descriptor, and make shutdown cancellable. [x] 30. Empty UI tree can panic; UI startup errors are swallowed Files: uiterm/tree.go, uiterm/ui.go Tree.uiKeyEvent indexes lines[activeLine] for a non-arrow key even when no lines exist. Guard empty trees. Ui.Run returns nil when termbox.Init fails, hiding startup failure. Return that error. Also stop/join the PollEvent goroutine on UI shutdown and make Close nonblocking/idempotent. [x] 31. Text UI is not Unicode-safe and timestamp parsing is fragile Files: uiterm/textbox.go, uiterm/textview.go Textbox cursor positions are byte offsets but editing/display iterates runes, so non-ASCII input can be split into invalid UTF-8. Textview assumes every line contains ']' when timestamps are hidden and can panic otherwise. Track rune boundaries and use safe timestamp parsing/fallbacks. [x] 32. OpenAL binding needs API and unsafe hardening Files: gumble/go-openal/openal/*.go Many public slice APIs unconditionally use &slice[0] and panic for empty input (NewBuffers(0), Delete empty lists, SetData empty data, GetIntegerv size zero, etc.). Add guards or documented errors. go vet reports unsafe pointer misuse in alcCore.go; replace stored uintptr C handles with a vetted representation/pattern and re-run vet. Listener orientation uses a global tempSlice without synchronization; use a local fixed array. [x] 33. OpenAL errors are mostly ignored Files: gumble/gumbleopenal/stream.go, gumble/go-openal/openal/*.go Source/buffer/context/capture calls generally do not check AL/ALC errors, so invalid device/context/buffer operations become silent audio failure. Add checked wrapper operations for lifecycle-critical calls and surface actionable errors to Barnard. [x] 34. Beep helpers panic when the external command is absent Files: ui.go, gumble/gumbleopenal/stream.go Both helpers panic on exec failure. Return/log an error or remove unused helpers; a missing optional beep binary must not terminate Barnard. [x] 35. Admin manual-ban duration accepts negative values Files: admin.go, gumble/gumble/bans.go Negative minutes become a negative duration, then are cast to uint32 seconds for the protocol, creating a huge ban duration. Reject negative durations and validate mask/address before sending. [x] 36. Admin, tree, and client code read mutable maps outside Client.Do Files: admin.go, ui_tree.go, client.go UI code ranges Client.Users, Channels, Channel.Users, and Children while network handlers mutate them. This overlaps issue 2 but must be fixed on the UI side as well: take a safe snapshot under the client lock and render the snapshot outside the lock. Suggested repair order ---------------------- 1. Remove crypto secret logging; fix ContextActions initialization/client; fix the handler deadlock and OpenAL Buffer.Delete. 2. Define client/UDP/audio-stream locking and lifecycle ownership, then add race/integration tests for user removal, disconnect, reconnect, and UDP fallback. 3. Make UI updates single-threaded and sanitize terminal output. 4. Repair recording/file/capture session ownership and joining. 5. Fix protocol data errors, validation, Opus interval math, and config input. 6. Harden the OpenAL binding and remaining UI/config edge cases. Required verification after fixes -------------------------------- - gofmt only touched files. - go test ./... - go test -race ./... - go vet ./... with no remaining unsafe-pointer warnings. - Add focused unit tests for every deterministic bug above. - Add a local Mumble integration test or reproducible harness for TCP-only, UDP success, blocked inbound UDP fallback, reconnect, user removal during UDP audio, and native 1.5 UDP reference packets. - Manually test capture stop/start, disconnect/reconnect, file start/stop, recording stop, zero mic volume, Unicode/UI input, and no-beep/no-ffmpeg failure paths.