alobbs / macchanger

GNU MAC Changer
http://www.gnu.org/software/macchanger
GNU General Public License v3.0
588 stars 112 forks source link

prefer /dev/random over /dev/urandom #5

Closed ZeroChaos- closed 10 years ago

ZeroChaos- commented 10 years ago

I'm not certain this is a huge deal, but it would seem obvious to me that you would want to open /dev/random first, and open /dev/urandom only if that fails, but you seem to do the opposite.

Was there a major reason for this? I doubt this is a critical issue but I would certainly have preference for the higher entropy first.

alobbs commented 10 years ago

https://github.com/alobbs/macchanger/commit/99ea3798680459fe8f5259748f2b8f917f1c952e fixed it. Thanks for reporting!

ZeroChaos- commented 10 years ago

very nice. thanks!

thoger commented 10 years ago

@ZeroChaos what would be the reason to prefer random over urandom? There seems to be reasons to prefer urandom when available:

ZeroChaos- commented 10 years ago

I've never seen random block unless I was trying to write it to fill a disk drive or something else stupid. It's pretty unlikely with the small amount macchanger needs to read that it wouldn't have enough randomness to fill it. I suppose the check could be changed to make sure random isn't blocking when you try to read however many bytes....

thoger commented 10 years ago

Reading from random can block. Actually, trying to run patched macchanger -r in a loop shows that it gets stuck surprisingly quickly on my system.

Why using urandom seems not good enough?

ZeroChaos- commented 10 years ago

if this is already causing issues then I'm fine with going back. I run a hardened system so it maintains higher entropy and I don't notice this issue. I'm okay with hwrng first, then urandom then random if it blocks "surprisingly quickly"

alobbs commented 10 years ago

Yes, /dev/urandom should be good enough, actually. Running macchanger so fast so many times as to exhaust /dev/random's buffer/pool is a really corner case that I don't think it's worth covering though.

I don't have any strong feeling about it. At the end of the day, it's reading just 32bits for the srandom(). Either way it'd work just fine. It isn't like we were generating a crypto key here.

ZeroChaos- commented 10 years ago

I agree it's a corner case, but like you said we aren't doing crypto here. If there is a hwrng we can use that, then urandom, then random. In my opinion, if there are already issues, then this order should be fine. (but damn that's a lot of mac changing).

thoger commented 10 years ago

For posterity, random/urandom reading to seed random() was added in 596056e. It replaced seeding with srandom(time(NULL));, which caused issues when users tried to run macchanger more than once per second.