[MediaWiki-commits] [Gerrit] mediawiki/core[master]: SECURITY: Ensure Message::rawParams can't lead to XSS
Reedy has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/391451 )
Change subject: SECURITY: Ensure Message::rawParams can't lead to XSS
..
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
---
M includes/Message.php
1 file changed, 20 insertions(+), 2 deletions(-)
Approvals:
Reedy: Verified; Looks good to me, approved
diff --git a/includes/Message.php b/includes/Message.php
index 2a55d0e..3b2f3cc 100644
--- a/includes/Message.php
+++ b/includes/Message.php
@@ -1123,11 +1123,29 @@
* @return string
*/
protected function replaceParameters( $message, $type = 'before',
$format ) {
+ // A temporary marker for $1 parameters that is only valid
+ // in non-attribute contexts. However if the entire message is
escaped
+ // then we don't want to use it because it will be mangled in
all contexts
+ // and its unnessary as ->escaped() messages aren't html.
+ $marker = $format === self::FORMAT_ESCAPED ? '$' : '$\'"';
$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 )] =
$marker . ( $n + 1 );
+ }
+ } else {
+ if ( $paramType === 'after' ) {
+ $replacementKeys[$marker . ( $n + 1 )]
= $value;
+ }
}
}
$message = strtr( $message, $replacementKeys );
--
To view, visit https://gerrit.wikimedia.org/r/391451
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If83aec01b20e414f9c92be894f145d7df2974866
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Reedy
Gerrit-Reviewer: Brian Wolff
Gerrit-Reviewer: Reedy
Gerrit-Reviewer: jenkins-bot <>
___
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
[MediaWiki-commits] [Gerrit] mediawiki/core[master]: SECURITY: Ensure Message::rawParams can't lead to XSS
Reedy has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/391451 )
Change subject: SECURITY: Ensure Message::rawParams can't lead to XSS
..
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
---
M includes/Message.php
1 file changed, 20 insertions(+), 2 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/51/391451/1
diff --git a/includes/Message.php b/includes/Message.php
index 2a55d0e..3b2f3cc 100644
--- a/includes/Message.php
+++ b/includes/Message.php
@@ -1123,11 +1123,29 @@
* @return string
*/
protected function replaceParameters( $message, $type = 'before',
$format ) {
+ // A temporary marker for $1 parameters that is only valid
+ // in non-attribute contexts. However if the entire message is
escaped
+ // then we don't want to use it because it will be mangled in
all contexts
+ // and its unnessary as ->escaped() messages aren't html.
+ $marker = $format === self::FORMAT_ESCAPED ? '$' : '$\'"';
$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 )] =
$marker . ( $n + 1 );
+ }
+ } else {
+ if ( $paramType === 'after' ) {
+ $replacementKeys[$marker . ( $n + 1 )]
= $value;
+ }
}
}
$message = strtr( $message, $replacementKeys );
--
To view, visit https://gerrit.wikimedia.org/r/391451
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If83aec01b20e414f9c92be894f145d7df2974866
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Reedy
Gerrit-Reviewer: Brian Wolff
Gerrit-Reviewer: Reedy
___
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
