Repository navigation
Fix ICE ufrag/pwd collision from per-call time-seeded rand() - #288
Open
nguyenquocthaoolli wants to merge 1 commit into
Open
nguyenquocthaoolli wants to merge 1 commit into
nguyenquocthaoolli wants to merge 1 commit into
Conversation
utils_random_string() called srand(time(NULL)) on every invocation. Since time(NULL) has 1-second resolution and carries no per-process or per-device entropy, any two callers (different processes, different physical devices) that generate ICE credentials within the same UTC second produce byte-identical ufrag/pwd. RFC 8445 15.4 requires these values to be unguessable; a value fully determined by wall-clock second does not meet that, and colliding credentials also defeat ICE's demultiplexing when two agents share a listening port. Confirmed in production across an embedded device fleet using this library (via esp_peer): dozens of independent devices produced identical a=ice-ufrag/a=ice-pwd values, including overlapping sessions. Seed the PRNG once instead of per-call, using /dev/urandom where available (falling back to time+pid mixing otherwise) rather than time(NULL) alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #287
`utils_random_string()` called `srand(time(NULL))` on every invocation. Since `time(NULL)` has 1-second resolution and carries no per-process or per-device entropy, any two callers (different processes, different physical devices) generating ICE credentials within the same UTC second produced byte-identical `ufrag`/`ice-pwd`. RFC 8445 §15.4 requires these values to be unguessable; a value fully determined by wall-clock second doesn't meet that, and colliding credentials also defeat ICE's demultiplexing when two agents share a listening port.
Confirmed in production across an embedded device fleet using this library (via `esp_peer`): dozens of independent devices produced identical `a=ice-ufrag`/`a=ice-pwd` values, including overlapping sessions.
Change
Seed the PRNG once instead of per-call, using `/dev/urandom` where available (falling back to time+pid mixing otherwise) rather than `time(NULL)` alone.
Testing
`gcc -fsyntax-only -Wall -Wextra` on the changed file is clean. I didn't have a way to exercise the full ESP-IDF/embedded build from this environment, so please let me know if you'd like the embedded fallback path adjusted (e.g. to plumb in `esp_random()` on ESP-IDF targets via a weak/overridable hook) — happy to iterate.
🤖 Generated with Claude Code