Page MenuHomePhabricator

Prevent sending a header with unsafe characters [\r\n\0]
ClosedPublic

Authored by 20after4 on May 4 2016, 7:23 PM.
Tags
None
Referenced Files
F13250272: D15843.diff
Fri, May 24, 1:39 PM
F13216541: D15843.diff
Sat, May 18, 1:17 AM
F13177405: D15843.diff
Wed, May 8, 7:41 PM
Unknown Object (File)
Sun, May 5, 2:03 AM
Unknown Object (File)
Mon, Apr 29, 2:49 PM
Unknown Object (File)
Apr 24 2024, 10:48 PM
Unknown Object (File)
Apr 17 2024, 8:54 AM
Unknown Object (File)
Apr 7 2024, 4:58 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

This change prevents a malformed filename from breaking things.
Due to the code in AphrontHTTPSink->writeHeaders.

Test Plan

If you attach a file to a maniphest task, and that file has a
newline or null in the filename, you get the following error
when attempting to download the file:

"Declining to emit response with unsafe HTTP header"

Diff Detail

Repository
rP Phabricator
Branch
FixFilenameHeader
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/applications/diffusion/controller/DiffusionServeController.php:1116PHL1Unknown Symbol
Errorsrc/applications/diffusion/controller/DiffusionServeController.php:1118PHL1Unknown Symbol
Errorsrc/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php:26PHL1Unknown Symbol
Errorsrc/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php:26PHL1Unknown Symbol
Errorsrc/applications/nuance/cursor/NuanceGitHubImportCursor.php:83PHL1Unknown Symbol
Errorsrc/applications/nuance/cursor/NuanceGitHubImportCursor.php:201PHL1Unknown Symbol
Errorsrc/applications/nuance/cursor/NuanceGitHubRepositoryImportCursor.php:28PHL1Unknown Symbol
Errorsrc/applications/nuance/github/NuanceGitHubRawEvent.php:100PHL1Unknown Symbol
Errorsrc/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php:51PHL1Unknown Symbol
Errorsrc/applications/transactions/editengine/PhabricatorEditEngine.php:1421PHL1Unknown Symbol
Errorsrc/infrastructure/cluster/PhabricatorDatabaseRef.php:296PHL1Unknown Symbol
Errorsrc/view/layout/PHUICurtainPanelView.php:27PHL1Unknown Symbol
Errorsrc/view/layout/PHUICurtainView.php:57PHL1Unknown Symbol
Unit
Tests Passed
Build Status
Buildable 12039
Build 15158: Run Core Tests
Build 15157: arc lint + arc unit

Event Timeline

20after4 retitled this revision from to Prevent sending a header with unsafe characters [\r\n\0].
20after4 updated this object.
20after4 edited the test plan for this revision. (Show Details)
20after4 added a reviewer: epriestley.
epriestley edited edge metadata.

Here's some effort to muddle through the RFC's on this:

I think this is defined here:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec19.html#sec19.5.1

And "quoted-string" is defined here:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2

And that basically says that escape sequences like \n and \r and such don't mean anything special: for example, you can not encode newline with \n or \x0A or anything fancy like that.

So the "correct" way to emit this header for "file\nwith\newlines" (with newlines) appears (???) to be:

Content-Disposition: attachment; filename="file
with
newlines"

I trust approximately zero software in existence to parse that according to my reading of the spec since the defacto way you separate HTTP headers is by exploding them on newlines, so simply stripping these characters out seems like the most reasonable approach to me.

Technically, I think we're supposed to put a backslash before all octets in the range 0-31 and 127, too, but it's hard to imagine anything parsing that wrong.

This revision is now accepted and ready to land.May 4 2016, 9:17 PM

rfc2616 is obsolete. You'll want to look at RFC7230 (low level details) and RFC7231 (headers).

Ah, thanks. RFC7230 appears to allow these characters to appear verbatim (the omission of %x22 is double quote, and %x5C is backslash):

%x09 (tab) / %x20 (space) / %x21 / %x23-5B / %x5D-7E / %x80-%xFF

And these to appear prefixed by backslash:

%x09 (tab) / %x20 (space) / %x21-7E / %x80-%xFF

So it looks like it's impossible (?) to encode \x00-\x08, \x0A-\x1F, and \x7F, and we should presumably strip all of these, then quote " and \\ (or vice-versa, quote/strip order shouldn't matter since the two sets are non-overlapping).

The most correct change is probably:

// Strip characters forbidden by RFC7230.
$filename = preg_replace('/[\x00-\x08\x0A-\x1F\x7f]/', '', $filename);
$filename = addcslashes($filename, '"\\');

But with testing, since I could easily have that preg_replace() syntax wrong.

This revision now requires changes to proceed.May 7 2016, 12:58 PM
This revision is now accepted and ready to land.May 7 2016, 1:14 PM