Project

General

Profile

Bug #6154

Notices are displaying raw html (interpreted by the browser), potential XSS vector

Added by Jim Pingle over 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Category:
Web Interface
Target version:
Start date:
04/14/2016
Due date:
% Done:

0%

Estimated time:
Affected Version:
2.3
Affected Architecture:

Description

The text of a notice is displayed as passed, presumably to allow for links in the notice text. At least one place that files notices -- the AutoConfigBackup package -- is passing through raw HTML from a server response, which breaks the display and is a potential XSS vector.

Fixing ACB may be good enough, though it still seems a bit dangerous, maybe only allow certain tags like "a" to show through, and strip out any script/onload/etc? Needs some thought.

Associated revisions

Revision e392cc2b (diff)
Added by Chris Buechler over 3 years ago

Store notices safely to prevent potential XSS when notices are displayed locally or by remote systems where they're shipped. Ticket #6154

Revision 78012791 (diff)
Added by Chris Buechler over 3 years ago

Store notices safely to prevent potential XSS when notices are displayed locally or by remote systems where they're shipped. Ticket #6154

Revision e4710ed5 (diff)
Added by Chris Buechler over 3 years ago

Sanitize notice output here as well. Ticket #6154

Revision 0f1304ee (diff)
Added by Chris Buechler over 3 years ago

Sanitize notice output here as well. Ticket #6154

History

#1 Updated by Steve Beaver over 3 years ago

It seems like in addition to fixing ACB, it would be good to sanitize notices as you suggest. A sanitiize_text() function might be useful elsewhere and could simple zap strings such as script, onload, onclick, onchange etc.

#2 Updated by Steve Beaver over 3 years ago

  • Priority changed from High to Normal

Lowered priority since JimP has taken care of the immediate problem.

#3 Updated by Chris Buechler over 3 years ago

ACB is good now. But head.inc probably should sanitize $id and $notice unconditionally. Any reason we couldn't always run those two through htmlspecialchars? I don't see anything using links within $notice, and even if they are that's probably not a great idea. That'd prevent notices from breaking the GUI in addition to security benefits.

#4 Updated by Jim Pingle over 3 years ago

Changing ACB directly was the quickest way to get a fix for everyone without updating pfSense itself, but looking through the source at uses of file_notice in deeper detail, doing it for all notices may be fine. Initially I thought I saw some place that relied on using HTML inside the message, forming a link or similar, but it looks like maybe all of that has shifted to using the $url parameter when filing a notice.

So we could probably do with an htmlspecialchars() on $notice['notice'] at source:src/usr/local/www/head.inc#L671 and for ACB on RELENG_2_3 we could revert or change the htmlspecialchars() it does to avoid having it done twice.

As for id, as long as htmlspecialchars() is run only on $notice['id'] at source:src/usr/local/www/head.inc#L668 that should be fine.

I suppose it could also be done for both in source:src/etc/inc/notices.inc#L93 and source:src/etc/inc/notices.inc#L94 as well when filing the notice rather than on display.

#5 Updated by Chris Buechler over 3 years ago

I'd rather store it safely, as there's less chance it'll become a security issue in our code, or outside of it where people are shipping notices elsewhere. For instance, discussed here.
https://thehackerblog.com/poisoning-the-well-compromising-godaddy-customer-support-with-blind-xss/

Pushed change to run all 5 through htmlentities before storing. That doesn't appear to hurt any current usage of notices.

#6 Updated by Chris Buechler over 3 years ago

  • Status changed from Assigned to Feedback
  • Assignee changed from Steve Beaver to Chris Buechler

added sanitation of the output in head.inc as well

#7 Updated by Chris Buechler over 3 years ago

  • Status changed from Feedback to Resolved

fixed

#8 Updated by Jim Pingle over 2 years ago

  • Private changed from Yes to No

Also available in: Atom PDF