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

Use better methods for generating entropy #6

Merged
merged 3 commits into from Aug 22, 2013

Conversation

Projects
None yet
4 participants
@ctso
Contributor

ctso commented Aug 20, 2013

The jsbn libraries are a bit old, last modified in 2009 to be exact. Crypto in the browser has come a long way since then, and I generally feel that the way it generates entropy could be greatly improved.

Using time to generate random values is well, not random. Math.random() is not as good as using crypto.getRandomValues, and while the RNG included in jsbn does attempt to use crypto.random, that API seems to be very old and not supported on modern browsers.

Up to you if you would like to accept this, as you state that the jsbn files have been copied over exactly. Not sure what your stance is on modifying them. This pull request does a few things:

  • Prefers crypto.getRandomValues
  • Falls back to collecting entropy via mouse movement.
  • When the PRNG (Arcfour) is initiated, if there is not enough entropy from getRandomValues or mouse movement, it falls back to Math.random.

I am currently using these changes in a live system (http://www.cyanogenmod.org/blog/cyanogenmod-account) that has a lot of "tinfoil hat" users, and would love to see them upstream :)

ctso added some commits Aug 20, 2013

Use crypto.getRandomValues for RNG if available
window.crypto.random is really old and depreciated.  Should use
crypto.getRandomValues instead.  If we are able to generate entropy from
crypto.getRandomValues, we should not add entropy from Math.random, as
it may be unreliable.
Use better entropy generation methods
Using time to generate entropy is not optimal, as time is predictible
and not random.  This change prefers crypto.getRandomValues, falls back
to using mouse movement, and ultimately if enough entropy is not
generated by the time we need a key, it will fall back to Math.random.
Initialize Uint32Array before getRandomValues
This seems to cause PhantomJS to fail, as getRandomValues does not
return the array.
@travist

This comment has been minimized.

Show comment
Hide comment
@travist

travist Aug 21, 2013

Owner

I love this idea... My only issue with it is that it is modifying the jsbn library when I would prefer not to fork that library. It does seem, however, that we may be able to duck punch these methods within the jsencrypt.js file so we achieve the same result without modifying the core library.

Thoughts?

Owner

travist commented Aug 21, 2013

I love this idea... My only issue with it is that it is modifying the jsbn library when I would prefer not to fork that library. It does seem, however, that we may be able to duck punch these methods within the jsencrypt.js file so we achieve the same result without modifying the core library.

Thoughts?

@travist

This comment has been minimized.

Show comment
Hide comment
@travist

travist Aug 21, 2013

Owner

But... then again... duck punching is pretty sloppy and almost more so than hacking a core library. I really do wish that Tom Wu would manage his repo and accept upstream changes since that would be the ultimate solution. What I may propose is that these changes do get accepted with an included *.patch file in the jsbn directory so that anyone is aware of the changes that have been made to it.

Owner

travist commented Aug 21, 2013

But... then again... duck punching is pretty sloppy and almost more so than hacking a core library. I really do wish that Tom Wu would manage his repo and accept upstream changes since that would be the ultimate solution. What I may propose is that these changes do get accepted with an included *.patch file in the jsbn directory so that anyone is aware of the changes that have been made to it.

@ctso

This comment has been minimized.

Show comment
Hide comment
@ctso

ctso Aug 21, 2013

Contributor

Sure, duck punching these methods would work, but I agree that it is dirty. If we are duck punching, we might as well just modify the code, the result is the same.

I'm fine with including *.patch files, however, I don't really feel that that is necessary either. This is a git repo after all, if you want to see the changes that is trivial. I suggest we simply update the README with links to the changes instead of including patch files, but it doesn't really matter either way.

Let me know how you would like to move forward.

Contributor

ctso commented Aug 21, 2013

Sure, duck punching these methods would work, but I agree that it is dirty. If we are duck punching, we might as well just modify the code, the result is the same.

I'm fine with including *.patch files, however, I don't really feel that that is necessary either. This is a git repo after all, if you want to see the changes that is trivial. I suggest we simply update the README with links to the changes instead of including patch files, but it doesn't really matter either way.

Let me know how you would like to move forward.

@travist

This comment has been minimized.

Show comment
Hide comment
@travist

travist Aug 22, 2013

Owner

Yeah, I am ok with adding the commit to the README. That way I can reference it in my blog post about this library where I say that library is "untouched". I will go ahead and pull this in and then update the readme so we have a commit to reference.

Owner

travist commented Aug 22, 2013

Yeah, I am ok with adding the commit to the README. That way I can reference it in my blog post about this library where I say that library is "untouched". I will go ahead and pull this in and then update the readme so we have a commit to reference.

travist added a commit that referenced this pull request Aug 22, 2013

Merge pull request #6 from ctso/master
Use better methods for generating entropy

@travist travist merged commit 8d640a7 into travist:master Aug 22, 2013

@koush

This comment has been minimized.

Show comment
Hide comment
@koush

koush Aug 22, 2013

The mouse coordinate stuff ought to be run through a whitening function.

koush commented Aug 22, 2013

The mouse coordinate stuff ought to be run through a whitening function.

@travist

This comment has been minimized.

Show comment
Hide comment
@travist

travist Aug 22, 2013

Owner

Submit a pull request and I will evaluate it... thanks for the suggestion.

Owner

travist commented Aug 22, 2013

Submit a pull request and I will evaluate it... thanks for the suggestion.

@zoloft

This comment has been minimized.

Show comment
Hide comment
@zoloft

zoloft Sep 2, 2013

Collaborator

Hi there.

First of all great work @ctso.
just two comments:

  • Opera does not implement window.crypto BUT its Math.random is cryptographically secure, so we can make a polyfill from it to cover the lack of crypto.getRandomValues
  • For devices that doesn't have a mouse as input device (e.g. mobile devices) we can use (where possible) gyroscope and accellerometer for entropy.

Probably i will make this modifications next weekend, if you can wait i will make a PR.

Antonio

Collaborator

zoloft commented Sep 2, 2013

Hi there.

First of all great work @ctso.
just two comments:

  • Opera does not implement window.crypto BUT its Math.random is cryptographically secure, so we can make a polyfill from it to cover the lack of crypto.getRandomValues
  • For devices that doesn't have a mouse as input device (e.g. mobile devices) we can use (where possible) gyroscope and accellerometer for entropy.

Probably i will make this modifications next weekend, if you can wait i will make a PR.

Antonio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment