Bug #6154
closed
Notices are displaying raw html (interpreted by the browser), potential XSS vector
0%
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.
Updated by Anonymous over 8 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.
Updated by Anonymous over 8 years ago
- Priority changed from High to Normal
Lowered priority since JimP has taken care of the immediate problem.
Updated by Chris Buechler over 8 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.
Updated by Jim Pingle over 8 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.
Updated by Chris Buechler over 8 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.
Updated by Chris Buechler over 8 years ago
- Status changed from Assigned to Feedback
- Assignee changed from Anonymous to Chris Buechler
added sanitation of the output in head.inc as well