The Phabricator dump at https://dumps.wikimedia.org/other/misc/ includes restricted project columns. E.g., I can see T309734 but not it's project or project column. However in the page HTML, you can find the column PHIDs PHID-PCOL-heepb6uvylp3nvayrdyg and PHID-PCOL-4yvz2dfotobspwzknhpy. And then searching in the dump, you can see that the task was moved from column "Inbox" to "In Progress". It's unlikely column names have any specific information other than generic terms, so this is really low risk. https://phabricator.wikimedia.org/diffusion/PHTO/browse/wmf%252Fstable/public_task_dump.py and https://phabricator.wikimedia.org/diffusion/PHTO/browse/wmf%252Fstable/wmfphablib/phabdb.py should just need to check that the project PHID of the project column is public.
Description
Details
- Risk Rating
- Low
- Author Affiliation
- Wikimedia Communities
Revisions and Commits
Related Objects
- Mentioned In
- T309910: Months of history missing from https://phabricator.wikimedia.org/source/phabricator-translations.git
- Mentioned Here
- rPHTOe2e1f7997f0b: Remove restricted project columns from public dump
T309910: Months of history missing from https://phabricator.wikimedia.org/source/phabricator-translations.git
T309734: Review team requests doc and add feedback
Event Timeline
The below should prevent non-public project columns from being returned:
diff --git a/wmfphablib/phabdb.py b/wmfphablib/phabdb.py index e0f7f0b..1376a5f 100755 --- a/wmfphablib/phabdb.py +++ b/wmfphablib/phabdb.py @@ -17,7 +17,7 @@ from config import bzmigrate_user from config import bzmigrate_passwd -def get_projectcolumns(dbcon): +def get_projectcolumns(dbcon, policy='public'): _ = dbcon.sql_x("SELECT id, \ phid, \ name, \ @@ -27,8 +27,11 @@ def get_projectcolumns(dbcon): dateCreated, \ dateModified, \ properties \ - FROM project_column", - (), limit=None) + FROM project_column \ + JOIN project \ + ON project_column.projectPHID=project.phid \ + WHERE project.viewPolicy=%s", + (policy,), limit=None) return _
It's unlikely column names have any specific information other than generic terms, so this is really low risk.
Agreed, but yes, it's definitely still a Vuln-Infoleak that should be mitigated sooner than later.
+ @brennen has co maintainer of Phabricator
+ @mmodell who no more works at the Wikimedia Foundation but is still somehow active :)
That is definitely a valid bug thanks for reporting it @Dylsss and special thanks to have tracked down the root cause. I wasn't aware of public dump for Phabricator nor did I have ever seen the phabricator/tools git repository so that is a huge time saver.
The resulting SQL query would looks like:
SELECT id, phid, name, status, sequence, projectPHID, dateCreated, dateModified, properties FROM project_column JOIN project ON project_column.projectPHID=project.phid WHERE project.viewPolicy='public'
I gave it a try on the Phabricator database which yields:
ERROR 1052 (23000): Column 'id' in field list is ambiguous
The original query only acted on the project_column table but the join with project which share similar field cause the ambiguity. They have to be explicitly prefixed which gives me:
SELECT project_column.id, project_column.phid, project_column.name, project_column.status, project_column.sequence, project_column.projectPHID, project_column.dateCreated, project_column.dateModified, project_column.properties FROM project_column JOIN project ON project_column.projectPHID=project.phid WHERE project.viewPolicy='public';
For the diff:
Count | Query |
---|---|
23311 | SELECT count(*) FROM project_column;` |
23298 | SELECT count(*) FROM project_column JOIN project ON project_column.projectPHID=project.phid WHERE project.viewPolicy='public'; |
So that is 13 columns (which come from 3 non public projects).
I have slightly amended your patch to rename get_projectcolumns to get_projectcolumnsbypolicy to be consistent with get_projectbypolicy.
@Dylsss I have uploaded the amended patch and you should have access to it. We do not upload security patches to Gerrit since that would make them public.
The resulting SQL looks valid and if I flip the condition to != 'public' I do get a list of 13 columns.
Stuff left to figure out:
how to deploy phabricator/tools?
Looks like we would need the patch to be send to Gerrit, merged then bump the git submodule in phabricator/deployment and deploy that (with scap?)
how to regenerate the dump?
It is a systemd timer phabricator_task_dump which triggers on 'Monday *-*-* 02:00:00':
May 23 02:00:01 phab1001 systemd[1]: Started phabricator public task dump. May 23 03:26:12 phab1001 systemd[1]: phabricator_task_dump.service: Succeeded. May 30 02:00:02 phab1001 systemd[1]: Started phabricator public task dump. May 30 03:26:34 phab1001 systemd[1]: phabricator_task_dump.service: Succeeded.
Then somehow the file is from this Friday: 03-Jun-2022 03:24 I am wiling to ignore that for now.
I don't know how to trigger a systemd timer to run immediately :-\
That is the general best practice, yes. But for low risk security issues, we're typically much more comfortable with those going through gerrit, especially when coupled with a quick deployment.
Looks like we would need the patch to be send to Gerrit, merged then bump the git submodule in phabricator/deployment and deploy that (with scap?)
Again, the Security-Team would be fine with something like this going through gerrit and then being (fairly quickly) deployed.
Thanks for looking into this @hashar!
I know the phabricator/tools repo is still in diffusion. IIRC, @mmodell mentioned that the deployment via scap doesn't work as intended due to puppet generated files, so there's work needed there.
I think the steps are:
- git am the patch on deployment.eqiad.wmnet
- git pull on the phab boxen
- php-fpm restart on the phab boxen (IIRC, will have to look at cache invalidation)
- Merge patchset for https://phabricator.wikimedia.org/diffusion/PHTO/phabricator-tools.git
@hashar or @brennen — what do you think of that? Does that all sound correct?
Paired with @brennen on this today. We tagged release/2022-06-03/1. It includes some other upstream fixes. We hope to deploy in the next phab deploy window (Wednesday).
From T309910#7985248 the rPHTR repository (phabricator-translations) apparently has been force pushed and rolled back to a commit from Feb 3rd. The branch has been moved back to the latest translations so they deployment repo probably want to reflect that by bumping the submodule.
It seems a force push has been done at some point.
Note: some documentation has been added to https://wikitech.wikimedia.org/wiki/Phabricator/Deployment
The patch has been added to the rPHTO repository with rPHTOe2e1f7997f0bf121f7afd132b5ba6ec323f948d3. It is in the release/2022-06-03/1 tag which can be browsed at https://phabricator.wikimedia.org/diffusion/PHTO/history/wmf%252Fstable/;release/2022-06-03/1
The deployment repo is https://gerrit.wikimedia.org/g/phabricator/deployment/ and Commit abf0edc Updated submodules to upstream 2022.21 bring in the change:
phabricator/deployment(wmf/stableu=)$ git show abf0edc --submodule=log ./tools/ commit abf0edc6f7ed02b86a27ae3ed8547852ed767457 Author: Brennen Bearnes <code@p1k3.com> Date: Fri Jun 3 11:54:08 2022 -0600 Updated submodules to upstream 2022.21 Submodule tools 7e8e5e3..e2e1f79: > Remove restricted project columns from public dump
Which is deployed.
The phabricator_public.dump file at https://dumps.wikimedia.org/other/misc/ is generated with the new code. Given @Dylsss has marked this task resolved I am assuming the dump no more contains the private id.
@sbassett we can indeed mark this task resolved. The name of some columns got leaked, then when I looked at their names from the dump files, the titles were not carrying any sensitive information (they are named something like work in progress backlog processing etc). I don't know how to make the task public though :-\
Ok, sounds good. I've made this task public. You're in both acl*security and acl*security_releng, so I think you should be able to set these policies by going to Edit Task and then setting the Visible To and Editable By policies (towards the bottom of the form) to Public.