paragonie/random_compat 1.* uses insecure CSPRNG (openssl_random_pseudo_bytes())

[AndrewCarterUK]

paragonie/random_compat#96

TL;DR - In environments where the PHP process is forked, openssl_random_psuedo_bytes can generate the same stream of values.

There are other security issues that also caused @paragonie-scott concern.

Also I'm not sure what to do with the branch name, the vulnerability spans more than one branch and can best be described as "all versions 1.*? (edit - as @stof mentioned it's actually all versions <2 )

[stof]

Also I'm not sure what to do with the branch name, the vulnerability spans more than one branch and can best be described as "all versions 1.*"?

the branch name has no meaning for the vulnerability database. It is here for readability of the YAML file. Affected versions are handled by constraints.

[stof]

0.9 versions are also affected. so a better constraint would be <2.0

[Ocramius]

Hey @AndrewCarterUK, since this was reported as security issue here, should ramsey/uuid also be added? (Same problem)

[AndrewCarterUK]

@Ocramius - I'm not sure on this one. I'm not sure where to draw the line between a bug and a security issue.

ping @ramsey - would be interested to hear your opinion?

My opinion:

Technically, UUIDs have no requirement to be cryptographically secure, and I can't see any promise of that on the repository page. However, could the fact that it produces collisions be abused? Possibly. Could most bugs potentially be abused? Probably

[GrahamCampbell]

I was of the understand this was NOT a SECURITY bug, but was merely an OCD fix to be on the "safe side".

[GrahamCampbell]

That's why this was a 2.x release rather than 1.x.

[paragonie-scott]

@GrahamCampbell is correct. It was a breaking change made for people who distrusted openssl_random_pseudo_bytes() motivated by a handful of cases where people were burned by it.

The difference between version 1 and 2 is: When no other CSPRNGs are available, version 1 falls back to openssl, version 2 throws an Exception. Most non-Windows servers will never experience this error. Windows users can use version 1 safely by ensuring they installed ext/mcrypt (for CryptGenRandom).

Encouraging people to use version 2 instead of version 1 is safer but using version 1 isn't inherently unsafe.

[AndrewCarterUK]

@GrahamCampbell @paragonie-scott - If, in certain environments, forked processes that wrap to have the same PID as a previous one produce the same data, then the output is predictable and thus not cryptographically secure.

(edit: removed explanation as you obviously know what it does)

I understand that this is only a problem in certain scenarios, but "only a security issue in certain circumstances" is still a security issue.

[paragonie-scott]

Right. Version 2 is always secure (or it fails loudly), version 1 is secure unless you're running an unreliable configuration (i.e. PHP 5 on Windows without ext/mcrypt or Linux with restrictive open_basedir).

That does make it a security issue. That does not make it a security issue that affects everyone who uses version 1. I was just trying to make sure the scope was clarified.

[stof]

then, it would be good to have a place describing exactly the issue, and have this as the link in the advisory instead of linking to the original github issue (which will confuse people).
@paragonie-scott creating a blog post explaining in which case version 1 is insecure would be a good way to handle this IMO.

[AndrewCarterUK]

@paragonie-scott - agreed, the next question is should affected versions of ramsey/uuid also be added?

I'm leaning towards no, as UUID's don't have to be crypto strong, and I don't think this library advertises them like this.

[Ocramius]

A duplicate UUID can cause a mess, fwiw.
On Apr 26, 2016 16:08, "Andrew Carter" [email protected] wrote:

@paragonie-scott https://github.com/paragonie-scott - agreed, the next
question is should affected versions of ramsey/uuid also be added?

I'm leaning towards no, as UUID's don't have to be crypto strong, and I
don't think this library advertises them like this.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#146 (comment)

[GrahamCampbell]

A duplicate UUID can cause a mess, fwiw.

That doesn't make it a security advisory though?

[Ocramius]

Yeah, that makes it a security advisory. Generating duplicate data on
purpose or breaking someone else's transactions is an exploit ;-)
On Apr 26, 2016 16:22, "Graham Campbell" [email protected] wrote:

A duplicate UUID can cause a mess, fwiw.

That doesn't make it a security advisory though?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#146 (comment)

[ramsey]

Section 4.4 of RFC 4122 (UUID) says:

The version 4 UUID is meant for generating UUIDs from truly-random or pseudo-random numbers.

This implies that a CSPRNG should be used, but they leave it ambiguous since they don't use the words SHOULD or MUST.

Section 6 say:

Do not assume that UUIDs are hard to guess; they should not be used as security capabilities (identifiers whose mere possession grants access), for example. A predictable random number source will exacerbate the situation.

This seems to imply that CSPRNGs aren't necessary.

I will point out that RFC 4122 does not reference RFC 2119 ("Key words for use in RFCs to Indicate Requirement Levels") as most other RFCs do.

[cebe]

That does make it a security issue. That does not make it a security issue that affects everyone who uses version 1. I was just trying to make sure the scope was clarified.

it is mainly a security issue of the setup instead of the library itself...

The linked information in the advisory should
have detailed information on who is actually affected by this.

[ramsey]

Maybe the security advisory needs to be for openssl_random_psuedo_bytes() in environments where the PHP process is forked and this is relied upon as a CSPRNG.

[fabpot]

What do we do here?

[Ocramius]

Doesn't seem like the world exploded, and people surely had time to upgrade by now.

My suggestion:

• Merge this
• Due to environment misconfiguration not being detected/useful in this repository, ignore the underlying issue about the used function, as we can't put a version constraint on it

[paragonie-scott]

I generally recommend libraries set their requirement string to ^1|^2 and applications set it to ^2. Never ^1.

If people use the Roave package to avoid security pitfalls, opting for version 2 makes sense. I'd say merge it, even if it's not a game over issue.