Fix random generators #210

at-wat · 2020-06-27T05:17:48Z

Seeding random generator each time limits number of generated sequence
to 31-bits, and causes collision on low time accuracy environments.
Use global random generator seeded by crypto grade random.

Reference issue

ref: #203

at-wat · 2020-06-27T05:35:49Z

util.go

@@ -12,6 +14,18 @@ import (
 	"github.com/pion/transport/vnet"
 )

+func init() { // nolint:gochecknoinits


To call this in the first use of random, any other call of randSeq and generateRandString should wait until calling Seed(), otherwise these parallel calls may return random generated by the default seed.
It makes code complex and may cause future bug, so I believe initializing in init() is the best solution.

at-wat · 2020-06-27T05:39:57Z

util.go

+		// crypto/rand is unavailable. Fallback to seed by time.
+		seed = time.Now().UnixNano()
+	}
+	rand.Seed(seed)


To avoid touching math/rand global generator, we need to declare package global random generator in this package. But in this case, we need to be careful that all code using random must use this generator. If math/rand global generator is used, it will be seeded by default value all time. (might be potentially dangerous)

Can we just not use crypto/rand instead? Not a big fan of init

codecov · 2020-06-27T05:40:19Z

Codecov Report

Merging #210 into master will increase coverage by 0.50%.
The diff coverage is 85.45%.

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   79.37%   79.88%   +0.50%     
==========================================
  Files          26       27       +1     
  Lines        2027     2048      +21     
==========================================
+ Hits         1609     1636      +27     
+ Misses        293      289       -4     
+ Partials      125      123       -2

Flag	Coverage Δ
#go	`79.88% <85.45%> (+0.50%)`	⬆️
#wasm	`18.55% <54.54%> (+0.94%)`	⬆️

Impacted Files	Coverage Δ
agent.go	`81.02% <55.55%> (+0.21%)`	⬆️
rand.go	`89.74% <89.74%> (ø)`
candidate_host.go	`77.77% <100.00%> (+4.44%)`	⬆️
candidate_peer_reflexive.go	`84.00% <100.00%> (+5.42%)`	⬆️
candidate_relay.go	`87.50% <100.00%> (+4.64%)`	⬆️
candidate_server_reflexive.go	`84.00% <100.00%> (+6.22%)`	⬆️
mdns.go	`70.83% <100.00%> (+1.26%)`	⬆️
util.go	`73.94% <100.00%> (-1.06%)`	⬇️
gather.go	`69.12% <0.00%> (-0.68%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47186b5...8e1986a. Read the comment docs.

Sean-Der · 2020-06-27T06:55:28Z

Would it be worth banning math/rand completely? Maybe a little heavy handed, but better safe then sorry. We can add it to travis checks, and would be really easy to get of it across every Pion project :)

at-wat · 2020-06-27T07:31:10Z

Would it be worth banning math/rand completely?

I don't think so. crypto/rand consumes hardware entropy pool and using too much affect all other crypto applications on the same instance.
Basically, we should select proper random generator for each usage.

In this code of unique ID generation, crypto/rand would be suitable, I think.

at-wat · 2020-06-27T08:15:04Z

Wait, randSeq is used to generate agent credentials. It must be crypto/random.

at-wat · 2020-06-27T08:23:20Z

For mDNS hostname,

https://tools.ietf.org/id/draft-ietf-rtcweb-mdns-ice-candidates-02.html#gathering

Generate a unique mDNS hostname. The unique name MUST consist of a version 4 UUID as defined in [RFC4122], followed by “.local”.

It must be UUIDv4 that is crypto grade 122bit random and contain UUID variant and version bits.
I think it's better to use UUID library like github.com/google/uuid which is designed and managed by crypto experts.

at-wat · 2020-06-27T08:41:53Z

We currently use UUID like random value for it, but it is defined as
https://tools.ietf.org/html/rfc5245#section-15.1

   candidate-attribute   = "candidate" ":" foundation SP component-id SP
                           transport SP
                           priority SP
                           connection-address SP     ;from RFC 4566
                           port         ;port from RFC 4566
                           SP cand-type
                           [SP rel-addr]
                           [SP rel-port]
                           *(SP extension-att-name SP
                                extension-att-value)

   foundation            = 1*32ice-char
   component-id          = 1*5DIGIT
   transport             = "UDP" / transport-extension
   transport-extension   = token              ; from RFC 3261
   priority              = 1*10DIGIT
   cand-type             = "typ" SP candidate-types
   candidate-types       = "host" / "srflx" / "prflx" / "relay" / token
   rel-addr              = "raddr" SP connection-address
   rel-port              = "rport" SP port
   extension-att-name    = byte-string    ;from RFC 4566
   extension-att-value   = byte-string
   ice-char              = ALPHA / DIGIT / "+" / "/"

Random part must be 1 to 32 characters of ALPHA, DIGIT, +, /. Using UUID is out of the standard.

ICE candidate ID is used in plain text in SDP. It doesn't need crypto grade random.

Sean-Der · 2020-06-27T08:46:43Z

I don't think so. crypto/rand consumes hardware entropy pool and using too much affect all other crypto applications on the same instance.

Looking through your list the only thing I see in the hot path is the VP8 code. If a user is really blocked on generation they can choose to use a weaker version.

I am worried that we will audit these now, but in the future this will get broken. I am not careful enough and with goimports sometimes I will swap between rand and math accidentally :/

I think it's better to use UUID library like github.com/google/uuid which is designed and managed by crypto experts.

Works for me! As long as it doesn't drag in a bunch of dependencies/is stable I agree! I would almost consider this 1st party, things on github.com/google probably aren't gonna get pulled/break like things under personal repos.

at-wat · 2020-06-27T09:52:09Z

https://developer.mozilla.org/en-US/docs/Web/API/RTCIceCandidate

usernameFragment

A DOMString containing a randomly-generated username fragment ("ice-ufrag") which ICE uses for message integrity along with a randomly-generated password ("ice-pwd"). You can use this string to verify generations of ICE generation; each generation of the same ICE process will use the same usernameFragment, even across ICE restarts.

https://developer.mozilla.org/en-US/docs/Web/API/RTCIceCandidate/usernameFragment

The string may be up to 256 characters long, and has no default value.

At least 24 bits of the text in the ufrag are required to be randomly selected by the ICE layer at the beginning of the ICE session.

Seeding random generator each time limits number of generated sequence to 31-bits, and caused collision. Use global random generator seeded by crypto grade random. Use crypto/rand for cryptographic values, and math/rand for unique identifier. - Use UUIDv4 for mDNS name - Use crypto/rand for ICE pwd and user fragment - Use properly seeded math/rand for UDP port, tie breaker and candidate ID

at-wat · 2020-06-27T12:00:32Z

I am worried that we will audit these now, but in the future this will get broken. I am not careful enough and with goimports sometimes I will swap between rand and math accidentally :/

Hmm, at the point I studied information science, random generators should be properly selected for each usage to keep both security and performance.
I'm not very sure current cryptographic random generator can replace all random generators. Please let me look into the latest situations.

For now, how about adding a rule to add comment to every math/rand import?

Sean-Der · 2020-06-27T21:49:57Z

For now, how about adding a rule to add comment to every math/rand import?

@at-wat Yea I like that! I think that is the perfect mix of helping us not make mistakes, but giving the flexibility to do what we need

at-wat force-pushed the fix-random-generator branch 2 times, most recently from dfefbea to 70b0d25 Compare June 27, 2020 05:35

at-wat commented Jun 27, 2020

View reviewed changes

at-wat force-pushed the fix-random-generator branch from 70b0d25 to 8e1986a Compare June 27, 2020 11:28

at-wat changed the title ~~Fix random generator~~ Fix random generators Jun 27, 2020

Sean-Der approved these changes Jun 27, 2020

View reviewed changes

at-wat mentioned this pull request Jun 28, 2020

Assert import "math/rand" has comment why it is used (ensure it is not used for crypto) pion/.goassets#32

Closed

at-wat merged commit e670416 into master Jun 28, 2020

at-wat deleted the fix-random-generator branch June 28, 2020 02:44

This was referenced Jul 13, 2020

Some tests fails on windows due generateRandString uses time.Now().UnixNano() #203

Closed

Backport bug fixes to v0 branch #243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix random generators #210

Fix random generators #210

at-wat commented Jun 27, 2020

at-wat Jun 27, 2020

at-wat Jun 27, 2020

Sean-Der Jun 27, 2020 •

edited

Loading

codecov bot commented Jun 27, 2020 •

edited

Loading

Sean-Der commented Jun 27, 2020 •

edited

Loading

at-wat commented Jun 27, 2020 •

edited

Loading

at-wat commented Jun 27, 2020

at-wat commented Jun 27, 2020

at-wat commented Jun 27, 2020

Sean-Der commented Jun 27, 2020 •

edited

Loading

at-wat commented Jun 27, 2020 •

edited

Loading

usernameFragment

at-wat commented Jun 27, 2020 •

edited

Loading

Sean-Der commented Jun 27, 2020

Fix random generators #210

Fix random generators #210

Conversation

at-wat commented Jun 27, 2020

Reference issue

at-wat Jun 27, 2020

Choose a reason for hiding this comment

at-wat Jun 27, 2020

Choose a reason for hiding this comment

Sean-Der Jun 27, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2020 • edited Loading

Codecov Report

Sean-Der commented Jun 27, 2020 • edited Loading

at-wat commented Jun 27, 2020 • edited Loading

at-wat commented Jun 27, 2020

at-wat commented Jun 27, 2020

at-wat commented Jun 27, 2020

Sean-Der commented Jun 27, 2020 • edited Loading

at-wat commented Jun 27, 2020 • edited Loading

usernameFragment

at-wat commented Jun 27, 2020 • edited Loading

Sean-Der commented Jun 27, 2020

Sean-Der Jun 27, 2020 •

edited

Loading

codecov bot commented Jun 27, 2020 •

edited

Loading

Sean-Der commented Jun 27, 2020 •

edited

Loading

at-wat commented Jun 27, 2020 •

edited

Loading

Sean-Der commented Jun 27, 2020 •

edited

Loading

at-wat commented Jun 27, 2020 •

edited

Loading

at-wat commented Jun 27, 2020 •

edited

Loading