Page MenuHomePhorge

Enforce viewable MIME types config on PDF documents
ClosedPublic

Authored by l2dy on Nov 11 2023, 08:13.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 10, 00:16
Unknown Object (File)
Fri, May 10, 00:16
Unknown Object (File)
Fri, May 10, 00:16
Unknown Object (File)
Fri, May 10, 00:16
Unknown Object (File)
Wed, May 8, 06:12
Unknown Object (File)
Wed, May 8, 05:56
Unknown Object (File)
Thu, May 2, 09:59
Unknown Object (File)
Wed, May 1, 19:19

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
139

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
139

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
139

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
139

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.