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

Security::strip_image_tags bypass #107

Closed
dsopas opened this issue May 18, 2016 · 18 comments
Closed

Security::strip_image_tags bypass #107

dsopas opened this issue May 18, 2016 · 18 comments

Comments

@dsopas
Copy link

dsopas commented May 18, 2016

Hi,

I found a security issue on your latest version.
In your security class – system/classes/Kohana/Security.php you have the following function to remove img tags from strings:

/**
 * Remove image tags from a string.
 *
 *     $str = Security::strip_image_tags($str);
 *
 * @param   string  $str    string to sanitize
 * @return  string
 */
public static function strip_image_tags($str)
{
    return preg_replace('#<img\s.*?(?:src\s*=\s*["\']?([^"\'<>\s]*)["\']?[^>]*)?>#is', '$1', $str);
}

Just by looking at that regex it's possible to see that after <img it expects a space. So to bypass this you could use:

<img/src...

PHP example:

<?php
$str = "XSS de Imagem: <img/src=x onerror=prompt(document.domain)>";
echo preg_replace('#<img\s.*?(?:src\s*=\s*["\']?([^"\'<>\s]*)["\']?[^>]*)?>#is', '$1', $str);

And it's still possible to inject a IMG on a string - in this case with a XSS vector.
Hope it helps.

Best,
David Sopas
Checkmarx.com

@enov
Copy link
Contributor

enov commented May 19, 2016

Thanks for the security report @dsopas.

Yeah, it seems it is indeed possible to inject an XSS vector, even though the img tag looks invalid HTML. I tested with a browser and the JS prompt runs.

We could tweak the regex to include a test for a slash / character. Are you aware of any additional character or ways that could still bypass the regex?

Thanks again.

@enov
Copy link
Contributor

enov commented May 19, 2016

This is what I came up with, let me know if it is OK:

#<img(?:/|\s)*[^>]*(?:src\s*=\s*["\']?([^"\'<>\s]*)["\']?[^>]*)?>#ig

@dsopas
Copy link
Author

dsopas commented May 19, 2016

Hi @enov ,

Here at Checkmarx we're improving our detection on Kohana framework and we detected that function.
It seems correct now.

Good work!

Best,
David Sopas
Checkmarx.com

@dsopas
Copy link
Author

dsopas commented May 19, 2016

@enov

It's possible to bypass again your protection. Using a special character (%0C) which I think it means a space you don't have to end the tag using >.

<img onerror=alert(1) src=x

I don't know if this will work using copy paste in Github but you can see it working on jsfiddle:
https://jsfiddle.net/emdj7ty5/1/

Using your updated code in my localhost I attached the screenshot.
imgxss

@dsopas
Copy link
Author

dsopas commented May 19, 2016

Or even simpler:

<img src=x onerror=prompt(1)//

@enov
Copy link
Contributor

enov commented May 19, 2016

Thank you for your thorough research @dsopas. We highly appreciate your time and efforts to detect the security shortcomings of Kohana.

I will try to come up with a better regex. Not a regex expert myself, but there are lots of online tools nowadays. Let me know if you are willing to issue a pull request yourself.

@dsopas
Copy link
Author

dsopas commented May 19, 2016

In my opinion you should never rely on regex to parse HTML. There's always a way to bypass it.
I can suggest you another thing. Grab the src/alt/class/id and construct the img tag. I think that way you can prevent bad stuff from happening. Still don't forget to validate those fields.

Let me know when you provide a fix so I can complete the Checkmarx security advisory.
Keep up the good work!

@enov
Copy link
Contributor

enov commented May 20, 2016

Thanks again @dsopas.

Here's another regex:

#<img/?\s*[^>/x{0C}]*(?=src\s*=)(?:src\s*=\s*["\']?([^"\'<>\s]*)["\']?[^>/x{0C}]*)?(?:>|//|\x{0C})#isu

It's becoming more and more complicated and I understand that if we use regex there might always be a way to bypass it.

We can deprecate and remove this method whole-together from the Security class, as there are other HTML tags that can can execute JavaScript and we do not have helper method for those. We can guide the users to sanitize their input against XSS using external libraries.

By the way, is there a reference for all the quirky HTML tag structure that the browsers accept? Or is this original research?

@enov
Copy link
Contributor

enov commented May 20, 2016

Here's an online shell test for it:

https://3v4l.org/lr9uS

@dsopas
Copy link
Author

dsopas commented May 20, 2016

I agree with your second option. Like I said regex is not the way to do it. If you still need to use, do what I wrote before. Grab src content only, sanitize it and construct the img yourself.
It's not a original research is something that you know with experience. I don't know any type of manual about this. Also it depends on the browser side. Sometimes browsers still render incomplete tags, others don't.

@dsopas
Copy link
Author

dsopas commented May 20, 2016

@enov really try to do what I wrote before.
I just bypassed it again:
<img srcset=x onerror=prompt(1)//

Best,
David Sopas
Checkmarx.com

@dsopas
Copy link
Author

dsopas commented Jun 22, 2016

Hi @enov any updates on this issue?
Thanks.

@enov
Copy link
Contributor

enov commented Jun 22, 2016

Hey @dsopas. Kindly leave the issue open. I might have some time next week to address it.

@dsopas
Copy link
Author

dsopas commented Jun 22, 2016

Ok, no problem. Just keep me in the loop please.

enov added a commit to kohana/core that referenced this issue Jun 27, 2016
Deprecated for security reasons.
See kohana/kohana#107

Users must be encouraged to use more secure and better maintained
external libraries.
@enov
Copy link
Contributor

enov commented Jun 27, 2016

todo:

@enov
Copy link
Contributor

enov commented Aug 18, 2016

@dsopas this has been done. Thank you for reporting this and sorry it took us so much time.

@enov enov closed this as completed Aug 18, 2016
@dsopas
Copy link
Author

dsopas commented Aug 18, 2016

No problem Samuel.
Nice work!

Best,

David Sopas
https://twitter.com/dsopas
https://www.linkedin.com/in/dsopas

On 18 August 2016 at 09:49, Samuel Demirdjian notifications@github.com
wrote:

@dsopas https://github.com/dsopas this has been done. Thank you for
reporting this and sorry it took us so much time.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#107 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALAIwpBnZ9nRX0CAr_mJ_lqRebwkox2tks5qhByRgaJpZM4IhhOf
.

@carnil
Copy link

carnil commented Jan 13, 2018

This issue was assigned CVE-2016-10510.

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

3 participants