Skip to content

Remove rand() fallback#3

Merged
karenetheridge merged 1 commit intokarenetheridge:masterfrom
nothingmuch:no_rand
Apr 8, 2018
Merged

Remove rand() fallback#3
karenetheridge merged 1 commit intokarenetheridge:masterfrom
nothingmuch:no_rand

Conversation

@nothingmuch
Copy link
Copy Markdown

This may have some justification for testing purposes, i.e. to get
determinism, but in code relying on cryptography this is unlikely to be
required, and I believe in that kind of circumstance it can be
reimplemented in the test without too much difficulty, both clarifying
the intent and avoiding any accidental use of this dangerous fallback in
production.

@nothingmuch
Copy link
Copy Markdown
Author

I'm not sure what the best way to document this is.

Previously, random data could have been created in circumstances where get_weak was used and no urandom device was available. I think Win32 at risk of being affected if the corresponding module was not installed.

This also presents a misconception, /dev/urandom is not actually "weak" in practice, except in very specific circumstances. Though technically it may produce non random data (e.g. on a headless server with no stored entropy) but I think the current nomenclature is still misleading, for reasons relating to why this code should be removed.

@nothingmuch
Copy link
Copy Markdown
Author

to save any reviewers time following links, travis check failed because I didn't port this to ruby

@karenetheridge
Copy link
Copy Markdown
Owner

karenetheridge commented Apr 7, 2018

to save any reviewers time following links, travis check failed because I didn't port this to ruby

lol, I turned on travis for all my repos, but I only bother to add a config file when it fails for the first time, to remind me that the repo still exists :)

@nothingmuch
Copy link
Copy Markdown
Author

ah that just shows how out of date I am, i assumed this was still part of their "HI LET ME REPORT FAIL FOR ALL YOUR PROJECTS" thing years back, which led me to just assume it is never a meaningful signal

@karenetheridge
Copy link
Copy Markdown
Owner

karenetheridge commented Apr 7, 2018

I've pushed a (working) travis config to the master branch. If you rebase and force-push, I think you'll find that tests will still fail here, but for a much more logical reason (nope, I'm not giving you a hint) :)

This may have some justification for testing purposes, i.e. to get
determinism, but in code relying on cryptography this is unlikely to be
required, and I believe in that kind of circumstance it can be
reimplemented in the test without too much difficulty, both clarifying
the intent and avoiding any accidental use of this dangerous fallback in
production.
@nothingmuch
Copy link
Copy Markdown
Author

nothingmuch commented Apr 7, 2018

Thanks! I think it's passing now (5.22-6 so far), so I'm going to guess it's that I forgot to git add Changes and failed the author tests which I neglected to run locally

@karenetheridge karenetheridge merged commit a8d581a into karenetheridge:master Apr 8, 2018
@nothingmuch
Copy link
Copy Markdown
Author

thanks! i will do another PR tomorrow covering the doc improvements, if i can make up my mind on how to explain the whole urandom thing without being even more confusing than the the urandom manpage that caused me to mislead people like that in the first place

@nothingmuch
Copy link
Copy Markdown
Author

derp, i forgot to remove ::rand from Factory.pm, see #4

@carnil
Copy link
Copy Markdown

carnil commented Dec 29, 2024

CVE-2018-25107 has been assigned for this issue.

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.

3 participants