Page MenuHomePhabricator

XSS vulnerability scanner false positives
Open, LowPublic

Description

Several API formats allow the inclusion of arbitrary HTML in responses, and XSS is prevented only by the Content-Type header. This causes a false positive in McAfee Secure and possibly other vulnerability scanners. McAfee can be petitioned to recognise such scan responses as false positives, but this process is difficult and unreliable, and has to be repeated regularly.

I would like to have a configurable "scan-safe" mode, off by default, which will disable the following output formats: php, txt, dbg and dump. The documentation should be updated to indicate that these formats are not preferred and will not work on all installations.

Additionally, json_encode() should use the JSON_HEX_TAG option where it is available (i.e. PHP 5.3.0+), regardless of configuration. In scan-safe mode in PHP<5.3, a JSON encoder which does not pass through "<" characters should be used.


Version: 1.20.x
Severity: normal

Details

Reference
bz34257

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 12:15 AM
bzimport set Reference to bz34257.
bzimport added a subscriber: Unknown Object (MLST).

(In reply to comment #0)

I would like to have a configurable "scan-safe" mode, off by default, which
will disable the following output formats: php, txt, dbg and dump. The
documentation should be updated to indicate that these formats are not
preferred and will not work on all installations.

With Roan's recommendation, I'd actually go a bit further and recommend to end-users to only use JSON.

(In reply to comment #1)

(In reply to comment #0)

I would like to have a configurable "scan-safe" mode, off by default, which
will disable the following output formats: php, txt, dbg and dump. The
documentation should be updated to indicate that these formats are not
preferred and will not work on all installations.

That can be done for sure.

With Roan's recommendation, I'd actually go a bit further and recommend to
end-users to only use JSON.

Yeah I'd really like people to only use JSON, and if I were to start rewriting the API today I'd make it JSON-only.

XML is much easier to parse than JSON, in reality if not in theory. I'm not sure, though, that we should worry about McAfee, if the AV writers can't do their job then they will loose market share. I've lost count of the systems that have been fixed by removing shop-installed AV software. And one AV package has even detected /strawberry/bin/perl.exe as "an unknown threat".

(In reply to comment #4)

XML is much easier to parse than JSON, in reality if not in theory. I'm not
sure, though, that we should worry about McAfee, if the AV writers can't do
their job then they will loose market share. I've lost count of the systems
that have been fixed by removing shop-installed AV software. And one AV
package has even detected /strawberry/bin/perl.exe as "an unknown threat".

These things came up from the fundraising quarterly PCI scans

https://rt.wikimedia.org/Ticket/Display.html?id=2374 for anyone who has access and wants to see

alext wrote:

Hello, I just want to share this with the Mediawiki community for reference.

A vulnerability scan of a Mediawiki 1.18.1 installation was falsely flagged as positive on an XSS injection attempt.

The injection url looks something like "https://site-address/load.php?debug=false&lang=en&modules=mediawiki.legacy.common..." and was replaced with "https://site-address/load.php?debug=false&lang=<script>somescript&modules=mediawiki.legacy.common..."

Mediawiki correctly issued a message saying that "<script>somescript" is an invalid language code but the vulnerability scanner falsely interpreted the echoed message as a positive injection.

(In reply to comment #6)

Mediawiki correctly issued a message saying that "<script>somescript" is an
invalid language code but the vulnerability scanner falsely interpreted the
echoed message as a positive injection.

Confirmed that this is a false positive. The Content-Type of the response is text/javascript, the injected text is wrapped in a comment, and injection of "*/" is protected against. Tested in Chromium and IE.

Anomie added subscribers: dpatrick, Bawolff, Anomie.

@dpatrick, @Bawolff: Would a "scan-safe" configuration parameter that disables some formats and changes the behavior of others be useful? Or would it just cause more trouble since scanners would be running against different output from what is actually used by real MediaWiki installations and people who file bogus bugs after using broken scanners wouldn't use the "scan-safe" option anyway?

Anomie added subscribers: EyalZ, matmarex, Aklapper.

@dpatrick, @Bawolff: Would a "scan-safe" configuration parameter that disables some formats and changes the behavior of others be useful? Or would it just cause more trouble since scanners would be running against different output from what is actually used by real MediaWiki installations and people who file bogus bugs after using broken scanners wouldn't use the "scan-safe" option anyway?

I feel like that's kind of cheating. What's the point of scanning something of we change the behaviour of the thing to be different for the scanner then in real life.

OTOH I have no involvement/knowledge of the scanning that fundraising does.

I personally would be more inclined to resolve this declined than to change the behaviour of the api.

Additionally, json_encode() should use the JSON_HEX_TAG option where it is available (i.e. PHP 5.3.0+), regardless of configuration. In scan-safe mode in PHP<5.3, a JSON encoder which does not pass through "<" characters should be used.

I guess that'd be an ok haredening measure if it didn't cause any problems - but on the other hand the api is often used to return html encoded in json, and that'd make things ugly. So I'm not sure that people would like this.

Anomie added a subscriber: Jhuff05.

Additionally, json_encode() should use the JSON_HEX_TAG option where it is available (i.e. PHP 5.3.0+), regardless of configuration.

Yep, and our FormatJson class uses JSON_HEX_TAG by default (at least it does now). Resolved?