Page MenuHomePhabricator

Security review for Guzzle 6.3.3
Closed, ResolvedPublic

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.

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

-> Security-Team-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?

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?

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 claimed this task.Sep 11 2018, 7:47 PM

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

Related tasks T202352 and T202110 rebased against current master.

Reedy closed this task as Resolved.Dec 4 2018, 2:22 PM
Reedy claimed this task.
Reedy added subscribers: Bawolff, Reedy.

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 added a comment.Dec 4 2018, 3:12 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 :)

Legoktm added a subscriber: Legoktm.Dec 6 2018, 8:01 AM

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.

Reedy added a comment.Dec 6 2018, 8:15 AM

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