Closed Bug 1548114 Opened 6 years ago Closed 4 years ago

Lando: Format code when landing a patch

Categories

(Conduit :: Lando, enhancement, P2)

Product:
Conduit
Component:
Lando
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: smacleod, Assigned: sheehan)

References

Details

(Keywords: conduit-triaged)

Attachments

(1 file)

GitHub Pull Request
53 bytes, text/x-github-pull-request
Details | Review

Lando should land a version of the patch but with code formatting applied.

RelMan generates a version of the Phabricator patch with the code automatically formatted if it can be, and uploads this as a taskcluster artifact. We should be able to anonymously query taskcluster when landing and see if the patch exists. If there is a formatted version we can use that instead of the one on Phabricator.

(In reply to Steven MacLeod [:smacleod] from comment #1)

Currently the tasks are indexed under project.releng.services.project.production.static_analysis_bot.phabricator.<revision-id>, see https://tools.taskcluster.net/index/project.releng.services.project.production.static_analysis_bot.phabricator

:marco/:bastien, is there a bug on file somewhere to track indexing based on diff id?

Flags: needinfo?(mcastelluccio)
Flags: needinfo?(bastien)

(In reply to Bastien Abadie [:bastien] from comment #3)

It's already done: https://tools.taskcluster.net/index/project.releng.services.project.production.static_analysis_bot.phabricator.diff

Awesome! Looks like all the dependencies are finished for this feature then.

Flags: needinfo?(mcastelluccio)

Here is an example with clang-format:
https://phabricator.services.mozilla.com/D29624#877011
Try job:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cbd1d839dbc271471df9673fd28e4b6302b19c1

Summary: Format code when landing a patch → Lando: Format code when landing a patch

Note that we cannot use a patch generated on try to generate patches that will be automatically landed. At least not without a security review.

I agree we need a formal security review of the code-review bot, now that we use Try tasks as analyzers.
Clang-format only produces whitespace changes, so they should be harmless.

(In reply to Bastien Abadie [:bastien] from comment #7)

I agree we need a formal security review of the code-review bot, now that we use Try tasks as analyzers.
Clang-format only produces whitespace changes, so they should be harmless.

The issue is that somebody could modify the analyzer itself to make it output what they want. In order to land it the patch has to be accepted though, so a reviewer should notice if somebody attempts to do this and so the landing would be blocked.
To reduce the risk to 0, we could enforce the patch to be only whitespace changes in the bot.

(In reply to Marco Castelluccio [:marco] from comment #8)

The issue is that somebody could modify the analyzer itself to make it output what they want. In order to land it the patch has to be accepted though, so a reviewer should notice if somebody attempts to do this and so the landing would be blocked.

The reviewer checking the diff is not sufficient; with just level-1 credentials, I'm fairly certain that I could serve an arbitrary patch for any differential revision to lando, if it is pulling the diff the diff from a task that is on try (without it showing up in anyway on phabricator.

To reduce the risk to 0, we could enforce the patch to be only whitespace changes in the bot.

My initial reaction to this, is that trying to ensure security by analyzing patches is not something we want to do. I'd anticipate that the code to check if a patch is whitespace only is complex enough that it shouldn't be major part of the security of this feature.

(In reply to Tom Prince [:tomprince] from comment #9)

(In reply to Marco Castelluccio [:marco] from comment #8)

The issue is that somebody could modify the analyzer itself to make it output what they want. In order to land it the patch has to be accepted though, so a reviewer should notice if somebody attempts to do this and so the landing would be blocked.

The reviewer checking the diff is not sufficient; with just level-1 credentials, I'm fairly certain that I could serve an arbitrary patch for any differential revision to lando, if it is pulling the diff the diff from a task that is on try (without it showing up in anyway on phabricator.

We could enforce that the author of the try push is our bot in the code review task, this way you shouldn't be able to do that unless you steal the bot's credentials.

To reduce the risk to 0, we could enforce the patch to be only whitespace changes in the bot.

My initial reaction to this, is that trying to ensure security by analyzing patches is not something we want to do. I'd anticipate that the code to check if a patch is whitespace only is complex enough that it shouldn't be major part of the security of this feature.

I was thinking it wouldn't be that hard, but maybe I'm wrong.

We could enforce that the author of the try push is our bot in the code review task, this way you shouldn't be able to do that unless you steal the bot's credentials.

Given the permissions given to people with level-1 access, it is not possible to verify that.

Let me add that, while I don't think it is possible to run the task to generate the patch on try safely, I do think it is possible to run the job in taskcluster; I'm just not sure what that would look like.

(In reply to Marco Castelluccio [:marco] from comment #10)

(In reply to Tom Prince [:tomprince] from comment #9)

My initial reaction to this, is that trying to ensure security by analyzing patches is not something we want to do. I'd anticipate that the code to check if a patch is whitespace only is complex enough that it shouldn't be major part of the security of this feature.

I was thinking it wouldn't be that hard, but maybe I'm wrong.

It isn't necessarily hard, but the resulting code will be complex and security sensitive, which is a combination to avoid if at all possible, particularly when dealing with potentially malicious input.

Marking as a security bug so I don't advertise a possible attack.

Thanks for bringing up the security aspect :tomprince.

The situation I'd be most worried about is:

  • Malicious User (MU) crafts a malicious patch (MP) which changes the in-tree reformat job to introduce a vulnerability upon landing.
  • MU creates an innocuous patch (IP) which fixes a small bug, and commits that separately on top of the MP.
  • MU Uploads MP and IP as a revision stack to Phabricator.
  • Review Bot executes the reformat job for IP, applying MP first since it is a parent of the revision for IP.
  • Review Bot has created a malicious reformatted patch (MRP) which is marked as the to land patch for IP.
  • MU removes MP as a parent of IP.
  • Reviewer for IP comes along and accepts IP.
  • Landing for IP is triggered, landing the harmful MRP without someone looking at it.

In order to prevent this attack the reviewer would need to inspect either the Phabricator emails or the revision timeline, and decide to look into the old parent (MU) that was removed. They would then also need to realize the MU is harmful. I think it'd be quite easy for this to slip by the reviewer.

The other thing to keep in mind is that since review bot is triggering these jobs for ALL diffs posted to phabricator, scm_level_1 doesn't factor in at all. The only credentials required to execute this attack would be a BMO account and the Phabricator account tied to it.

Group: conduit-security

The issue in comment 13 could be addressed by having lando trigger the job that creates the formatting patch, so exactly what was requested to be landed gets formatted.

(In reply to Steven MacLeod [:smacleod] from comment #13)

The other thing to keep in mind is that since review bot is triggering these jobs for ALL diffs posted to phabricator, scm_level_1 doesn't factor in at all. The only credentials required to execute this attack would be a BMO account and the Phabricator account tied to it.

This itself is yet another vulnerability, isn't it?

  • MU would like some free cloud computing capacity.
  • MU registers an account on BMO and Phabricator.
  • MU pushes many patches which have modified our in-tree format job to mine their cryptocurrency of choice.

(In reply to :Ehsan Akhgari from comment #15)

This itself is yet another vulnerability, isn't it?

  • MU would like some free cloud computing capacity.
  • MU registers an account on BMO and Phabricator.
  • MU pushes many patches which have modified our in-tree format job to mine their cryptocurrency of choice.

Indeed, this was brought up previously elsewhere. There has been an RRA now and it was discussed as part of it. I believe either :marco or :bastien are following up with some metrics so that at minimum we'll be able to see if someone starts abusing this. Could either of you see also the bug/issue tracking that work?

Flags: needinfo?(mcastelluccio)
Flags: needinfo?(bastien)

Given that we have decided to format code only on landing, I don't think we need to worry about people being able to run arbitrary code. (It is an issue for the codereview bot, though)

We had a issue in our team's tracker, I filed a mirror issue at https://github.com/mozilla/code-review/issues/42.

Flags: needinfo?(mcastelluccio)
Flags: needinfo?(bastien)

I, for one, think the code should be formatted before it's reviewed. For at least two reasons:

  • you want the code that lands to actually be the code that was reviewed.
  • reviewbot comments about formatting kind of get in the way of reviewing. It's not always clear to reviewers whether what they're seeing was fixed against what reviewbot commented about or not. If instead of saying that things should be reformatted, reviewbot just attached a new reformatted diff, it would be much better for reviewers (at least it would be much better for me as a reviewer).
Depends on: 1714470
Assignee: nobody → sheehan
Group: conduit-security
Mentor: steven
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Attached file GitHub Pull Request —
See Also: → 1848741
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: