Page MenuHomePhabricator

Language::commaList (and friends) causing several taint-check false positives
Closed, ResolvedPublic

Description

T250338#6170464 is (partially) an example. The method consists of:

public function commaList( array $list ) {
	return implode(
		wfMessage( 'comma-separator' )->inLanguage( $this )->escaped(),
		$list
	);
}

taint-check correctly infers that the return value of the function is escaped (because it contains an escaped part), hence trying to further escape the result will trigger an issue. However, $list may very well be tainted, hence it makes sense to escape the return value sometimes. In order for the double escape to be noticeable, one would have to use raw html in the 'comma-separator' message, which is unlikely.

I honestly don't have a nice solution for this. It's unclear whether the retval of commaList is intended for use in HTML or not. Perhaps there should be two different methods using different escaping levels. Alternatively, we might just annotate commaList as returning a non-escaped value, assuming that no-one will ever use raw HTML in the comma-separator message.

Event Timeline

For use in messages there is also Message::listParam( 'comma' ) which can delay the parsing and maybe help (or not).

I would expect that the caller of commaList has to escape also all items or you can use Message::listParam( 'comma' ) for messages where it is unknown which type the message is used (parse/escaped/plain). But that double escaping problem is not solved by it.

Change 902363 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] language: Annotate list() methods as preserving taintedness

https://gerrit.wikimedia.org/r/902363

The patch above doesn't really fix this issue, but at least it lets taint-check know that the return value can be tainted (as well as escaped at the same time).

Also adding MediaWiki-Internationalization -- this is not an issue in taint-check, it's just taint-check getting understandably confused by a core method with inconsistent escaping. This should be fixed in core.

Change 902363 merged by jenkins-bot:

[mediawiki/core@master] language: Annotate list() methods as preserving taintedness

https://gerrit.wikimedia.org/r/902363

MobileFrontend and Translate extension now failing on phan, please have a look.

MobileFrontend

includes/specials/SpecialMobileDiff.php:361 SecurityCheck-XSS Calling method \OutputPage::addHTML() in \SpecialMobileDiff::showFooter that outputs using tainted argument #1. (Caused by: Builtin-\OutputPage::addHTML) (Caused by: includes/specials/SpecialMobileDiff.php +419; includes/specials/SpecialMobileDiff.php +416; ../../includes/user/UserGroupMembership.php +152; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +161; ../../includes/user/UserGroupMembership.php +132; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +123; ../../includes/language/Language.php +2577; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +134; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +123; ../../includes/language/Language.php +2577; Builtin-\Message::text; ../../includes/language/Language.php +3454)

Translate

utils/TranslateLogFormatter.php:40 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere

CentralAuth

includes/GlobalRename/GlobalRenameLogFormatter.php:21 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/GlobalRename/GlobalRenameLogFormatter.php:24 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/GlobalRename/GlobalRenameLogFormatter.php:27 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/LogFormatter/GlobalUserMergeLogFormatter.php:19 SecurityCheck-XSS Calling method \Message::rawParam() in \MediaWiki\Extension\CentralAuth\LogFormatter\GlobalUserMergeLogFormatter::extractParameters that outputs using tainted argument #1. (Caused by: ../../includes/language/Message.php +1135) (Caused by: includes/LogFormatter/GlobalUserMergeLogFormatter.php +16; ../../includes/logging/LogFormatter.php +564; ../../includes/logging/LogFormatter.php +546; ../../includes/logging/LogFormatter.php +670; ../../includes/logging/LogFormatter.php +632; Builtin-\Message::text; ../../includes/logging/LogFormatter.php +635; Builtin-\Message::text; ../../includes/logging/LogFormatter.php +642; ../../includes/user/User.php +1687; ../../includes/user/User.php +439; ../../includes/user/User.php +421; Builtin-\Wikimedia\Rdbms\DBConnRef::selectRow; ../../includes/user/User.php +1696; ../../includes/logging/LogFormatter.php +650; Builtin-\Message::text; ../../includes/language/Language.php +3454)
includes/LogFormatter/WikiSetLogFormatter.php:55 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/LogFormatter/WikiSetLogFormatter.php:63 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/LogFormatter/WikiSetLogFormatter.php:78 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/LogFormatter/WikiSetLogFormatter.php:89 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/LogFormatter/WikiSetLogFormatter.php:96 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere
includes/Special/SpecialGlobalGroupMembership.php:562 SecurityCheck-XSS Calling method \Message::rawParams() in \MediaWiki\Extension\CentralAuth\Special\SpecialGlobalGroupMembership::showEditUserGroupsForm that outputs using tainted argument #1. (Caused by: Builtin-\Message::rawParams) (Caused by: includes/Special/SpecialGlobalGroupMembership.php +552; includes/Special/SpecialGlobalGroupMembership.php +548; ../../includes/user/UserGroupMembership.php +152; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +161; ../../includes/user/UserGroupMembership.php +132; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +134; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; includes/Special/SpecialGlobalGroupMembership.php +547; includes/Special/SpecialGlobalGroupMembership.php +555; includes/Special/SpecialGlobalGroupMembership.php +548; ../../includes/user/UserGroupMembership.php +152; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +161; ../../includes/user/UserGroupMembership.php +132; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +134; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; includes/Special/SpecialGlobalGroupMembership.php +547; ../../includes/language/Language.php +3454) (Param is raw)
includes/Special/SpecialGlobalGroupMembership.php:562 SecurityCheck-XSS Calling method \Message::rawParams() in \MediaWiki\Extension\CentralAuth\Special\SpecialGlobalGroupMembership::showEditUserGroupsForm that outputs using tainted argument #2. (Caused by: Builtin-\Message::rawParams) (Caused by: includes/Special/SpecialGlobalGroupMembership.php +553; includes/Special/SpecialGlobalGroupMembership.php +549; ../../includes/user/UserGroupMembership.php +152; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +161; ../../includes/user/UserGroupMembership.php +132; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +134; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; includes/Special/SpecialGlobalGroupMembership.php +547; includes/Special/SpecialGlobalGroupMembership.php +556; includes/Special/SpecialGlobalGroupMembership.php +549; ../../includes/user/UserGroupMembership.php +152; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +161; ../../includes/user/UserGroupMembership.php +132; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; ../../includes/user/UserGroupMembership.php +134; ../../includes/user/UserGroupMembership.php +121; ../../includes/language/Language.php +2594; Builtin-\Message::text; includes/Special/SpecialGlobalGroupMembership.php +547; ../../includes/language/Language.php +3454) (Param is raw)

Not sure if related, but could be due to taint issues after changing taint handling.

OAuth

src/Frontend/OAuthLogFormatter.php:27 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere

Wikibase

client/includes/RecentChanges/RecentChangeFactory.php:114 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue SecurityCheck-DoubleEscaped on this line but this suppression is unused or suppressed elsewhere

matmarex assigned this task to Daimona.