Page MenuHomePhabricator

Feedback/comment on LibUp 2.0 plans/architecture/changes
Closed, ResolvedPublic0 Estimate Story Points

Description

I've cc'd a bunch of people (sorry if I missed anyone) who've regularly interacted with LibUp on the new 2.0 model. Please see https://www.mediawiki.org/wiki/Libraryupgrader/2.0 and leave comments/suggestions/feedback there.

The tentative deadline is July 25.

Event Timeline

Legoktm created this task.Jul 11 2019, 8:19 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 11 2019, 8:19 PM

There are two main security considerations: 1) code injection into our repositories 2) protection of the libraryupgrader Gerrit account.
Addressing the first:

  • libup will only +2 patches that touch specific files (e.g. composer.json, package.json, package-lock.json). An attacker would need to inject content into these files OR subtly trick humans into +2'ing changes to other files.

I'd recommend against the Gerrit account or the Cloud VPS instance having the ability to +2 code.

Instead, I'd recommend the l10n-bot approach and let a Jenkins independently validate that only files matching a strict whitelist of files were modified, and only +2 it after that.

There are two main security considerations: 1) code injection into our repositories 2) protection of the libraryupgrader Gerrit account.
Addressing the first:

  • libup will only +2 patches that touch specific files (e.g. composer.json, package.json, package-lock.json). An attacker would need to inject content into these files OR subtly trick humans into +2'ing changes to other files.

I'd recommend against the Gerrit account or the Cloud VPS instance having the ability to +2 code.
Instead, I'd recommend the l10n-bot approach and let a Jenkins independently validate that only files matching a strict whitelist of files were modified, and only +2 it after that.

The whitelist approach would mean that auto-fix-up patches would not be self-merged by the bot (unless we whitelist *), which is fine but not great.

Noting that I explicitly requested feedback from the security team in a separate task to fit within their workflows (T227820: (informal) Security Concept Review For LibUp 2.0).

  • libup will only +2 patches that touch specific files (e.g. composer.json, package.json, package-lock.json). An attacker would need to inject content into these files OR subtly trick humans into +2'ing changes to other files.

I'd recommend against the Gerrit account or the Cloud VPS instance having the ability to +2 code.

Just to be clear, this is describing what libup is already doing, nothing is changing for this part. Do you think the current/previous model was also unsafe?

Instead, I'd recommend the l10n-bot approach and let a Jenkins independently validate that only files matching a strict whitelist of files were modified, and only +2 it after that.

Last time we discussed this, we rejected this approach, though we were trying to implement the filtering in Gerrit rather than Jenkins (T174760#3573713).

In any case, maybe it would be clearer to explicitly document what we trust, and what we don't:

  1. the libup code that runs out of the container - fully trusted
  2. the libup code that runs inside the container - fully trusted
  3. shelling out to npm/composer for any purpose - untrusted

Are we all in agreement on that?

The theoretical attack scenario has some untrusted code sticking in an extra dependency in package.json/composer.json that does malicious stuff. I still trust the libup code that analyzes the patch to see which files were modified when deciding to +2. Maybe we have libup diff the dependencies in package.json/composer.json after all the untrusted code has been run to ensure that any changes were whitelisted as "good libraries", before +2'ing?

The whitelist approach would mean that auto-fix-up patches would not be self-merged by the bot (unless we whitelist *), which is fine but not great.

Auto-fix patches like phpcbf have never been self-merged FWIW. I never trusted the autofix code enough to enable it.

Jcross added a subscriber: Jcross.

Security Team has reviewed and is untagging to remove from team backlog as we will not be actively working on the ticket. This was reviewed in a Concept Review: https://phabricator.wikimedia.org/T227820

Legoktm closed this task as Resolved.Dec 18 2019, 7:08 AM
Legoktm claimed this task.

The theoretical attack scenario has some untrusted code sticking in an extra dependency in package.json/composer.json that does malicious stuff. I still trust the libup code that analyzes the patch to see which files were modified when deciding to +2. Maybe we have libup diff the dependencies in package.json/composer.json after all the untrusted code has been run to ensure that any changes were whitelisted as "good libraries", before +2'ing?

FWIW, this was more or less implemented.

Closing this as resolved since libup 2.0 is implemented now. Further requests for changes should be filed as separate tickets.