Potential security thing for hashes #2071

Closed
nwertzberger opened this Issue Jan 4, 2013 · 13 comments

Projects

None yet

6 participants

@nwertzberger

in the code for routes, a raw string is passed to _updateHash on line 1242, which can cause security issues according to certain automated scanners. Here's a potential solution that I am using, which quiets down these scanners:

// Update the hash location, either replacing the current entry, or adding
// a new one to the browser history.
_updateHash: function(location, fragment, replace) {
  if (replace) {
    var href = location.href.replace(/(javascript:|#).*$/, '');
    location.replace(href + '#' + fragment);
  } else {
    // Some browsers require that `hash` contains a leading #.
    location.hash = '#' + encodeURIComponent(fragment);
  }
}
@jashkenas
Owner

Pinging @braddunbar about the encodeURIComponent addition here.

@braddunbar
Collaborator

My initial response would be that the user should be sanitizing fragments before passing them to Backbone since it's such a complex problem. Further, if users are already encoding fragments before passing them to navigate, this could cause problems with double encoding.

@nwertzberger Just to be clear, does this prevent a particular type of exploit that you've run into or is it just to quiet the automated warning?

@nwertzberger

This would be for an automated warning. I don't know of a way to hijack the hash tag to make you go to another site or anything given the current code path. I can see issues with insecure code using the source hash in a tag without escaping it, but yeah, I'm not seeing a real bug right in backbone. Of course, I'm not a security expert.

The errors were from completely backbone stack traces for what looks like writing page history either on start, checkUrl, or navigate (look for this.getFragment as the source). If it gets encoded at any point during these operations, It would stop whining. I assume that you could sneak one in there if it were a purely backbone path without having any compatibility issues.

@deanapeterson

I second the insertion of encoding.

I am working on a project that requires passing security scans for all code put into production. Backbone has been flagged.

Thanks, @nwertzberger, for the patch.

@braddunbar
Collaborator

@nwertzberger @deanapeterson Would you mind pointing me towards the analysis tools you're using? Encoding and decoding fragments in Backbone.History has caused quite a few issues in the past and I'd like to make sure that we're addressing a particular exploit that we fully understand before patching it.

@nwertzberger

Fortify is the tool.

I think this is because others might grab location.hash (jQuery users) and have been the victims of XSS that way. The pathway taken to get there seems to be unlikely to occur, as the sources are different ways to grab a base url name if that method is supported. It looks like the hash way is only used if you couldn't use a newer method. it's suggesting that the following is a dangerous idea (i know that this does nothing useful).

this.iframe.location.hash = '#' + this.location.pathname;
this.location.hash = "#" + this.location.pathname;

Especially because you are using .hash part, i don't understand how one could, for example, redirect to a nefarious page. I'd be much more worried about the if (replace) logic.

@deanapeterson

Same here. HP Fortify.

@deanapeterson

This issue is blocker for me. In my context, the company has huge security restrictions and if Fortify deems an issue as critical (like this) it stops deployment.

I'm gonna work with @nwertzberger solution, and see what happens. I'll post what ever solutions I discover or create here.

@braddunbar
Collaborator

Ok, here is what I've come up with after some research. The exploit in question seems to rely on something like the following:

var fragment = location.hash.slice(1);
if (!$('a[name|="' + fragment + '"]')[0]) {
  // ...
}

Using a fragment value of " <img src=nonexistent onerror=alert(1)>, an attacker can execute arbitrary code of their choosing. The solution is to use jQuery.find (or similar), which doesn't evaluate html and thus prevents arbitrary code execution from the hash. This is certainly a valid attack vector, but I don't see how encoding the hash value will prevent it since malicious fragment values would likely come from a user supplied url or a phishing attack, neither of which are passed through History#_updateHash. A more complete solution is to avoid using methods that evaluate html with arbitrary user input.

I don't think that the proposed solution will prevent the attack and we certainly shouldn't make the change just to silence security warnings so I'm going to close this one for now. If I'm wrong about the exploit or there is another separate exploit that I missed please let me know and we can reopen.

@braddunbar braddunbar closed this Jan 14, 2013
@simoncb

@deanapeterson we are also using hp fortify and the same issues with backbone are being flagged. We are in a similar situation where critical issues in fortify need to be addressed in order to be compliant.

I was wondering what you ended up doing in order to pass the fortify scans?

@nwertzberger

The initial suggestion is what I have used to pass security scans when this came up. We have since moved to AngularJS.

@deanapeterson
@joevennix

This check doesn't actually seem useful, since it lacks the case-insensitive regex modifier. JaVaScRiPt: URLs work just as well as javascript:.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment