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

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

Closed
EvanSevy opened this issue Sep 24, 2019 · 8 comments
Closed

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

EvanSevy opened this issue Sep 24, 2019 · 8 comments
Labels

Comments

@EvanSevy
Copy link

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
Copy link

tiesont commented Sep 24, 2019

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

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

@mganss
Copy link
Owner

mganss commented Sep 24, 2019

@EvanSevy
Copy link
Author

EvanSevy commented Sep 28, 2019

@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
Copy link
Owner

mganss commented Sep 29, 2019

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:

  • e.Attribute.Value.StartsWith("https") is redundant. If a string starts with "https" it will also start with "http".
  • e.Reason = RemoveReason.NotAllowedAttribute; doesn't do anything.

@EvanSevy
Copy link
Author

@mganss you just mentioned that e.Reason = RemoveReason.NotAllowedAttribute; doesn't do anything. Though in this link #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
Copy link
Owner

mganss commented Sep 30, 2019

@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.

@mganss mganss closed this as completed Apr 28, 2020
@Dev-questions
Copy link

@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
Copy link
Owner

mganss commented Jun 23, 2020

@Dev-questions See #230 (comment)

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

No branches or pull requests

4 participants