Page MenuHomePhabricator
Create Task
Maniphest T339389

Application Security Review Request : endroid/qr-code PHP library
Closed, ResolvedPublic
Actions

Description

Project Information

Description of the tool/project:
QR code generator written in PHP.

Description of how the tool will be used at WMF:
To generate QR codes from URLs, as part of the #19 proposal on the 2023 Community Wishlist Survey.

Dependencies

Has this project been reviewed before?
No.

Working test environment
Checkout https://gerrit.wikimedia.org/r/c/mediawiki/extensions/UrlShortener/+/930716, browse to any page and click on the "Download QR code" link in the toolbox. This will not work on PatchDemo until https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/930855 is merged.

Post-deployment
As for updating the package as needed, and the QR code functionality:
Maintained by: Community-Tech
Primary contact: @KSiebert

For the UrlShortener extension as a whole: Platform Engineering

Details

Risk Rating
Low
SubjectRepoBranchLines +/-
Add endroid/qr-code 4.6.1mediawiki/vendormaster+12 K -3
Customize query in gerrit

Related Objects
Search...

Event Timeline

MusikAnimal created this task.Jun 16 2023, 8:24 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 16 2023, 8:24 PM
MusikAnimal mentioned this in T242467: Automatically generate and display QR Code.Jun 16 2023, 8:25 PM
MusikAnimal added a comment.EditedJun 16 2023, 8:28 PM

P49443 looks pretty scary. I'm not sure if you're obligated to review all of that code or not... but I should mention that I think we could get away with using just bacon/bacon-qr-code (which requires dasprid/enum) if necessary. However the API is much harder to work with and documentation is lacking. Using endroid/qr-code also opens the door for us to add more features later, such as generating PNGs instead of SVGs, and adding an image in the QR code.

MusikAnimal added a parent task: T242467: Automatically generate and display QR Code.Jun 16 2023, 8:36 PM
sbassett moved this task from Incoming to Upcoming Quarter Planning Queue on the secscrum board.Jun 20 2023, 2:59 PM
sbassett subscribed.EditedJun 20 2023, 3:30 PM
In T339389#8939657, @MusikAnimal wrote:

P49443 looks pretty scary. I'm not sure if you're obligated to review all of that code or not...

No, the Security-Team is not resourced to support line-by-line code reviews of vendor/third-party packages. For these kinds of reviews, we typically follow our third-party checklist as a basic guideline, and then supplement that with some additional automated security tools and manual pentesting to look for potential weaknesses within the code. Beyond that, we need to accept at least a default low risk of heavily-leveraging FOSS code within our codebases and projects.

Target date for deployment: Sooner the better :)

@KSiebert - Per our application security review SOP, these requests are prioritized at the beginning of each quarter with a default timeline of end of quarter for delivery. If a review is more pressing for legitimate reasons, we can try to schedule a more specific date for delivery of the review within a given quarter.

MusikAnimal added a comment.Jun 20 2023, 4:42 PM
In T339389#8949418, @sbassett wrote:

No, the Security-Team is not resourced to support line-by-line code reviews of vendor/third-party packages. For these kinds of reviews, we typically follow our third-party checklist as a basic guideline, and then supplement that with some additional automated security tools and manual pentesting to look for potential weaknesses within the code. Beyond that, we need to accept at least a default low risk of heavily-leveraging FOSS code within our codebases and projects.

Understood! I did go through the code review checklist and I believe it passes most of the points. I did not however do the static analysis or check every available vulnerability database, which I understand you have automation for. I'm happy to try the automation myself though if that is the protocol (I assumed you would repeat the process anyway?).

Given the popularity of the library, lack of documented issues, etc., I endorse accepting the risk, but that responsibility if you need to hear it I suppose should come from @KSiebert.

sbassett changed the task status from Open to In Progress.Jul 10 2023, 5:27 PM
sbassett assigned this task to Mstyles.
sbassett triaged this task as Medium priority.
sbassett moved this task from Upcoming Quarter Planning Queue to In Progress on the secscrum board.
sbassett added a subscriber: Mstyles.

Assigned to @Mstyles for completing by September 30th, 2023.

KSiebert added a comment.Sep 1 2023, 7:47 PM

@MusikAnimal @sbasset Once @Mstyles has worked on this, maybe we can get a risk level and it will be easier to decide if we accept it. Thanks a lot!

Mstyles added a comment.Sep 1 2023, 7:58 PM

I'm hoping that https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/930855 will be merged soon so that the library can be tested as it's being used. @KSiebert is there a chance the team is not moving forward with this project?

gerritbot added a comment.Sep 6 2023, 7:05 PM

Change 930855 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/vendor@master] Add endroid/qr-code 4.5.2

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

gerritbot added a project: Patch-For-Review.Sep 6 2023, 7:05 PM
MusikAnimal added a comment.Sep 6 2023, 7:08 PM
In T339389#9137659, @Mstyles wrote:

I'm hoping that https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/930855 will be merged soon so that the library can be tested as it's being used.

It was my impression adding new packages to mediawiki/vendor requires security review, not the other way around?

is there a chance the team is not moving forward with this project?

We've been waiting for the security review, otherwise we're ready to go :)

There's more work on the horizon (T341361) but that can be done later and is independent of this security review.

Mstyles added a comment.Sep 21 2023, 11:15 PM

@MusikAnimal you're right we would want the security review before being merged. It'll be posted tomorrow!

Mstyles closed this task as Resolved.Sep 26 2023, 8:53 PM
Mstyles moved this task from In Progress to Our Part Is Done on the secscrum board.

Security Review Summary - T339389 - 2023-09-22

Overall, the current vendor code under consideration...
with an overall risk rating of: low.

The qr-code project has no security concerns that I can find.
Since this is low risk, you can go ahead and merge all patches that are waiting. If you have any questions, please ask.
I marked this task as resolved since the project is low risk and there are no follow up actions.

qr-code

General Security Information

Statistic/InfoValueRisk
Repositoryhttps://github.com/endroid/qr-code none
Relevant tag/branchmaster none
Last commit reviewed (if relevant)013f6c3 none
Recent contributions to code (6 months)5 medium
Active developers with > 10 commits2 medium
Current overall usage4.1k stars, 711k forks low
Current open security issues0 none

Vulnerable Packages
none found from snyk, php security checker

Outdated Packages
As reported via composer outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

PackageCurrentWantedLatest (Remediation)
endroid/qualitydev-master 2d0ece0dev-master ee2ddb8dev-master ee2ddb8

*semgrep*
one very low severity issue reported from semgrep command line

.github/workflows/CI.yml

yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha.third-party-action-not-
pinned-to-commit-sha
   An action sourced from a third-party repository on GitHub is not pinned to a full length
   commit SHA. Pinning an action to a full length commit SHA is currently the only way to use
   an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a
   bad actor adding a backdoor to the action's repository, as they would need to generate a
   SHA-1 collision for a valid Git object payload.
   Details: https://sg.run/koLA

    14┆ - uses: shivammathur/setup-php@v2

Static Analysis Findings

  1. gitleaks no leaks found. low risk
  2. git secrets run with a custom pattern set, returned no actionable results. low risk
  3. whispers returned no results. low risk
  4. scan scan:latest returned no results low risk
  5. semgrep supply chain found no vulnerabilities low risk

DAST Findings
Wapiti was run against a locally hosted wiki using the provided patch
No relevant findings were found

General Security Recommendations

  1. It is advisable to add a watermark for the qr codes that are generated to make the origin clear
gerritbot added a comment.Sep 29 2023, 11:15 PM

Change 930855 merged by jenkins-bot:

[mediawiki/vendor@master] Add endroid/qr-code 4.6.1

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

Maintenance_bot removed a project: Patch-For-Review.Sep 29 2023, 11:30 PM
ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.29; 2023-10-03).Sep 30 2023, 1:00 AM
Reedy mentioned this in T348590: Make use of endroid/qr-code?.Oct 10 2023, 9:04 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL