Skip to content
Permalink
Browse files

Merge pull request #474 from dmethvin/fix-9521-xss-hash

Fixes #9521. Prioritize #id over <tag> to avoid XSS via location.hash.
  • Loading branch information
dmethvin committed Aug 25, 2011
2 parents 84f2908 + 749dbad commit db9e023e62c1ff5d8f21ed9868ab6878da2005e9
Showing with 20 additions and 2 deletions.
  1. +2 −2 src/core.js
  2. +18 −0 test/unit/core.js
@@ -16,8 +16,8 @@ var jQuery = function( selector, context ) {
rootjQuery,

// A simple way to check for HTML strings or ID strings
// (both of which we optimize for)
quickExpr = /^(?:[^<]*(<[\w\W]+>)[^>]*$|#([\w\-]*)$)/,
// Prioritize #id over <tag> to avoid XSS via location.hash (#9521)
quickExpr = /^(?:[^#<]*(<[\w\W]+>)[^>]*$|#([\w\-]*)$)/,

// Check if a string has a non-whitespace character in it
rnotwhite = /\S/,
@@ -467,6 +467,24 @@ test("isXMLDoc - HTML", function() {
document.body.removeChild( iframe );
});

test("XSS via location.hash", function() {
expect(1);

stop();
jQuery._check9521 = function(x){
ok( x, "script called from #id-like selector with inline handler" );
jQuery("#check9521").remove();
delete jQuery._check9521;
start();
};
try {
// This throws an error because it's processed like an id
jQuery( '#<img id="check9521" src="no-such-.gif" onerror="jQuery._check9521(false)">' ).appendTo("#qunit-fixture");
} catch (err) {
jQuery._check9521(true);
};
});

if ( !isLocal ) {
test("isXMLDoc - XML", function() {
expect(3);

7 comments on commit db9e023

@mala

This comment has been minimized.

Copy link

@mala mala replied Aug 26, 2011

Hi dmethvin,

quickExpr = /^(?:\s*(<[\w\W]+>)[^>]*$|#([\w\-]*)$)/

is more better.

var user_input = "<img>" $("a[class=" + user_input + "]") // create img element

see http://bugs.jquery.com/ticket/9521#comment:3

@dmethvin

This comment has been minimized.

Copy link
Member Author

@dmethvin dmethvin replied Aug 26, 2011

Agreed but it is also more likely to cause a regression. For now I would like to stay with this patch, which only addresses the use of location.hash in $().

@Krinkle

This comment has been minimized.

Copy link
Member

@Krinkle Krinkle replied Sep 4, 2011

Why does quickExpr check for <tag> at all ?

Since a while now, $.fn.init checks something else, before it looks for quickExpr. The if statement that checks if .charAt(0) is < and the last character >, and if so, prepare for a HTML-match.

Afaik, there is no case were $() should accept HTML that doesn't start with < and ends with >. So, with the charAt-check for HTML in place, can't quickExpr be simplified to only match strings that start with a #, and pass on to document.getElementById ?

I'm probably missing something obvious here..

@dmethvin

This comment has been minimized.

Copy link
Member Author

@dmethvin dmethvin replied Sep 7, 2011

@Krinkle, it wouldn't surprise me if this code has gotten out of whack because of progressive changes. Could you open a ticket with your observations and reference this pull request? A patch and some more test cases would be good as well, maybe we can get this into 1.7.

@fr34k8

This comment has been minimized.

Copy link

@fr34k8 fr34k8 replied Jan 21, 2015

threat is open for a while but "anything" discovered with #1.10.1 ?!

@dmethvin

This comment has been minimized.

Copy link
Member Author

@dmethvin dmethvin replied Jan 21, 2015

Not sure what you're asking about, but sure that this is the wrong place to ask.

@fr34k8

This comment has been minimized.

Copy link

@fr34k8 fr34k8 replied Jan 21, 2015

Agreed

Please sign in to comment.
You can’t perform that action at this time.