search

emoose / openhotel

some node.js/socket.io javascript tile thing
Other
5 stars 6 forks source link

XSS vulnerability #13

Closed loadletter closed 10 years ago

loadletter commented 10 years ago

Putting 

<script>alert("test");</script>

in the namefield causes it to be executed

diggles commented 10 years ago

this must be a regression, I have seen people try this and it not working. I guess as a rule all user input should be sanitized server-side before sending updates to clients

breaks commented 10 years ago

I've seen these attempts fail before too, but looks like they work now

I think user submitted usernames might be getting passed straight through at https://github.com/emoose/openhotel/blob/master/app.js#L784

Perhaps running usernames (and other user submitted strings) through something like

function escapeString(s)
{
            // Quotes
    return s.replace(/\"/g, '&quot;')
            .replace(/\'/g, '&#39;')
            .replace(/\`/g, '&#96;')

            // Symbols
            .replace(/\&/g, '&amp;')
            .replace(/\,/g, '&#44;')
            .replace(/\;/g, '&#59;')
            .replace(/\^/g, '&#94;')
            .replace(/\*/g, '&#42;')
            .replace(/\!/g, '&#33;')
            .replace(/\?/g, '&#63;')
            .replace(/\@/g, '&#64;')
            .replace(/\$/g, '&#36;')
            .replace(/\%/g, '&#37;')
            .replace(/\=/g, '&#61;')
            .replace(/\+/g, '&#43;')
            .replace(/\-/g, '&#45;')
            .replace(/\\/g, '&#92;')
            .replace(/\//g, '&#47;')
            .replace(/\|/g, '&#124;')
            .replace(/\ /g, '&nbsp;') // Space character

            // Brackets
            .replace(/\</g, '&lt;')
            .replace(/\>/g, '&gt;')
            .replace(/\{/g, '&#123;')
            .replace(/\}/g, '&#125;')
            .replace(/\[/g, '&#91;')
            .replace(/\]/g, '&#93;')
            .replace(/\(/g, '&#40;')
            .replace(/\)/g, '&#41;');
}

could do the trick? (might want to double check everything, I think I got the escape codes correct)

I recall before that user strings used to be whitelisted to only alphanumerics, not sure what happened to that.

breaks commented 10 years ago

87dc73cb4d11275c87d8bf608ac0ab2ee8278aef seems to have fixed it

emoose commented 10 years ago

Yeah it seems fixed for now, not sure if the way we're doing it is the best though, I'm sure I had problems with it before...

Well I'll close this since the vuln mentioned in OP is fixed, if anyone spots any other XSS problems just make a new issue