Page MenuHomePhabricator

Add composer/semver 0.1.0 to mediawiki/vendor
Closed, ResolvedPublic

Description

As part of T99084: Allow extensions to specify which MW core versions they support in extension.json, we will be re-using the composer LinkConstraint code, which has now been split out into a separate composer/semver library.

Tree: https://github.com/composer/semver/tree/0.1.0

Before adding to mediawiki/vendor, this needs a security review, so assigned to Chris.

Details

Related Gerrit Patches:
mediawiki/vendor : masterAdd composer/semver 0.1.0

Event Timeline

Legoktm created this task.May 14 2015, 4:14 PM
Legoktm assigned this task to csteipp.
Legoktm raised the priority of this task from to Medium.
Legoktm updated the task description. (Show Details)
Legoktm added a subscriber: Legoktm.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 14 2015, 4:14 PM

At 37 kloc, this will take some time to review.

There are also open security issues with composer (https://github.com/composer/composer/issues/1074) that make me a little uncomfortable including it as a general library if we don't have a way of limiting how it's used.

Is there a chance you can carve out the code you need, and we'll just include those? Or can we add a patch throwing an exception if someone tries to use the self-update code?

csteipp set Security to None.
csteipp moved this task from Incoming to Ready on the Security-Team board.

I'm trying to come up with a way to address what you pointed out, but however I go about it, I come back to that I don't have sufficient knowledge of the threat model you try to counter in the software at hand. Please outline the threat model you have in mind regarding the self-update code.

@JanZerebecki, according to the issue (I haven't traced through the code enough to exploit it myself) when the self-update is used, it uses http (when openssl isn't installed) to download a new version of composer which it compiles and runs. If we provide composer as a library, and someone writes code to that uses the self-update from composer to update itself, it would be vulnerable too. I'm not sure how likely that is in this particular case, but statements like, "There was simply no expectation of security at the beginning" [https://github.com/composer/composer/pull/1092#issuecomment-8429586] don't inspire confidence in me that there aren't other similar issues. So limiting the scope of what we pull in would be a benefit, in my opinion.

which it compiles and runs.

AFAIK that is not true. Nothing gets compiled. Only the user runs something.

According to my reading of https://github.com/composer/composer/blob/bc45d9185513575434021527d7756420e9f4b2cf/src/Composer/Command/SelfUpdateCommand.php it may download a composer.phar from HTTP without verification. Under certain conditions it replaces $_SERVER['argv'][0] with the download. But it does not itself execute the downloaded code. Unless running http://de2.php.net/manual/en/phar.construct.php on a .phar does.

I think having that composer code available is equivalent to having the following PHP functions/features available: copy, stream wrappers, include. Certainly exploitable after someone where to combine them.

Under the normal circumstances of the user running php composer.phar self-update it is only exploitable because the user themselves runs php composer.phar ... again after that.

Do you think a security review of HHVMs and PHPs Phar::__construct($file) implementation for potential code execution from $file is worthwhile relative to other security reviews? Or was that done before we started running that code in production?

Legoktm claimed this task.May 20 2015, 7:44 PM
Legoktm added a subscriber: csteipp.

I'm asking upstream to see if we can split the specific code we need into a separate library so we don't need to pull in all of composer. Assigning to myself for now.

https://github.com/composer/composer/issues/3545 the general impression I'm getting is that upstream wants to do it, but they want to do it nicely with proper separation and stuff, and it's not high on their priority lists.

Legoktm renamed this task from Add composer/composer#bc45d9185513575434021527d7756420e9f4b2cf to mediawiki/vendor to Add composer/semver 0.1.0 to mediawiki/vendor.Jul 23 2015, 6:26 AM
Legoktm updated the task description. (Show Details)
Legoktm reassigned this task from Legoktm to csteipp.Jul 23 2015, 6:28 AM

@csteipp: the code has now been split out into a *much smaller* library, so re-assigning to you for a security review.

csteipp closed this task as Resolved.Jul 30 2015, 5:55 PM

Nothing looks too worrying

Legoktm reopened this task as Open.Jul 30 2015, 8:04 PM

Nothing looks too worrying

Thanks! Re-opening since the library hasn't been added to mediawiki/vendor yet :)

Change 228132 had a related patch set uploaded (by Legoktm):
Add composer/semver 0.1.0

https://gerrit.wikimedia.org/r/228132

Change 228132 merged by jenkins-bot:
Add composer/semver 0.1.0

https://gerrit.wikimedia.org/r/228132

Legoktm closed this task as Resolved.Jul 30 2015, 9:03 PM
csteipp moved this task from Ready to Done on the Security-Team board.Oct 13 2015, 11:56 PM