diff --git a/fix.txt b/fix.txt new file mode 100644 index 0000000..a3a55c8 --- /dev/null +++ b/fix.txt @@ -0,0 +1,308 @@ +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 +--------------------------------- + +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. + +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. + +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. + +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. + +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(). + +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. + +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 +--------------------------------------------------- + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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 +------------------------------------------ + +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. + +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. + +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. + +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. + +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. + +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). + +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. + +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 +------------------------------------------------------ + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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. + +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.