From 206a3aebff8916a1ef0fed61a95eda51eea75781 Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Sun, 24 Sep 2017 00:57:05 +0000 Subject: [PATCH] SECURITY: Ensure Message::rawParams can't lead to XSS If you used wfMessage( 'foo' )->rawParams( 'bar"baz' ) there's a possibility of leading to xss, if the foo message has a $1 in an attribute, as the quote characters may end the attribute. To prevent that, we convert $1 to $'"1 for after parameters, so if any of them end up in attributes, the attribute escaping will break the parameter name, preventing substitution. This would of course break if someone intentionally inserted a raw parameter into an attribute, but that's silly and I don't think we should allow that. This is similar to the parser strip marker issue. Bug: T176247 Change-Id: If83aec01b20e414f9c92be894f145d7df2974866 --- includes/Message.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/includes/Message.php b/includes/Message.php index 0240fa7..4292260 100644 --- a/includes/Message.php +++ b/includes/Message.php @@ -1126,8 +1126,21 @@ class Message implements MessageSpecifier, Serializable { $replacementKeys = []; foreach ( $this->parameters as $n => $param ) { list( $paramType, $value ) = $this->extractParam( $param, $format ); - if ( $type === $paramType ) { - $replacementKeys['$' . ( $n + 1 )] = $value; + if ( $type === 'before' ) { + if ( $paramType === 'before' ) { + $replacementKeys['$' . ( $n + 1 )] = $value; + } else /* $paramType === 'after' */ { + // To protect against XSS from replacing parameters + // inside html attributes, we convert $1 to $'"1. + // In the event that one of the parameters ends up + // in an attribute, either the ' or the " will be + // escaped, breaking the replacement and avoiding XSS. + $replacementKeys['$' . ( $n + 1 )] = '$\'"' . ( $n + 1 ); + } + } else { + if ( $paramType === 'after' ) { + $replacementKeys['$\'"' . ( $n + 1 )] = $value; + } } } $message = strtr( $message, $replacementKeys ); -- 1.9.5 (Apple Git-50.3)