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

Some kernels have frandom and erandom #367

Closed
wants to merge 1 commit into from
Closed

Some kernels have frandom and erandom #367

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 26, 2013

When a users kernel has frandom and erandom this would cause the whole app to crash.

When your kernel supports this frandom will replace random and erandom will replace urandom. The advantage here is that it's faster and that erandom doesn't use up any entropy at all. This helps to remove jank from Android.

Without this patch many people running custom ROM's can't use K-9 Mail due to a force close.

When a users kernel has frandom and erandom this would cause the whole app to crash.

When using erandom the system doesn't use any entropy at all.
@cketti
Copy link
Member

cketti commented Aug 26, 2013

Do you have more information on what kernels/devices/ROMs are affected?

I don't like the idea of using a known bad PRNG. If we can't fix it, we shouldn't allow the user to use K-9 Mail (although an error message would be nicer than a crash).

@ghost
Copy link
Author

ghost commented Aug 26, 2013

Sure, just google "xda frandom kernel". Basically any custom ROM that has frandom can't use K-9 Mail right now as it force closes.

Users with erandom aren't effected by bad PRNG since it works differently.

http://forum.xda-developers.com/showpost.php?p=37409586&postcount=134
http://forum.xda-developers.com/showthread.php?t=2113150
https://github.com/Ryuinferno/frandom-android

@obra
Copy link
Contributor

obra commented Aug 26, 2013

On Mon, Aug 26, 2013 at 10:17:46AM -0700, cketti wrote:

I don't like the idea of using a known bad PRNG. If we can't fix it, we shouldn't allow the user to use K-9 Mail (although an error message would be nicer than a crash).

Disallowing users to use the app is really not the best solution. They're just going to use another email app that isn't any better and be angry with us. A warning popup on account configuration isn't unreasonable, though.

@ghost
Copy link
Author

ghost commented Aug 26, 2013

You could add a warning, but there isn't much point to it since erandom shouldn't be effected by bad PRNG. Of course you could add one just to say "hey, if something messes up then it's your fault for using a custom ROM".

"Both frandom and erandom generate random data very fast, with the algorithm that is used in the alledged RC4 cipher."

"frandom calls the kernel's random generator for 256 bytes of random data while erandom uses an internal random generator to generate its own 256 bytes of seed. This internal random generator is exactly like the one used to generate random data for each /dev/{f,e}random stream. It is seeded when the module is initialized, using the kernel's random generator."

"By using /dev/erandom the kernel's random entropy is left untouched, so random data is generated without interfering with other applications that may need kernel entropy."

So basically this patch will help everyone on stock ROM's and for them using custom it'll let them go on their merry way.

@cketti
Copy link
Member

cketti commented Aug 26, 2013

"Bottom line: It's probably OK to use frandom for crypto purposes. But don't. It wasn't the intention."
-- http://www.billauer.co.il/frandom.html

That's not the issue, though. The PRNG problem wasn't caused by any of /dev/*random. /dev/urandom is simply used to properly seed SecureRandom instances in the fix. As far as I can tell, kernels that include /dev/{f,e}random should work just fine with the fix, because there's no need to remove /dev/urandom to use /dev/{f,e}random.
From the patch in this pull request it looks like writing to /dev/urandom fails... which is odd. But since /dev/urandom is a global PRNG I think the PRNGFixes class not being able to add bytes to increase the /dev/urandom entropy shouldn't cause problems.

@blackbox87: Can you confirm that reading from /dev/urandom works fine?

@ghost
Copy link
Author

ghost commented Aug 26, 2013

I can confirm that reading from it works fine, but urandom isn't really urandom as an init.d script is setup to link to erandom instead.

if [ -e /dev/frandom ]; then chmod 666 /dev/frandom; chmod 666 /dev/erandom; rm /dev/random && ln /dev/frandom /dev/random; chmod 666 /dev/random; mv /dev/urandom /dev/urandom.ORIG && ln /dev/erandom /dev/urandom; chmod 666 /dev/urandom; fi;

If you wanted access to the real urandom then that's still there, but it's named urandom.ORIG instead. It's not removed because keystore uses it.

@cketti
Copy link
Member

cketti commented Aug 26, 2013

@blackbox87: Thanks. I guess that explains why writing to it fails.

I think it's a bad idea to link /dev/urandom to /dev/{e,f}random. But if users of those custom ROMs are willing to accept the risk, that's fine with me.

Considering all this, I'm +1 for merging. A comment explaining why writing to /dev/urandom could fail, while reading does not, should be added.

@zjw
Copy link
Contributor

zjw commented Aug 26, 2013

Interesting that they "chmod 666". The devices don't appear to be writable, so that would likely cause IOExceptions:

[https://github.com/Ryuinferno/frandom-android/blob/master/frandom.c#L284]

@ghost
Copy link
Author

ghost commented Aug 26, 2013

I'll be changing the permission myself since it's obviously only readable.

I guess in normal situations most apps don't ever write to urandom, but when the bad PRNG fix was added to K9 it would cause the IOException.

@obra
Copy link
Contributor

obra commented Aug 27, 2013

I've added some notes about the reason for ignoring the exception and merged this

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.

None yet

3 participants