Page MenuHomePhabricator

security review of ramsey/uuid
Closed, DeclinedPublic

Description

mediawiki/extensions/EventBus has a dependency on ramsey/uuid for the generation of type 1 UUIDs (we do not seem to have anything to do this); Per https://www.mediawiki.org/wiki/Manual:External_libraries, I'm requesting a security review of ramsey/uuid.

See also:

Event Timeline

Eevans created this task.Dec 2 2015, 9:38 PM
Eevans raised the priority of this task from to Needs Triage.
Eevans updated the task description. (Show Details)
Eevans added subscribers: Eevans, csteipp.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptDec 2 2015, 9:38 PM

Thanks @Eevans, we'll try to get this done as soon as we can

csteipp triaged this task as High priority.Dec 3 2015, 1:12 AM
csteipp added a project: Security-Team-Reviews.
csteipp set Security to None.
csteipp moved this task from Backlog to Scheduled on the Security-Team-Reviews board.
csteipp moved this task from Backlog to Ready on the Security-Team board.
Dereckson added a subscriber: Dereckson.EditedDec 3 2015, 1:24 AM

Not a thorough review, but some notes of concern suggesting we should perhaps use the version 3 of the library, and so review that instead of 2.8.3.

Security notes

The Uuid::compareTo method doesn't seem to run in constant time, and so shouldn't be used to compare anything where a timing attack would be a concern.

Other notes

  • The application requires as a -dev dependency \Moontoast\Math but uses it in main code (Uuid::getDateTime).
  • MediaWiki isn't a 64-bits ony application, and so any call to Uuid::getTimeLow, Uuid::getFields or Uuid::getNode methods should be avoided.

These two last concerns are mitigated in the current master tree, so it only apply to 2.8.3, not version 3 of the library.

Thank you @csteipp and @Dereckson for the promptly response! @Eevans, any reason not to switch to ver 3 of the lib?

Legoktm added a subscriber: Legoktm.Dec 3 2015, 8:52 AM

MediaWiki core has a UIDGenerator class that creates v4 ids, is there a reason that won't work? (I'm totally unfamiliar between the difference between v1 and v4)

MediaWiki core has a UIDGenerator class that creates v4 ids, is there a reason that won't work?

The reason for importing this lib is that we need both v1 and v4 UUIDs in the extension emitting events.

(I'm totally unfamiliar between the difference between v1 and v4)

v4 UUIDs are time UUIDs (i.e. UUIDs based on the current time stamp) and are therefore not unique. v1 UUIDs, on the other hand, guarantee uniqueness across processes and machines and we use it for setting event IDs in order to be able to de-duplicate them.

hashar added a subscriber: hashar.Dec 3 2015, 9:02 AM

And on the EventBus change https://gerrit.wikimedia.org/r/#/c/254086/11/composer.json

ramsey/uuid" claims to provide "RFC 4122 version 1, 3, 4, and 5 UUID"
MediaWiki core already has such support embedded via includes/utils/UIDGenerator.php
So I would get rid of that ramsey/uuid dependency and reuse MediaWiki core built-in class.
The only usage being for a v1 UUID (based on time/ mac address) maybe our getTimestampedID128 is good enough for that purpose :-}

ramsey/uuid is being proposed for mediawiki/vendor with https://gerrit.wikimedia.org/r/#/c/256575/ I commented there:

I am not opposed to introducing this dependency, but I am wondering whether we could add UUID v1 in core or replace our home made generator with ramsey/uuid :-)

So again. I am not opposed to ramsey/uuid. But I find it annoying we will end up with duplicate code between core and the 3rd party lib.

So again. I am not opposed to ramsey/uuid. But I find it annoying we will end up with duplicate code between core and the 3rd party lib.

+1, I sympathise. I'd be perhaps more open to the latter. UUID generation is tricky, so I'd rather have it maintained elsewhere.

Eevans added a subscriber: aaron.EditedDec 3 2015, 3:30 PM

Not a thorough review, but some notes of concern suggesting we should perhaps use the version 3 of the library, and so review that instead of 2.8.3.

Unfortunately, version 3 is PHP v5.4+.

MediaWiki core has a UIDGenerator class that creates v4 ids, is there a reason that won't work?

The reason for importing this lib is that we need both v1 and v4 UUIDs in the extension emitting events.

(I'm totally unfamiliar between the difference between v1 and v4)

v4 UUIDs are time UUIDs (i.e. UUIDs based on the current time stamp) and are therefore not unique. v1 UUIDs, on the other hand, guarantee uniqueness across processes and machines and we use it for setting event IDs in order to be able to de-duplicate them.

Actually, that's backward, if I understand you. Version 1 UUIDs are based on a timestamp (100 nano-second intervals from an epoch based on the adoption of the Gregorian calendar), and a "node ID" (typically the machine's mac address). Version 4 UUIDs are based entirely on (pseudo)random numbers. Version 4 is a successor to version 1, the biggest complaint with v1 being that they are not sufficiently opaque, (which incidentally, is exactly why they are so convenient, assuming you aren't concerned about the security implications of leaking the when and where of generated IDs.

https://en.wikipedia.org/wiki/Universally_unique_identifier#Variants_and_versions

And on the EventBus change https://gerrit.wikimedia.org/r/#/c/254086/11/composer.json

ramsey/uuid" claims to provide "RFC 4122 version 1, 3, 4, and 5 UUID"
MediaWiki core already has such support embedded via includes/utils/UIDGenerator.php
So I would get rid of that ramsey/uuid dependency and reuse MediaWiki core built-in class.
The only usage being for a v1 UUID (based on time/ mac address) maybe our getTimestampedID128 is good enough for that purpose :-}

ramsey/uuid is being proposed for mediawiki/vendor with https://gerrit.wikimedia.org/r/#/c/256575/ I commented there:

I am not opposed to introducing this dependency, but I am wondering whether we could add UUID v1 in core or replace our home made generator with ramsey/uuid :-)

So again. I am not opposed to ramsey/uuid. But I find it annoying we will end up with duplicate code between core and the 3rd party lib.

I agree. And there is this (authored by @aaron; previously abandoned), which would introduce v1 generation into UIDGenerator. It is not uncontroversial either though, I'm afraid.


I am more and more convinced (as already suggested here), that we should eliminate the extension's dependency on ramsey/uuid (at least for now), and instead generated a v4 UUID (which UIDGenerator is capable of doing). The only drawback that I am aware of, is that we would be unable to extract a timestamp from the ID, which should not be an issue since there is a separate attribute in each event that contains an identical ISO8601 timestamp.

If it is determined that we absolutely need a v1 UUID (despite the redundant timestamp), then we can take up the issue of an external dependency vs. @aaron's UIDGenerator changeset (or similar) at a later date.

csteipp moved this task from Ready to Waiting on the Security-Team board.Dec 3 2015, 7:24 PM
Eevans added a comment.Dec 4 2015, 1:19 AM

I am more and more convinced (as already suggested here), that we should eliminate the extension's dependency on ramsey/uuid (at least for now), and instead generated a v4 UUID (which UIDGenerator is capable of doing). The only drawback that I am aware of, is that we would be unable to extract a timestamp from the ID, which should not be an issue since there is a separate attribute in each event that contains an identical ISO8601 timestamp.

Just an update here; As of patch 23, I have done exactly this, removed the ramsey/uuid dependency and made use of a v4 UUID generated by includes/UIDGenerator.php

csteipp closed this task as Declined.Dec 4 2015, 4:59 PM
csteipp claimed this task.

Getting this off our workboard for now in that case. If you change your mind and would like it reviewed, feel free to reopen this.

hashar added a comment.Dec 7 2015, 8:08 PM

Per @Eevans and @aaron MediaWiki core now has support for UUIDv1 https://gerrit.wikimedia.org/r/#/c/56567 \O/

sbassett moved this task from Waiting to Done on the Security-Team board.Jun 11 2019, 6:03 PM
Restricted Application added a project: Analytics. · View Herald TranscriptJun 11 2019, 6:03 PM