Page MenuHomePhabricator

XSS in erwin85 relatedchanges
Closed, ResolvedPublic

Description

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 26 2018, 11:00 PM
valerio.bozzolan set Security to Software security bug.Mar 26 2018, 11:01 PM
valerio.bozzolan added a project: Security.
valerio.bozzolan changed the visibility from "Public (No Login Required)" to "Custom Policy".

adding tool standards committee

@Erwin hasn't edited their home wiki in two years.

CC'ing other maintainers.

Given the security nature of the problem I'm fine with giving permit to WMF security team to have access and fix the problem or if none of maintainers hasn't answered for, let's say, one day. I volunteer to fix it.

I can give you access if you want.

eranroz added a comment.EditedApr 2 2018, 5:09 PM

I think in /data/project/erwin85/public_html/inc/errorhandler.inc.php

print_r($errcontext,true)

should be replaced to

print_r(array_map("htmlspecialchars", $errcontext),true)
Alphos added a comment.Apr 2 2018, 5:45 PM

What do you think of htmlentities over htmlspecialchars ?

And assuming a recent enough version of PHP (which, duh) :

print_r( // or var_dump instead of print_r, pretty please ? I strongly advise against print_r, which can give somewhat misleading information in certain situations
    array_map(
        function( $x ) { return htmlentities( $x, ENT_QUOTES | ENT_XHTML, "UTF-8", true ); }, // or htmlspecialchars instead of htmlentities ; besides, let's make sure we call the correct values for those optional params, especially the charset, so it matches the output HTML page, shall we ? :-)
        $errcontext
    ),
    true // if var_dump(), this line and the trailing comma on the previous line should be removed ; I'm also guessing the echo/print that actually outputs the return value of the print_r() has to be removed
);

That's off the top of my head (which isn't what it used to be), and without having looked at the code, but it should help a bit :-)

Honestly, i would suggest using htmlspecialchars over htmlentities. Htmlentities does a whole bunch of really pointless stuff.

Alphos added a comment.Apr 2 2018, 9:43 PM

I'm really OK with both ; but I'd rather leave the lambda function (instead of a plain string callback) in order to keep the parameters set to these explicit values in the call, considering the (X)HTML headers of the output page, if you don't mind.

TheDJ added a comment.Apr 3 2018, 9:50 AM
if($display) {
                        echo '<p><span style="font-weight:bold">' . htmlspecialchars( $type ) . ':</span> ' . htmlspecialchars( $error_msg_html ) . '</p>';
                }

I escaped everything that gets thrown to html.

I also disabled most of the tools. I think i'm gonna delete all of it soon, it seems almost all tools are broken, and for most of them are alternatives. It's time to wind these things down.

I escaped everything that gets thrown to html.

Can I push the change to the public repository? See the git log if you want.

Most of the recursion tools work and are still needed, AFAIK. I'm just not sure any steward or SMWT user is using xwiki.php. Better open a separate task for those deprecations.

Bawolff closed this task as Resolved.Jul 10 2018, 12:14 AM
Bawolff claimed this task.

I escaped everything that gets thrown to html.

Can I push the change to the public repository? See the git log if you want.

Up to the owner of the tool to decide that sort of policy - but for a toolforge thing, I think its probably easiest.

Anyways, this looks fixed, so closing and making public.

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Jul 10 2018, 12:14 AM