Page MenuHomePhorge

Enforce viewable MIME types config on PDF documents
ClosedPublic

Authored by l2dy on Nov 11 2023, 08:13.

Details

Summary

Let instance admins decide whether to allow PDFs to be viewable as a Web page. See https://github.com/mozilla-conduit/phabricator/commit/5ec132bf9ebfb90558f1b7f646772176629f86d0.

MOZILLA: Instead of always allowing PDFs to be viewable in the web UI, [...]
This checks that the PDF mimetype is viewable according to the system
configuration.

Ref Q83.

Test Plan
  1. Set files.viewable-mime-types to exclude application/pdf.
  2. Upload a pdf file.
  3. See "No document engine can render the contents of this file." in web UI.

Diff Detail

Repository
rP Phorge
Branch
security/respect-viewable-mime
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 906
Build 906: arc lint + arc unit

Event Timeline

Update config description

The Referenced Files section of this diff looks like someone is looking for a vulnerability. Any idea what’s happening here?

In D25464#13328, @speck wrote:

The Referenced Files section of this diff looks like someone is looking for a vulnerability. Any idea what’s happening here?

That's a different issue unrelated to this patch. Let's track it in T15665.

This will require documentation of some sort, specifically for the upgrade notes to indicate that if someone relies on rendering PDFs currently then after upgrading they would need to update that configuration.

I think making a task to document the issue and linking in the release notes would be a good approach and what upstream would typically have done.

src/applications/files/config/PhabricatorFilesConfigOptions.php
140 ↗(On Diff #1474)

Linking to the task here might also be a good idea.

In D25464#13372, @speck wrote:

This will require documentation of some sort, specifically for the upgrade notes to indicate that if someone relies on rendering PDFs currently then after upgrading they would need to update that configuration.

I think making a task to document the issue and linking in the release notes would be a good approach and what upstream would typically have done.

This change is a defense-in-depth and opt-in only measure. I intentionally did not modify the default files.viewable-mime-types, which still includes application/pdf, so that it is not a breaking change.

Unless there is proof of successful exploit, which may have been demonstrated in Mozilla's private bug tracker that I don't have access to, I'm still doubtful whether video, audio and PDF files viewed inline could introduce a XSS vulnerability. I did not add the Security tag because of this.

Okay I misunderstood the default value. I don’t think a security tag is necessary either.

Accepting - only discussion on phrasing but functional change looks good

src/applications/files/config/PhabricatorFilesConfigOptions.php
140 ↗(On Diff #1474)

Instead of “some types are vulnerable” what do you think of “some types may be vulnerable XSS exploits when rendered by the browser”?

This revision is now accepted and ready to land.Nov 12 2023, 15:49
src/applications/files/config/PhabricatorFilesConfigOptions.php
140 ↗(On Diff #1474)

text/html is apparently vulnerable to XSS attacks, but it's not included in $viewable_default, so we are safe from that.

In conclusion, there isn't sufficient proof yet to disable certain MIME types in $viewable_default, and I could not make a task with incomplete (maybe even incorrect) information.

I hope Mozilla's engineers could chime in and help, but before that we could land this as an opt-in configuration.

l2dy added inline comments.
src/applications/files/config/PhabricatorFilesConfigOptions.php
140 ↗(On Diff #1474)

Thanks for your advice. To be consistent with "viewed directly in the browser" in the first sentence, I rephrased it a bit differently, but it's definitely clearer than before.