html/template: does not recognize rgb() as a CSS color

daviddengcn commented 6 years ago

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (`go version`)?

tip

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (`go env`)?

does not matter

What did you do?

An code fragment to reproduced is here: https://play.golang.org/p/r8LOFN_roBo Using string instead of template.HTML produces the same result.

What did you expect to see?

color: rgb(10,20,30)

What did you see instead?

color: ZgotmplZ

daviddengcn commented 6 years ago

I had thought any template.HTML-typed value will be output directly. template.CSS works. But why string does not.

jimmyfrasche commented 6 years ago

This may be a bug as I'd expect css function syntax to be allowed.

However, if you use template.CSS instead of template.HTML as the type of ColorRGB it works correctly.

bradfitz commented 6 years ago

I don't know this area, but if this is ultimately deemed not a bug, somebody should consider what documentation could be added to clarify this for future users.

zegl commented 6 years ago

It happens because rgb() looks like unsafe JavaScript, which template.HTML tries to prevent. See the documentation on the ZgotmlpZ error. The documentation for template.CSS notes that that type is there to mark the content as safe, and that the source should be trusted.

daviddengcn commented 6 years ago

rgb() is actually a safe css function. More functions can be found: https://www.w3schools.com/cssref/css_functions.asp

(a better way may be find css standards)

I don't think encouraging people to use template.CSS is a good idea. It can only be a workaround. Using string will be safer in case some careless bug may introduce unsafe code.

jamdagni86 commented 6 years ago

@ianlancetaylor does this need to be fixed? I'll have a look at it if it needs one.

ianlancetaylor commented 6 years ago

@jamdagni86 Frankly, I don't know.

CC @mikesamuel @stjj89

mikesamuel commented 6 years ago

cssValueFilter is the function that is rejecting these. It's conservative and does not make any attempt to recognize safe functions.

@daviddengcn said

rgb() is actually a safe css function.

I agree that rgb, rgba, hsl, and hsla are safe functions. They're just not recognized as such.

The tests (code) give a sense of what is currently recognized.

jamdagni86 commented 6 years ago

@mikesamuel - one approach to fix this is to figure out if the value is a safe CSS function call. If it is, then we'll have to make sure the function call is of a valid syntax. Do we have a parser somewhere which I can use?

If you had other thoughts, please let me know.

empijei commented 6 years ago

@jamdagni86 the current implementation just ranges over the decoded bytes and detects potentially dangerous characters, so there is no parser to use.

Switching to a parser to allow certain functions would imply a complete rewrite of that logic, and would very likely introduce issues and potential vulnerabilities in code that is currently protected by that check (e.g. existing code with a globally defined and vulnerable rgb javascript function or something similar).

I don't think switching to an allowed-list can be easily done in a secure way, and would probably not be considered backward compatible.

empijei commented 5 years ago

Apparently we will get more functions:

acos()
asin()
atan()
atan2()
calc()
clamp()
cos()
hypot()
max()
min()
pow()
sin()
sqrt()
tan()

And this is not considering the already existing ones:

attr() ← Dangerous
calc()
cubic-bezier()
hsl()
hsla()
linear-gradient()
radial-gradient()
repeating-linear-gradient()
repeating-radial-gradient()
rgb()
rgba()
var() ← Dangerous

These are some options we could go for:

invest some time in finding a scalable way to support CSS functions;
document that in order to use those functions the CSS type should be used and that user-controlled data should not be passed to it, but this might become a footgun;
ATM the parameters accepted by these functions are colors, numbers and percentages, we could offer helper functions to escape/sanitize them before they get promoted to the CSS type without supporting it in the cssValueFilter.

Are there any opinions on how to approach this?

stjj89 commented 5 years ago

document that in order to use those functions the CSS type should be used and that user-controlled data should not be passed to it, but this might become a footgun;

Documentation for this already exists, actually. In the CSS type docs, the rgba function is one example of an acceptable CSS value.

CSS typed strings are already foot-cannons in the html/template design, so I don't think pointing users this way makes it any worse.

invest some time in finding a scalable way to support CSS functions;

I'm not sure how difficult CSS parsing is, but I wonder how added complexity (and potential for bugs) this will add the html/template.

empijei commented 5 years ago

Do you think it is reasonable to just add something to the doc for the CSS type?

In my opinion since most of the inputs are numbers this is very unlikely to cause vulnerabilities even if users have to disable escaping.

mikesamuel commented 5 years ago

@empijei I think it's safe to assume that numeric inputs to functions are safe. I don't see any greater risk for info leakage due to numbers in functions than numbers directly in positioning rules.

For functions that take strings, we'd need to know whether the string will be assumed to be a URL.

An easy first cut would be to just

Allow functions on a whitelist including those you listed above.
Allow function arguments that are not quoted strings

golang / go