mganss / HtmlSanitizer

Cleans HTML to avoid XSS attacks
MIT License
1.55k stars 200 forks source link

Is it safe to allow data URI's for img's? And best way to do it? #187

Closed EvanSevy closed 4 years ago

EvanSevy commented 5 years ago

I've seen people discussing something similar in other issue posts, but not exactly.

My question is whether it is safe, particularly in regard to XSS, to allow data URI's within img tags?

I believe I'm going to allow img's from http and https. Are img's from a Data URI any less safe than img's from http/https?

How would I go about enabling this behavior of data URI's in img tags?

tiesont commented 5 years ago

Your last question is actually covered in the wiki, to wit:

var sanitizer = new HtmlSanitizer();
sanitizer.AllowedSchemes.Add("data");
mganss commented 5 years ago

AFAICT this might be a problem in older browsers: https://security.stackexchange.com/questions/165713/using-data-uris-to-perform-xss-in-anchor-tags-vulnerability

EvanSevy commented 5 years ago

@tiesont , my question was for whether a data uri was safe for specifically img tags?

This post seems to say it is not a problem: https://security.stackexchange.com/a/83940/86412

Here is the code I used to handle this with HtmlSanitizer:

static void FilterAttributes(object sender, RemovingAttributeEventArgs e)
{
    if( (e.Tag.TagName == "IMG" && e.Attribute.Name.Equals("src") 
            && (e.Attribute.Value.StartsWith("data:image/") || e.Attribute.Value.StartsWith("http") || e.Attribute.Value.StartsWith("https")))
        || (e.Tag.TagName.Equals("IFRAME") && e.Attribute.Name.Equals("src")))
    {
        e.Reason = RemoveReason.NotAllowedAttribute;
        e.Cancel = true;
    }
}

and where I called it:

sanitizer.AllowedAttributes.Remove("src");
sanitizer.RemovingAttribute += FilterAttributes;
mganss commented 5 years ago

The link I referenced above is about <a>s, not <img>s, so not applicable here. I can't find anything about XSS potential for data: URIs in images. There's obviously a problem if you allow the javascript: protocol in <img> but even that is impossible in modern browsers.

There's nothing in the OWASP XSS Filter Evasion Cheat Sheet nor in the HTML5 Security Cheatsheet.

The code looks ok; two minor things, though:

EvanSevy commented 5 years ago

@mganss you just mentioned that e.Reason = RemoveReason.NotAllowedAttribute; doesn't do anything. Though in this link https://github.com/mganss/HtmlSanitizer/issues/165 you state that "(the RemoveReason will be NotAllowedUrlValue)", so I blindly implemented the offending RemoveReason.NotAllowedAttribute you mentioned above thinking that it was simple enough and was required for some reason. Is there certain cases where setting 'RemoveReason.NotAllowedXXX' is valid? Or is this perhaps deprecated?

mganss commented 5 years ago

@EvanSevy The RemoveReason serves as an input parameter to the event handler. It allows the event handler to determine the reason why an attribute is about to be removed.

In contrast, the Cancel property allows the event handler to signal back to the code that invoked it.

Dev-questions commented 4 years ago

@mganss -- var sanitizer = new HtmlSanitizer(); sanitizer.AllowedSchemes.Add("data");

Above is allowing all data URIs, where i need to allow only data:image URI. I don't want to allow other once like dat:text/html etc. Is it possible to allow only data:image?

mganss commented 4 years ago

@Dev-questions See https://github.com/mganss/HtmlSanitizer/issues/230#issuecomment-648194241