15:29:22 SimpleCaptcha/SimpleCaptcha.php:879 SecurityCheck-DoubleEscaped Calling method \RawMessage::__construct() in \SimpleCaptcha::confirmEditMerged that outputs using tainted argument #1. (Caused by: ../../includes/RawMessage.php +54; ../../includes/language/Message.php +991) (Caused by: Builtin-\Html::element)
Seen on an unrelated patch touching this repo
The relevant code is passing the return value of Html::element() (which is already HTML-escaped) to RawMessage::__construct, which indeed doesn't feel right: calling ->text() would produce escaped output, and calling ->escaped() would produce double-escaped HTML. I think it should use Html::rawElement() instead.
Patch posted. I know this is likely lower-risk for now, but let's protect out of an abundance of caution.
I'm just going to push this one through gerrit, since it's merely a double-escape and likely not exploitable. We can keep this bug private until it makes it to wmf.7 this week.
Change 736066 had a related patch set uploaded (by SBassett; author: SBassett):
[mediawiki/extensions/ConfirmEdit@master] SECURITY: Avoid double-escaping html tag contents
Hmm, so phan-taint-check didn't like Html::rawElement either:
16:19:01 SimpleCaptcha/SimpleCaptcha.php:879 SecurityCheck-DoubleEscaped Calling method \RawMessage::__construct() in \SimpleCaptcha::confirmEditMerged that outputs using tainted argument #1. (Caused by: ../../includes/RawMessage.php +54; ../../includes/language/Message.php +991) (Caused by: Builtin-\Html::rawElement)
I guess we could suppress it with the standard comment, but I don't love that phan-taint-check is unhappy with both here...
Ok, so this is interesting: phan-taint-check has both Html::element and Html::rawElement as overall escaped taints, I guess for Html::rawElement because it does apparently escape $attribs? Even if the (likely) more relevant piece there is $content, at least in this case. Anyhow, I think this needs the suppression comment for now, unless we're concerned that $attribs are _actually_ being double-escaped.
Change 736066 merged by jenkins-bot:
[mediawiki/extensions/ConfirmEdit@master] SECURITY: Avoid double-escaping html tag contents
Change 736247 had a related patch set uploaded (by SBassett; author: SBassett):
[mediawiki/extensions/ConfirmEdit@wmf/1.38.0-wmf.7] SECURITY: Avoid double-escaping html tag contents
Change 736247 merged by jenkins-bot:
[mediawiki/extensions/ConfirmEdit@wmf/1.38.0-wmf.7] SECURITY: Avoid double-escaping html tag contents
Ohhhh right, because Html::rawElement also outputs HTML, and the plugin considers it escaped (while not technically correct, I guess it's because you're not supposed to escape the return value of Html::(raw)Element).
I tested that code manually, and I think there's some special handling for RawMessage which makes it work regardless of its content. I'm not sure if this is intended or what, but I'm not going to change that.
Most importantly, I can't understand why it's wrapping the whole thing in an errorbox div. The hook errors are already inside an errorbox div, so the output is really ugly. Also, formatting the message is up to whatever is checking the Status object (for edits via the UI, EditPage.php), not to the hook handler. In fact, the hook is also fired for APIEdit requests, and the errors aren't formatted correctly. I'll push a fix shortly.
Change 736294 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):
[mediawiki/extensions/ConfirmEdit@master] Don't put HTML via RawMessage in the EditFilterMergedContent hook Status
Change 736294 merged by jenkins-bot:
[mediawiki/extensions/ConfirmEdit@master] Don't put HTML via RawMessage in the EditFilterMergedContent hook Status