Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Randomize session cookie/ID #99

Merged
merged 1 commit into from
Jan 13, 2017
Merged

Conversation

dvlemplgk
Copy link
Contributor

Get a random cookie on OS supporting /dev/urandom instead of always the
same 0x78563412.

Get a random cookie on OS supporting /dev/urandom instead of always the
same 0x78563412.
@lategoodbye
Copy link
Contributor

Why can't we use portable C functions like rand() here?

@ch-aurich
Copy link
Contributor

Yes, I would welcome a portable version as well

@dvlemplgk
Copy link
Contributor Author

I thought about that, read the man page of rand and refrained from using it. I want a true random value here. Rand() must be seeded to give some reasonable randomness. Where to get a proper seed? Uptime is bad, time of day is bad (We're using open-plc-utils on OpenWrt where there's mostly no hw clock.). I have no idea how to use rand() properly since rand() was designed to give reproducible "randomness" and I definitly don't like this behavior here.
I could however use rand as a fallback when there's no /dev/urandom. But the seeding problem remains.

@dvlemplgk
Copy link
Contributor Author

I forgot to mention that rand only returns [0..RAND_MAX] and RAND_MAX = 2147483647 (glibc), that's only 31 bits.

@dvlemplgk
Copy link
Contributor Author

Can we agree on something here to move this pull request forward?

@mhei
Copy link
Contributor

mhei commented Jan 12, 2017

I vote for merging this PR as is: it improves the situation on "real OSes" 😃 and does not hurt on others...

@ch-aurich ch-aurich merged commit ff500f8 into qca:master Jan 13, 2017
@dvlemplgk dvlemplgk deleted the random-cookie branch January 16, 2017 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants