Maniphest T202143

Security review for Guzzle 6.3.3
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	BPirkle
	Aug 17 2018, 3:16 PM

Description

Requesting security review of Guzzle 6.3.3 per https://www.mediawiki.org/wiki/Manual:External_libraries

The intention is to replace existing custom code with a well-maintained, PSR-7 compliant external library, which provides automatic fallback from curl to php streams. See proposed change at T202110

Guzzle would likely also be used in MultiHttpClient, which needs a non-curl, mediawiki-independent http solution. See T139169 , which resolved MultiHttpClient's curl dependency but introduced a dependency on mediawiki code that is undesirable in a library.

Related Objects

Mentioned In: T221986: Security Review of RESTBagOStuff
T202110: Create GuzzleHttpRequest class as new default for HttpRequestFactory
T202352: Convert MultiHttpClient to use Guzzle
Mentioned Here: T202352: Convert MultiHttpClient to use Guzzle
T191638: Security review of symfony/validator library
T139169: Add non-parallel MultiHttpClient fallback for environments that don't have curl available
T202110: Create GuzzleHttpRequest class as new default for HttpRequestFactory

Event Timeline

BPirkle created this task.Aug 17 2018, 3:16 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 17 2018, 3:16 PM

Requesting security review

-> Application Security Reviews, otherwise noone will find this... :)

Thank you @Aklapper! I was going by https://www.mediawiki.org/wiki/Manual:External_libraries , which mentions adding MediaWiki-Vendor. Should I update that documentation to say add both MediaWiki-Vendor and Security-Reviews, or is it just Security-Reviews now?

BPirkle mentioned this in T202352: Convert MultiHttpClient to use Guzzle.Aug 21 2018, 12:51 AM

T191638: Security review of symfony/validator library ended up being closed without a security review officially happening as the library is "currently well maintained and free of significant CVE's"

one CVE on file refers to guzzle

I'm curious to see if guzzle will get the same treatment or if due to the nature of the library an additional security review would still be done?

Addshore added a project: User-Addshore.Aug 21 2018, 9:55 AM

Addshore moved this task from Unsorted 💣 to Watching 👀 on the User-Addshore board.

Proposed usage of Guzzle in MultiHttpClient is at T202352

The code to review seems to be https://github.com/guzzle/guzzle

CVE-2016-5385 is the HTTP_PROXY vulnerability. Not a worrying one that it affected them, imho.

Bawolff moved this task from Incoming to Scheduled on the deprecated-security-team-reviews board.Sep 11 2018, 7:18 PM

Bawolff claimed this task.Sep 11 2018, 7:47 PM

• Fjalapeno subscribed.Sep 25 2018, 3:53 PM

Bump, just to make sure this doesn't fall off the radar.

Related tasks T202352 and T202110 rebased against current master.

I'm happy for this to go ahead.

As the guzzle libraries are pretty widely used, so it's more about "due diligence" than actual thorough security review of the code (unless we had reasonable suspicion otherwise)

Last CVE was https://www.cvedetails.com/cve/CVE-2016-5385/ fixed in https://github.com/guzzle/guzzle/releases/tag/6.2.1

Vendor patch can be landed (I probably shouldn't CR+2 it as I created it), and make development a bit easier

Looks like some possible test failures in core that need addressing, but that's irrelevant to this review

Reedy mentioned this in T202110: Create GuzzleHttpRequest class as new default for HttpRequestFactory.Dec 4 2018, 2:26 PM

Looks like some possible test failures in core that need addressing, but that's irrelevant to this review

Seems they were just transient and possibly related to me creating the vendor patch on the wrong branch. Jenkins gives a V+2 now :)

The only minor security thing I noticed was that MediaWiki's HTTP stuff does not follow redirects by default, while Guzzle will follow 5 layers of redirects by default. So as long as everyone continues to use the MediaWiki abstraction layer, the defaults should still be as strict as we want them.

In T202143#4802539, @Legoktm wrote:

The only minor security thing I noticed was that MediaWiki's HTTP stuff does not follow redirects by default, while Guzzle will follow 5 layers of redirects by default. So as long as everyone continues to use the MediaWiki abstraction layer, the defaults should still be as strict as we want them.

I guess that's a documentation/RELEASE-NOTES thing, plus paying attention in CR

Extensions may be more likely to start calling Guzzle directly, but it's obviously a back compat thing for a few versions, so may deter them doing so for a while

BPirkle mentioned this in T221986: Security Review of RESTBagOStuff.Jun 26 2019, 9:34 PM

sbassett moved this task from Scheduled to Done on the deprecated-security-team-reviews board.Jul 9 2019, 8:27 PM

• chasemp removed a project: deprecated-security-team-reviews.Jan 8 2020, 4:12 PM

• chasemp added a project: Application Security Reviews.Jan 8 2020, 4:38 PM

• chasemp moved this task from Incoming to Our Part Is Done on the Application Security Reviews board.Jan 8 2020, 4:43 PM

• chasemp added a project: secscrum.Mar 10 2020, 8:11 PM

• chasemp moved this task from Incoming to Our Part Is Done on the secscrum board.Mar 10 2020, 8:19 PM