Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XSS #36

Closed
dmethvin opened this issue Apr 19, 2013 · 17 comments
Closed

XSS #36

dmethvin opened this issue Apr 19, 2013 · 17 comments
Milestone

Comments

@dmethvin
Copy link
Member

Many thanks to the blogger who announced a 0-day without giving us a heads-up courtesy. But that's what 0-days are all about I suppose. Link broken intentionally.

blog.mindedsecurity.com/2013/04/jquery-migrate-is-sink-too.html

@rwaldron
Copy link
Member

The solution they are providing actually breaks jQuery http://jsfiddle.net/rwaldron/GFdJD/ :(

@dmethvin
Copy link
Member Author

I intentionally pulled out the XSS check in that regex, thinking it wasn't needed. Now if I could only recall WHY.

@freddyb
Copy link

freddyb commented Apr 19, 2013

They are actually providing two workarounds (sic!). The first should work.

Best bet is to patch jQuery asap and not rely on workarounds.

@dmethvin
Copy link
Member Author

@freddyb The bug was fixed in jQuery 1.7. The problem is that the plugin circumvents it. There is nothing to fix in the current versions of jQuery. The fix has to be in the plugin.

@fhemberger
Copy link

@rwldrn There only was a 'return' missing inside the function: http://jsfiddle.net/GFdJD/3/
You still need to check whether this influences any other parts of the jQuery testing suite.

@rwaldron
Copy link
Member

@fhemberger yes, I know—that's why it doesn't work, why I don't want people using it, and why I made a point to document it here.

@fhemberger
Copy link

So do you have a better idea for a fix?

@mdawaffe
Copy link

I'm not sure what problems it will cause, but a temporary fix might be to add back 1.8's regex (the same one is in 1.7). It seems to reproduce 1.8's behavior, but my testing is pretty limited.

Index: jquery-migrate.js
===================================================================
--- jquery-migrate.js
+++ jquery-migrate.js
@@ -189,8 +189,8 @@
 var matched, browser,
    oldInit = jQuery.fn.init,
    oldParseJSON = jQuery.parseJSON,
-   // Note this does NOT include the #9521 XSS fix from 1.7!
-   rquickExpr = /^(?:[^<]*(<[\w\W]+>)[^>]*|#([\w\-]*))$/;
+   // https://github.com/jquery/jquery-migrate/issues/36
+   rquickExpr = /^(?:[^#<]*(<[\w\W]+>)[^>]*$|#([\w\-]*)$)/;

 // $(html) "looks like html" rule change
 jQuery.fn.init = function( selector, context, rootjQuery ) {

Alternatively, the following implements something close to 1.9's behavior: http://bugs.jquery.com/ticket/11290

// https://github.com/jquery/jquery-migrate/issues/36
window.jQuery = (function( jQ ) {
    function jQueryFixed( a ) {
        var args = jQ.makeArray( arguments );
        a = jQ.trim( a );
        if ( "string" === typeof a && a.charAt( 0 ) !== "<" ) {
            if ( a.indexOf( "<" ) > -1 ) {
                jQ.find.error( a );
            }
        }
        return jQ.apply( null, args );
    }
    jQ.extend( jQueryFixed, jQ );
    return jQueryFixed;
})(jQuery);

@rwaldron
Copy link
Member

The whole thing can be avoided by not using the migrate plugin.

@dmethvin
Copy link
Member Author

Input welcome on this...before we ship if possible.

@slubowsky
Copy link

Sorry I didn't try before you shipped, but this fix broke code that has worked in every version of jQuery since probably 1.4 (including 2.0.0 with migrate plugin 1.1.1). I tried to come up with a reduced case but in the time I was able to spend couldn't reproduce it (it actually seemed to work in a stand alone test case but fails for me in my real use case every time) so I am not going to go ahead and open a bug report. What I find is that I am now getting an error on line 223 of jQuery-migrate-1.2.0.js
Uncaught TypeError: Cannot read property '1' of null
If I switch back to migrate plugin 1.1.1 it works again. The string I am passing is kind of strange (I get it dynamically from a third party and actually never examined the contents before) and perhaps arguably shouldn't work, but it always worked until now. An example of the kind of string I am passing is:

<div><h3><span id="name"></span></h3><div id="description" class="desc"><html xmlns:fo="http://www.w3.org/1999/XSL/Format"><body leftmargin="0" topmargin="0" bottommargin="0" rightmargin="0"><font face="Arial"><table border="1" width="300" cellpadding="5" cellspacing="0"><tr bgcolor="#9CBCE2"><th width="50&#37;" align="left">Field Name</th><th width="50&#37;" align="left">Field Value</th></tr>....MORE ROWS HERE ....</table></font></body></html></div><div class="zoom"><span id="zoom" class="button">zoom to item</span></div></div>

@dmethvin
Copy link
Member Author

dmethvin commented May 2, 2013

Yeah, I oversimplified the regex. The # in the color is being detected so it is a bug.

If you are getting that from a third party you should be using $.parseHTML() explicitly and telling it not to run scripts. But also that embedded in that HTML is a full document (with an embedded html tag) so it's anyone's guess as to what will come out of invalid markup.

@dmethvin
Copy link
Member Author

dmethvin commented May 2, 2013

Opened gh-38.

@cheald
Copy link

cheald commented Aug 26, 2013

I have a proof-of-concept of an outstanding jquery-migrate exploit in this vein. I can't find an appropriate place to report it privately - how do you all prefer to do that?

Please feel free to hit me at cheald at gmail directly. Thanks!

@mgol
Copy link
Member

mgol commented Aug 26, 2013

@cheald You can ping @dmethvin on #jquery-dev in a private message or you can send a mail to him. Thanks!

@t-ashula
Copy link

the issue still remains in v1.2.1
http://jsbin.com/UQEgAsO/3

@cheald
Copy link

cheald commented Nov 19, 2013

@t-ashula The response was that this is working as intended. See my writeup here: https://www.coffeepowered.net/2013/08/26/jquery-migrate-xss/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants