Page MenuHomePhabricator

Public Phabricator dump includes restricted project columns
Closed, ResolvedPublicSecurity

Description

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.

Details

Risk Rating
Low
Author Affiliation
Wikimedia Communities

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 _
sbassett changed Author Affiliation from N/A to Wikimedia Communities.Jun 2 2022, 2:14 PM
sbassett changed Risk Rating from N/A to Low.
sbassett subscribed.

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:

CountQuery
23311SELECT count(*) FROM project_column;`
23298SELECT 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 :-\

We do not upload security patches to Gerrit since that would make them public.

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:

  1. git am the patch on deployment.eqiad.wmnet
  2. git pull on the phab boxen
  3. php-fpm restart on the phab boxen (IIRC, will have to look at cache invalidation)
  4. 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

@Dylsss and @hashar - I'm not seeing anything on this bug that should keep it protected, unless we care about the PHIDs within the task description. Happy to make this public if there's nothing of concern.

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 :-\

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 7 2022, 2:31 PM
sbassett changed the edit policy from "Custom Policy" to "All Users".

@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.