Fix XSS in /respond.php

[Zemnmez]

Tripped over this one. By no means even slightly a complete pentest. Had a little look over your code, your url generating code is strange; URL shortening algorithms work by mapping large indexes to short strings, a more correct algorithm is to map your number onto a high base dictated by the set of acceptable characters. Here is an implementation in PHP:

<?PHP
$d = 'abcdefghijklmnopqrtsuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_.~';
$dl = strlen($d);

function short($i) {
    global $d, $dl;
    if ($i == 0) return $d[0];

    $out = "";
    while($i != 0) {
        $rem = $i % $dl;
        $i = floor($i / $dl);
        $out .= $d[$rem];
    }
    return $out;
}

function unshort($s) {
    global $d, $dl;
    $o = 0;
    foreach (str_split($s) as $pos => $ch) {
        $o += strpos($d, $ch) * pow($dl, $pos);
    }
    return $o;
}

1538 → Mu Mu → 1358

[NuckChorris]

This isn't an XSS attack and it doesn't solve what is a real attack, more on this when I get to my laptop

[NuckChorris]

Okay, basically, we know our URL generating is really strange, but it's a necessity. Believe me, I want to change it, but as of right now this is the best we've got. It's not a security flaw or anything, though.

Basically, we pick 6 random letters from a-z, there's no unsafety or anything. The unsafest part of it is reattaching the file extension but it probably also isn't an issue since we reconnect everything past the last dot: there's no way to use ../ in the extension, since . is the delimiter already.

Your patch does literally nothing, except needlessly change the algorithm used to generate the filename and avoid what is potentially a race condition (which is cool, but not really an issue at this scale)

Now, don't get me wrong, the plan is definitely to switch to base36(id++) but that will occur when we get a chance to convert Pomf to use X-Sendfile. Oh, and you didn't need to write your own base conversion, PHP's standard library ships with base_convert, which, while it fails on larger integers, would suffice for these purposes.

Also, what the fuck is this patch? You just make it provide a header of Content-Type: application/json, which is great, but that's not at all what the commit says you did

[Zemnmez]

You have misunderstood my pull, which is my fault. The pull itself fixes an XSS exploit in your site. The description is advice on how to construct the shortened URLs. Also for securing uploads themselves you want to set Content-Disposition: attachment and prevent Java applet uploads, since you mentioned X-Sendfile.

[NuckChorris]

mmm I still don't think we're actually vulnerable to the attack you're trying to mitigate, but it's still a harmless fix no less.

That being said, I'm fairly certain @nokonoko has not been committing his changes, so I can't really say "I'll merge this" until he actually does that :/

[Zemnmez]

I am well aware what XSS is. By not setting the correct Content-Type, your JSON payload will be interpreted as HTML by the browser. Since the JSON encoder only escapes for JSON, it's trivial to inject arbitrary HTML into the response body and thus gain client side code execution.

[Zemnmez]

Also, CSRF tokens are needed ;¬)