1632688 - `mach try` now adds all untracked files in the source tree to the temporary commit before pushing

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Kris Maglione [:kmag]]

Since bug 1630047, if tryselect doesn't find any new explicit files to add to the commit, it calls hg addremove with an empty arguments list, which adds all untracked files in the tree to the commit. If this doesn't cause the push to fail due to it containing unsuitably large files or symlinks, it has a serious risk of pushing sensitive information to the public try server, like any credentials files a developer happens to have in their source tree. Someone should probably comb all try pushes since this landing to make sure this has not actually happened.

Comment 1

[Tom Prince [:tomprince]]

Attachment: Try syntax commits

I did some poking. Using the hg revset:

heads((3910c83dfabc63a963a465fdb89e4663aad34c45:) and not desc("Merge try head"))

And the following set of commands[1]

curl 'https://hg.mozilla.org/try/json-log?rev=heads%283910c83dfabc63a963a465fdb89e4663aad34c45%3A+and+not+desc%28%22Merge+try+head%22%29%29&revcount=100000' | jq '.entries|.[].node' -r > tee nodes
hg pull -r $(python2 -c 'print " -r ".join(open("nodes").readlines())') try
hg log -r 'draft() and ancestors(include("nodes-3"))  ' -T 'node' > try-revs
hg log -r 'include("try-revs") and adds("re:^(?!try_task_config.json)")' -T "{node}:\n{file_adds % '  {file}\n'}" > added-files-per-commit
grep '^ ' added-files-per-commit | sort -u | cut -c 3- > added-files

I think the primary case that will be impacted by this, is people using mach try syntax
The following command should show those commits:

hg log -r 'include("try-revs") and desc("try:") and adds("*")' --stat

Spot checking those, I don't see anything immediately concerning; I've attached the authors and commits from there.

[1] This needs this hg extension to run.

Comment 2

[Hal Wine [:hwine] use NI!]

This is a significant concern. We'd like to have the causal bug backed out.

Tom:

• can you roll back bug 1630047 on your own, or do we need someone to push things after that?
• which additional files are picked up? E.g. are dot-files added?

Comment 3

[Tom Prince [:tomprince]]

Attachment: Bug 1632688: [mozversioncontrol] Don't try to add files if none are provided;

Comment 4

[Tom Prince [:tomprince]]

https://hg.mozilla.org/mozilla-central/rev/9442967f483a7c61b520c3f559a8db9fb29aa573

Comment 5

[Tom Prince [:tomprince]]

Looking at all the caller of add_remove_files, there are three:

• tryselect, which is affected
• lint hooks, which already have a check for not having any files
• vendoring tools. which pass an explicit list of directories.

Testing the behavior of hg add and git add without at a list of files, hg will add all files (including dot files) and git will error.

Comment 6

[Hal Wine [:hwine] use NI!]

With the problematic code no longer executed (thanks Tom!) We'd like to pin down the branches where someone might be using the buggy version.

• What was the revision where the bug initially landed
• What beta builds were released during the existence of the bug? (devs working on bug fixes may have been impacted)
• When is the next beta scheduled? (i.e. how soon will devs working on beta no longer have the bad code.)
• What release builds were made from the bad source?
• Would the queries in comment 1 have caught try pushes based on affected mozilla-beta or mozilla-release code?

Based on those answers, we can determine when "last use of bad code" is likely to occur, and run another sweep then.

Since we may not know every kind of secret that could have been in an affected try run, I'll prepare a communication to notify impacted users of the situation, so they can rotate any secrets they are concerned with.

Comment 7

[Tom Prince [:tomprince]]

(In reply to Hal Wine [:hwine] (use NI, please) from comment #6)

With the problematic code no longer executed (thanks Tom!) We'd like to pin down the branches where someone might be using the buggy version.

• What was the revision where the bug initially landed
  https://hg.mozilla.org/integration/autoland/rev/3910c83dfabc / https://hg.mozilla.org/mozilla-central/rev/3910c83dfabc

• What beta builds were released during the existence of the bug? (devs working on bug fixes may have been impacted)
• When is the next beta scheduled? (i.e. how soon will devs working on beta no longer have the bad code.)

No betas had the commit in question, as the code landed after the last merge to beta.

• What release builds were made from the bad source?

Nightlies released between when that commit landed and now.

• Would the queries in comment 1 have caught try pushes based on affected mozilla-beta or mozilla-release code?

It would be good to have someone (:sheehan) check the logic, but it should. (I'll post a followup comment walking through the logic there)

Based on those answers, we can determine when "last use of bad code" is likely to occur, and run another sweep then.

Since we may not know every kind of secret that could have been in an affected try run, I'll prepare a communication to notify impacted users of the situation, so they can rotate any secrets they are concerned with.

Comment 8

[Tom Prince [:tomprince]]

Explanation of audit code

heads((3910c83dfabc63a963a465fdb89e4663aad34c45:) and not desc("Merge try head"))

• 3910c83dfabc63a963a465fdb89e4663aad34c45: is all commits to try after the relevant commit landed
• desc("Merge try head") catches trivial merges that :sheehan runs periodically to prune the number of heads of try, and are not relevant
• heads(....) gets all commits in the set that have no children in the set (so pulling all these commits should pull all commits pushed after the code landed

curl 'https://hg.mozilla.org/try/json-log?rev=heads%283910c83dfabc63a963a465fdb89e4663aad34c45%3A+and+not+desc%28%22Merge+try+head%22%29%29&revcount=100000' | jq '.entries|.[].node' -r > tee nodes

This queries try for all the commits in the above revset, and extracts the commits from them.

hg pull -r $(python2 -c 'print " -r ".join(open("nodes").readlines())') try

This pulls all the commits to the local repository

hg log -r 'draft() and ancestors(include("nodes"))  ' -T 'node' > try-revs

The lists all the commits commits that are in the draft phase[1] and ancestors of the heads discovered above.

hg log -r 'include("try-revs") and adds("re:^(?!try_task_config.json)")' -T "{node}:\n{file_adds % '  {file}\n'}" > added-files-per-commit

This gets the list of commits that add files other than try_task_config.json and the files that are added.

grep '^ ' added-files-per-commit | sort -u | cut -c 3- > added-files

This extracts the list of unique files added.

---

hg log -r 'include("try-revs") and desc("try:") and adds("*")' --stat

This gets the list of commits that have try: in the commit message[2] that add any files.

[1] pushes to try typically have all non-upstream commits in draft phase, but it might be possible that not all of them are; it might make sense to adjust this to catch those cases
[2] I believe that this bug only impacted people use mach try syntax (the other try selectors should add the try_task_config.json file, which means the bug isn't triggered), and those require having try: in the commit message.

Comment 9

[Hal Wine [:hwine] use NI!]

:sheehan -- what are your thoughts on the work to date in this bug, and in comment 8 in particular?

Comment 10

[Connor Sheehan [:sheehan]]

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

I think most of this looks good.

hg log -r 'draft() and ancestors(include("nodes"))  ' -T 'node' > try-revs

The lists all the commits commits that are in the draft phase[1] and ancestors of the heads discovered above.

I think this should be -T '{node}\n', otherwise try-revs will just be a file containing nodenodenodenode.... This is what I see locally when running hg log -r 'draft()' -T 'node' > try-revs && cat try-revs. Can you make sure this isn't a typo from writing up how the audit script works?

Also I don't think we want the draft() piece, as it's possible to push changesets to try in the public phase which would be excluded here.

(Slightly unrelated but you can probably use the experimental extdata revset instead of the custom include to achieve the same thing in your revsets).

Comment 11

[Tom Prince [:tomprince]]

Ah, I have a template alias for node -> {node}\n, and didn't think the remove that when copying pasting the commands.

[templates] 
node = "{node}\n"

Also I don't think we want the draft() piece, as it's possible to push changesets to try in the public phase which would be excluded here.

We do need something to exclude commits in central. I guess maybe not ancestors(central)

(Slightly unrelated but you can probably use the experimental extdata revset instead of the custom include to achieve the same thing in your revsets).

TIL

Comment 12

[Hal Wine [:hwine] use NI!]

Emails sent to the 9 devs listed in attachment 9143148 [details]

Comment 13

[Hal Wine [:hwine] use NI!]

Remaining actions per slack channel discussion:

• [ ] consider another query sweep late next week (Thursday 2020-04-30)
• [ ] decide if further action is needed then

Comment 14

[Tom Prince [:tomprince]]

Attachment: bug-1632688-anylysis.zip

I redid the analysis as of earlier today, and uploaded the results.

curl 'https://hg.mozilla.org/try/json-log?rev=heads((3910c83dfabc63a963a465fdb89e4663aad34c45%3A)+and+not+desc(%22Merge+try+head%22))&revcount=100000' | jq '.entries|.[].node' -r > nodes
hg pull -r $(python2 -c 'print " -r ".join(open("nodes").readlines())') try
hg log -r 'not ancestors(central+beta+release+esr68) and ancestors(include("nodes"))  ' -T '{node}\n' > try-revs
hg log -r 'include("try-revs") and adds("re:^(?!try_task_config.json)")' -T "{node}:\n{file_adds % '  {file}\n'}" > added-files-per-commit 
hg log -r 'include("try-revs") and desc("try:") and adds("*")' -T '{user}\t {node}\n' > authors

Comment 16

[Daniel Veditz [:dveditz]]

not sure how to rate this as a security bug? Not really a client vulnerability at all as implied by the sec-rating, but if a developer accidentally leaked auth tokens into the public tree that could be used to harm our users indirectly if it's not caught and revoked. Going with a "worry level" rating even if it's not quite applicable. (this bug should not get an advisory)

Comment 18

[Tom Prince [:tomprince]]

https://hg.mozilla.org/mozilla-central/rev/9442967f483a7c61b520c3f559a8db9fb29aa573

Comment 19

[Hal Wine [:hwine] use NI!]

Per matrix chat with :tomprince, a final verification by spot checking commits still remains to be done before the bug can be closed.

ni :tomprince for visibility

Comment 20

[Hal Wine [:hwine] use NI!]

no problems have been reported, so believe this patch is solid.