Item14069: Attach filename parameter needs further sanitization.

The attach operation's filename parameter is vulnerable to a variety of cross-site scripting attacks due to inadequate escaping of HTML characters. As an example, this URL,

   http://wiki.example.com/bin/attach/Main/WebHome?filename=%22%3e%3c%73%43%72%49%70%54%3e%61%6c%65%72%74%28%36%36%35%36%31%29%3c%2f%73%43%72%49%70%54%3e

will cause a Javascript alert pop-up to appear. This is because the dencoded value of the filename parameter is

    "><sCrIpT>alert(66561)</sCrIpT>

The source of the problem appears to be in core/lib/Foswiki/UI/Attach.pm, within the attach() function. My own testing confirms that applying Foswiki::entityEncode() to the filename argument value before rendering to HTML corrects this issue.

I note that there is a lengthy discussion on the wiki about suspicious characters appearing in filenames (RejectDontRenameBadFileUploads). The conclusion was to accept all non-space and non-slash characters and to encode the names appropriately upon display. If this philosophy is extended to attachment comments and filepaths, which also are user-generated, then these should also get encoded on display as part of the attach operation.

MichaelDaum noted the following:

• one needs to have edit rights on a topic to access its /bin/attach path, i.e. a guest can't access it without an intermediate login dialog.
• vulnerability does not manifest with TopicInteractionPlugin installed as it uses a different attachment dialog.
• The SecurityHeadersPlugin mitigates this vulnerability for browsers that honor XSS detection requests; some detect XSS automatically. Notably, Firefox does not, out of the box (see discussion on foswiki-security).

-- RayPlante - 12 May 2016

VadimBelman noted that he was unable to rename attachments with underscores, and tracked the issue down to the commit on this task.

Unfortunately, the simple fix isn't going to work. The problem is that the SkinTemplateTokens FILENAME and FILEPATH are used both in URLs, and in displayed text. Entity encoding then breaks access to filenames that contain encoded entities. When the encoded name is submitted as a URL parameter, the file is "not found". So with your fix, it's not possible to rename or delete an existing attachment such as file_with_underscore.

I've tried a few things, and discussed the issue with VadimBelman. We decided that the best way to handle this is to split the involved tokens into two tokens, one entity encoded, and the other URL encoded. Unfortunately this breaks compatibility with custom skins. But it does seem to work and has the added benefit of supporting filenames with any characters including currently prohibited characters like < and >

FILENAME	Entity encoded filename
FILENAME_URL	URL Encoded filename
FILEPATH	Entity encoded file path
FILEPATH_URL	URL encoded file path
NEW_FILENAME	Entity encoded filename for rename
NEW_FILENAME_URL	URL Encoded filename for rename

In addition to the template changes, This also requires changes to Foswiki::UI::Rename. Proposed patch is attached for further review.

• Item14069.patch: Proposed fix.

-- Main.GeorgeClark - 14 Oct 2016 - 02:07

Why not wrap the use of %FILENAME into an %ENCODE instead of inventing an encoded filename?

-- MichaelDaum - 14 Oct 2016

I did try that as my first attempt. Every instance of FILENAME was wrapped either in ENCODE{"FILENAME" type="entity"} or ENCODE{"FILENAME" type="url"}. I'm not sure what I was doing wrong, but no matter what I did, the alert still kept popping up. After failing with that, we came up with the split tokens. I can try again, but I was quite unsuccessful.

-- GeorgeClark - 14 Oct 2016

I see this:

%ENCODE{%FILENAME%}%

Try this instead:

%ENCODE{"%FILENAME%"}%

(note the quotes)

-- MichaelDaum - 14 Oct 2016

Found part of the problem. The rendering of the attachnew / attachagain template called expandMacros() and expandTML() before it substituted in the FILEPATH, so encode had already been expanded.

I noticed the missing quotes too. But the bad expansion order was also messing things up.

-- GeorgeClark - 14 Oct 2016

That was the problem. It wasn't possible to clean up FILEPATH using %ENCODE in the template because of the rendering order in UI::Attach. With this fixed I've been able to fix the issue. I have no idea why the macros were being expanded before all of the template tokens had been substituted.

-- GeorgeClark - 14 Oct 2016

Fix checked into Release02x01 branch. distro:7eaea2c550a3 Someone else please verify? Unit tests all pass.

-- GeorgeClark - 14 Oct 2016

Merged into master and Item13897 branches. xgettext run - there were template MAKETEXT changes.

-- GeorgeClark - 14 Oct 2016

Setting to waiting for release.

-- GeorgeClark - 10 Nov 2016

Commenting is disabled while not logged in