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

Break the md5 hash #35

Merged
merged 1 commit into from
Mar 20, 2013
Merged

Break the md5 hash #35

merged 1 commit into from
Mar 20, 2013

Conversation

andrew13
Copy link
Collaborator

Otherwise the md5 string can be decrypted and the application string is reveled. Which is a security concern.

Otherwise the md5 string can be decrypted and the application string is reveled. Which is a security concern.
@andrew13
Copy link
Collaborator Author

or don't use the app.key. Either way.

@ghost
Copy link

ghost commented Mar 12, 2013

Probably safest not to use the app.key at all. We really just need a random string here. Could be something like:

$this->confirmation_code = md5( uniqid(mt_rand(), true) )

which produces a 32-character token like c2683a623ecce511be79182e8b83637d

@andrew13
Copy link
Collaborator Author

agreed, there would need to be a check that the generated code is unique

@Zizaco
Copy link
Owner

Zizaco commented Mar 13, 2013

I got your points but microtime() is also unique and I believe it's almost impossible to break since the app.key is used as hash salt ( http://en.wikipedia.org/wiki/Salt_(cryptography) ). To break it will only be possible if the hacker knows the app.key.

What do you guys think?

@andrew13
Copy link
Collaborator Author

You don't need to know the app key, once you undo the md5 you can clearly see the microtime and app.key. MD5 is not a secure encryption mechanism. http://www.md5.net/encryption.php

I'd say we should use hash('sha256',microtime().static::$app['config']->get('app.key'))

@ghost
Copy link

ghost commented Mar 13, 2013

I strongly feel that the app's key should not be used here at all, even if it's hashed with something more secure than md5. It's just not necessary to use it as the seed. The app key is meant to be kept private since it's used by Laravel as the key for unencrypting data if you ever use Laravel's Crypt functions. So sending it out in an email to new users, even in hashed form, isn't secure because then we're relying on the security of the hash function when we don't even need to do that. We can just not use it as the seed.

All we need is a unique, unguessable string, right?

A UUIDv4 would work perfectly. (v4 is the random UUID)

Microtime isn't considered unique because theoretically it could be called twice at the same time on a machine or across multiple machines. That's why uniqid() (which is based off microtime) blocks for 1 microsecond to ensure uniqueness if it's not called with the true flag set for more entropy (which is why I used the true flag). But even then you have only ensured uniqueness on that machine. If you have more than one web server, the PHP manual recommends prefixing with the machine name to guarantee uniqueness across multiple machines. Since we can't know Confide users' machine names or that they're unique, I added mt_rand as the prefix for this. At that point you can be certain it's unique on any given machine and pretty darn certain that it's also unique across multiple machines. And it's unguessable because of the random prefix, it's not just time based. So md5( uniqid(mt_rand(), true) ) would work fine to create the token. ... But, all that said, a UUIDv4 is an even better solution, if you're okay including the function to generate it, because it's also unique even when two are generated at the same time on one or more machines, but has the added advantage of being even more secure/less guessable than uniqid() + mt_rand() because it's entirely random and not partially based on the time. So I think a UUIDv4 is the ideal solution to generating a unique token here.

Here's a function to generate a UUIDv4: http://stackoverflow.com/a/2040279 It works great. I've used it in the past. You can optionally leave out the dashes if you want it to be a bit shorter, then it'll be 32 characters just like md5's output.

Sorry for the long answer. Thoughts on UUIDv4?

@andrew13
Copy link
Collaborator Author

Preach on. Agreed with this being a perfect application of a UUIDv4 generated string.

@ghost
Copy link

ghost commented Mar 13, 2013

lol

@ghost
Copy link

ghost commented Mar 13, 2013

I posted my UUID class https://github.com/j20/php-uuid A token can be generated using

UUID::v4(false);

Zizaco added a commit that referenced this pull request Mar 20, 2013
@Zizaco Zizaco merged commit 52d01e1 into Zizaco:master Mar 20, 2013
@andrew13 andrew13 deleted the patch-3 branch March 20, 2013 15:55
@andrew13
Copy link
Collaborator Author

@j20 This is probably overkill but would you make the UUID a composer package or mine if I did? I'd like to use it in multiple projects and don't want to manage it individually per project.

Also @Zizaco I'll be submitting a pull request using UUID later this week.

@Zizaco
Copy link
Owner

Zizaco commented Mar 20, 2013

@andrew13, that would be nice :)

@ghost
Copy link

ghost commented Mar 21, 2013

@andrew13 Yeah. No problem. I'll get that reformatted for Packagist, probably tomorrow, and will drop a link here when it's done.

@andrew13
Copy link
Collaborator Author

@Zizaco Great, I'll send it over later this week.

@j20 Awesome, once that's done I'll add it to Confide.

@ghost
Copy link

ghost commented Mar 22, 2013

Got it working. Thanks for the help via email Andrew. https://github.com/j20/php-uuid https://packagist.org/packages/j20/php-uuid

@andrew13
Copy link
Collaborator Author

@j20 Awesome, I'll get the updates posted shortly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants