Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F10720787
T176247-master.patch
Reedy (Sam Reed)
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Authored By
Reedy
Nov 10 2017, 11:18 PM
2017-11-10 23:18:00 (UTC+0)
Size
2 KB
Referenced Files
None
Subscribers
None
T176247-master.patch
View Options
From f7604d542652872b1137edbf5912d0b865433c7c Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
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 | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/includes/Message.php b/includes/Message.php
index 2a55d0ee74..3b2f3ccc7b 100644
--- a/includes/Message.php
+++ b/includes/Message.php
@@ -1123,11 +1123,29 @@ class Message implements MessageSpecifier, Serializable {
* @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 );
--
2.14.1
File Metadata
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
5081136
Default Alt Text
T176247-master.patch (2 KB)
Attached To
Mode
T168823: Tracking bug for 1.27.4/1.28.3/1.29.2 security releases
Attached
Detach File
T176247: It's possible to mangle HTML via raw message parameter expansion
Attached
Detach File
Event Timeline
Log In to Comment