The following code:

tab_focus = $(window.location.hash, this).closest('.horizontaltabspane');

gets picked up by security scanners because it passes "window.location.hash" directly to JQuery. This is mitigated by the check above it which is why I'm making this an open ticket and not going through the security team process:

if (hash !== '#' && $(hash, this).length

which should require that the "window.location.hash" is actually present in the form before processing. However it would be better to escape the has before sending directly to JQuery.

We can use the variable already created:

var hash = window.location.hash.replace(/[=%;,\/]/g, "");

for this purpose. The following patch replaces:

tab_focus = $(window.location.hash, this).closest('.horizontaltabspane');

with:

tab_focus = $(hash, this).closest('.horizontaltabspane');

so that the input to the jQuery call is sanitized.

CommentFileSizeAuthor
hash-location-sanitization.diff699 bytesacouch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acouch created an issue. See original summary.

David_Rothstein’s picture

Issue tags: -Security

This was unpublished briefly while it was brought to the attention of the Drupal Security Team. However, I'm republishing it now because our review found that there is no vulnerability.

The main mitigation of this actually comes from Drupal core, which added protection for this kind of problem a while ago via https://www.drupal.org/SA-CORE-2013-001. (Without that protection, the module would be vulnerable.) It is possible that switching to the "pre-sanitized" hash variable is sufficient to deal with that, but in general my understanding is that the best way to avoid these types of vulnerabilities (and, likely, to avoid false positives on security scans) is to switch to something like $(this).find(window.location.hash) rather than $(window.location.hash, this).

David_Rothstein’s picture

Title: Protect Against XSS When Getting Hash » Avoid bad code pattern (that is picked up by security scanners) when getting hash
nils.destoop’s picture

Status: Needs review » Fixed

Thx for the patch. This was committed to the dev branch

  • zuuperman committed cf294d9 on 7.x-1.x authored by acouch
    Issue #2831815 by acouch: Avoid bad code pattern (that is picked up by...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.