309 lines
16 KiB
Plaintext
309 lines
16 KiB
Plaintext
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.
|