Skip to content

Commit

Permalink
[#1792] Fix XSS and bug with numeric/quoted values in filters
Browse files Browse the repository at this point in the history
The problem is with the function `queryStringToJSON` that we use in
`ckan/public/base/javascript/view-filters.js` to convert the URL's query string
into a JavaScript object to then parse it.

To understand the bug, take a look at this example:

```javascript
> "filters=country:Brazil|year:2014".queryStringToJSON()
Object {filters: "country:Brazil|year:2014"}
> "filters=country:Brazil".queryStringToJSON()
Object {filters: "country:Brazil"}
> "filters=year:2014".queryStringToJSON()
Object {filters: 2014} // The correct result would be { filters: "year:2014" }
```

Looking at `queryStringToJSON` code, I found the problematic part at
https://github.com/ckan/ckan/blob/1792-filterable-resource-views/ckan/public/base/javascript/view-filters.js#L49-L57

```javascript
// ...

// Fix
key = decodeURIComponent(key);
value = decodeURIComponent(value);
try {
  // value can be converted
  value = eval(value);
} catch ( e ) {
  // value is a normal string
}

// ...
```

This code tries to `eval` the query string's values. "Normal" strings throw an
error when eval'd, which the code ignores. But unfortunately `"year:2014"`
isn't a normal string. See:

```javascript
> eval("year:2014")
2014
> year: 2014
2014
> { year: 2014 }
2014
> eval("year:2014|country:Brazil")
SyntaxError: Unexpected token :
> eval("country:Brazil")
ReferenceError: Brazil is not defined
```

We'll have the same problem if the filter value is between quotes, as in:

```javascript
> "filters=country:'Brazil'".queryStringToJSON()
Object {filters: "Brazil"}
```

The XSS issue is related to us calling `eval()` on all parameters, so some
malicious user could send a link to e.g.
`http://demo.ckan.org/?param=alert('abc')` and execute JS code on anyone that
clicks on that. This was discovered in bevry-archive/jquery-sparkle#5
  • Loading branch information
vitorbaptista committed Aug 5, 2014
1 parent dab0d74 commit 68fe124
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 6 deletions.
6 changes: 0 additions & 6 deletions ckan/public/base/javascript/view-filters.js
Expand Up @@ -49,12 +49,6 @@ String.prototype.queryStringToJSON = String.prototype.queryStringToJSON || funct
// Fix
key = decodeURIComponent(key);
value = decodeURIComponent(value);
try {
// value can be converted
value = eval(value);
} catch ( e ) {
// value is a normal string
}

// Set
// window.console.log({'key':key,'value':value}, split);
Expand Down
18 changes: 18 additions & 0 deletions ckan/public/base/test/spec/view-filters.spec.js
Expand Up @@ -56,6 +56,24 @@ describe('ckan.views.filters', function(){
filters._initialize('?filters=country:Brazil|country:Argentina');
assert.deepEqual(expectedFiltersReverse, filters.get());
});

it('should work with a single numeric filter', function() {
var expectedFilters = {
year: ['2014']
};

filters._initialize('?filters=year:2014');
assert.deepEqual(expectedFilters, filters.get());
});

it('should work with quoted filters', function() {
var expectedFilters = {
country: ['"Brazil"']
};

filters._initialize('?filters=country:"Brazil"');
assert.deepEqual(expectedFilters, filters.get());
});
});

describe('#get', function(){
Expand Down

0 comments on commit 68fe124

Please sign in to comment.