Document Barnard audit findings

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.