Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Fix CVE-2015-5687, other security vulnerabilities (CVEs pending) #904

Closed
wants to merge 2 commits into from
Closed

Conversation

paragonie-scott
Copy link

  • Fix: PHP Object Injection in Session driver (CVE-2015-5687)
  • Fix: Weak random number generator being used for security-critical components (no CVE assigned yet)
  • Added: Security library with CSPRNG, hash_equals() polyfill, binary-safe string operations

@Radiergummi
Copy link
Member

Out of curiosity - why not use openssl_random_pseudo_bytes for generating random numbers?

Edit - my fault, missed the line.

@paragonie-scott
Copy link
Author

https://github.com/paragonie/random_compat/blob/master/ERRATA.md

It can fail silently. random_compat uses it as a last resort.

@paragonie-scott
Copy link
Author

Oops.

@paragonie-scott
Copy link
Author

@TheBrenny Have you had time to review this? We typically go public 30 days after disclosure or 24 hours after the fixed version is released.

@TheBrenny
Copy link
Member

@paragonie-scott, sadly, I'm not a part of the dev team, so for me to review this would be pointless. I merely just help them out with any of the bugs floating around, because they're super busy.

I want to point out, though, that @CraigChilds94 is getting round to merging things pretty soon, as you can see by the comment on my (accidentally closed because I deleted the branch) PR #884.

So I wouldn't count it as a fixed version release until this gets merged, and even then, because this is the 0.9-dev branch, I wouldn't count it as a version release. But hey, what's a version release is up to you, I guess! 😛

@paragonie-scott
Copy link
Author

A version release means your users can run a command e.g. composer update
and get the fixes.

I need to make one more commit when I get home in a couple hours.
On Jul 30, 2015 6:54 PM, "Jarod Brennfleck" notifications@github.com
wrote:

@paragonie-scott https://github.com/paragonie-scott, sadly, I'm not a
part of the dev team, so for me to review this would be pointless. I merely
just help them out with any of the bugs floating around, because they're
super busy.

I want to point out, though, that @CraigChilds94
https://github.com/CraigChilds94 is getting round to merging things
pretty soon, as you can see by the comment on my (accidentally closed
because I deleted the branch) PR #884
#884.

So I wouldn't count it as a fixed version release until this gets merged,
and even then, because this is the 0.9-dev branch, I wouldn't count it as a
version release. But hey, what's a version release is up to you, I guess! [image:
😛]


Reply to this email directly or view it on GitHub
#904 (comment).

@TheBrenny
Copy link
Member

All in your own time, don't stress. 😃

return Security::hashEquals(
crypt($value, $hash),
$hash
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these four lines be joined onto one? Not a life-or-death, just a better convention?

@paragonie-scott
Copy link
Author

Okay, I've updated the randomBytes() to be in line with random_compat and implemented the style suggestions.

@paragonie-scott
Copy link
Author

@CraigChilds94 Can you give an ETA on when you'll be able to look at this one? And also, when the next release for 0.9.x will be?

An advisory for these security issues will be sent out to Full Disclosure on August 27 at the latest.

@paragonie-scott
Copy link
Author

We're getting close to the 30 day mark. Please let me know if you need any more information on the changes this PR makes.

$message_compare = hash_hmac('sha256', $given, $blind);
$correct_compare = hash_hmac('sha256', $expected, $blind);
return $correct_compare === $message_compare;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why calculate hmacs on the args? is this a timing attack thing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. This is a timing attack mitigation strategy called "Double HMAC". By using a random nonce ($blind above), you eliminate any useful timing information from leaking out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively: Yes, this is a timing attack thing.

…o 0.9-dev

Conflicts:
	anchor/libraries/hash.php
	system/helpers.php
	system/session/drivers/cookie.php
@paragonie-scott paragonie-scott deleted the 0.9-dev branch August 27, 2015 17:03
@paragonie-scott
Copy link
Author

Looks like this was fixed by deleting the driver and using RandomLib. That's acceptable as well.

@kierwils
Copy link
Member

Thanks for pointing use in the right direction, the cookie driver should have been removed ages ago it was never used but was far from secure.

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

Successfully merging this pull request may close these issues.

5 participants