Handlebars incorrectly listed as a vulnerability

[infolock]

I'll keep this short and sweet to begin with. If you want more information, please let me know and I'll provide more (I just don't want to create a book to read unnecessarily).

Pull Request #5 contains a line that lists Handlebars as containing an XSS Vulnerability.

https://github.com/nodejs/security-advisories/pull/5/files#diff-9bcb553b1623927590a9446a8cb25cab

If one reads this vulnerability carefully, one can easily see that this is a vulnerability implemented by developers applying values to attributes without wrapping those values with quotes.

While the quotes around HTML element attribute values are optional, omitting them actually introduces a possible XSS Vulnerability.

However, this can be done with or without Handlebars. If Handlebars is marked as introducing the vulnerability - then any library that allows a developer to pass in HTML to its methods are also culprits of introducing this vulnerability... which isn't exactly valid because the vulnerability is introduced by the HTML passed into the library - not the library itself.

A library would have to basically implement the W3 spec on HTML elements and then apply a magical validation formula that would ultimately end with something resembling the infamous parsing XML with regex stackoverlow problem :P.

I am requesting that this be reconsidered and the consideration of removing this from the node security advisories as well.

Additional information can be found here: handlebars-lang/handlebars.js#1514 (comment)

[sam-github]

https://github.com/nodejs/security-advisories/blob/90a639ca734756175fe2a9f1ef1fee12226ec17b/ecosystem/handlebars/61.json

^--- Is this the vulnerability you are referring to?

This vuln came from the initial nsp data dump, see https://github.com/nodejs/security-wg/commits/master/vuln/npm/61.json, I'm just trying to understand the context.

Pull Request #5 contains a line that lists Handlebars as containing an XSS Vulnerability.

If we are talking about vuln #61, then that's not exactly what it says. 61.json claims that handlebars < v4.0.0 had a vuln, but that handlebars-lang/handlebars.js#1083 fixed it, and anyone using handlebars >= 4.0.0 is NOT vuln.

Again, I didn't look in detaily (yet), but https://www.veracode.com/blog/research/cut-and-paste-component-vulnerabilities-short-study-how-handlebarsjs-vulnerability-has seems to show this was a simple bug, which was then fixed in handlebars-lang/handlebars.js@83b8e84, and according to handlebars-lang/handlebars.js@83b8e84#commitcomment-31690763 has been fixed in 3.0.4 and 3.0.5 --- so 61.json's description of which versions are vulnerable can be changed now.

But you are describing this as NOT a bug that can be easily fixable at all, so I'm confused.

Are you talking about a different vuln, not 61? Am I misunderstanding the report or the links above?

[infolock]

This is the same bug. The handlebars fix was reverted because the fix introduced a breaking change and has been reverted.

They are now reconsidering a new patch. However, the primary point is they (the handlebars team) should never had to make a patch in the first place as this is not a vulnerability with their code - it is a vulnerability introduced by developer bad practices.

So the issues are:

• a vulnerability (Snyk) was created that (incorrectly) called handlebars out as having a vulnerability
• this was then added to the node security advisories (which was correct because it was a listed advisory)
• Handlebars created a patch - but that has since been reverted because it broke backwards compatibility (V1.3.x (Retrofit Security Patch...) handlebars-lang/handlebars.js#1514 (comment))

I’m hoping to have the issue removed from snyk’s site (which I’ve emailed their team and am waiting for their reply). The hope is to also have this removed from your list here as well (thus this issue ticket).

Otherwise the handlebars team will continue to feel compelled to release a patch for a vulnerability that they never introduced.

[infolock]

Thanks for pointing us to that 1083 issue (this further re-enforces the point that this isn't a handlebars vulnerability in the first place). Please note that even in that thread, they are saying the same thing I've listed above: handlebars-lang/handlebars.js#1083 (comment)

...With that said, technically this is more of a fault from developers not inserting the correct “ or ‘ to contain the value of the html Attribute, but it is such a common usage that I think you should try to fix it, by escaping a few more characters as you mentioned and I have recommended above.

This is exactly what I'm saying as well. But instead of just saying that "hey, this is technically due to developers doing something stupid - but lets go ahead and fix it anyways" - the recommendation is to remove the security vulnerability entirely.

Because the vulnerability isn't due to handlebars - it is due to developers not properly applying quotes to the HTML attribute values =)

[sam-github]

We got this vuln in the nsp data dump, are you saying it originally was found by snyk?

@lirantal any comment?

[infolock]

Oh, no I'm not suggesting snyk found the issue - sorry. I just assume that nsp pulled it in from somewhere. if nsp found it then that would make sense of how it got to where it is.

Either way, the issue isn't who reported it first. the issue is that Handlebars is not responsible (at all) for this vulnerability. it is a (known?) XSS Vulnerability regarding HTML attributes not having quotes around their values.

Example:

<img src=http://www.evil.org />

this is susceptible to an XSS. (one source to read up on is on the OWASP repo )

Note that the issue is how the developer forms their HTML tags - which is what introduces that vulnerability. Not because they are using Handlebars.

[infolock]

The point is - the developer can introduce this same issue without handlebars.

var myURL = 'http://evil.org';

document.write("<img src=" + myURL + " />");

note that we are doing the exact same thing the vulnerability describes, but without handlebars at all.

doing the above would be considered introducing an xss vulnerability to the site.

would/should we create a vulnerability report against Javascript for this? no. because the developer is being dumb enough to do this - which is not something javascript can or should dictate.

The same is true for handlebars when the developer is using Handlebars Expressions:

<img src={{myURL}} />

[grnd]

Hello @infolock, I'm Danny from @snyk. Also a member of the node foundation security WG.
(I read the report you submitted to us, I couldn't respond since there was no email address provided).

From what I gather, handlebars attempts to escape the dangerous chars to prevent XSS. It did so for these chars [&<>"', but missed the =` char.
This commit introduced the fix with the following commit msg:

Escape = in HTML content
There was a potential XSS exploit when using unquoted attributes that this should help reduce.

I agree that developers can introduce XSS issues in various ways, even without handlebars, but that doesn't mean that there's no issue with the way handlebars escapes the data. Handlebars folks did attempt to protect against the issue, so developers can expect this to be handled for all types of bad characters, not just some of them.

[infolock]

hi @grnd - I think I'm almost understanding what you're saying, so I want to be 100% sure I do before I close this out.

take the following example:

<div>
    <img src={{myURL}} />
</div>

what you're saying is handlebars originally introduced a bad-character escaper to handle issues like the above, where the data in myURL could contain potentially harmful characters - and attempted to strip them out or escape them. one of which (the =) was missed. because of this, a vulnerability report was issued against them.

Is that about the gist ?

if that is correct, let me follow-up by asking this: if the example above was written as follows, would the same vulnerability issued against them still be valid?

<div>
    <img src="{{myURL}}" />
</div>

if the answer is "yes, the vulnerability is still present even when the value is surrounded by quotes", then I have completely misunderstood what the reported vulnerability even was and we can close this issue.

If the answer is "no, the vulnerability is no longer present" - then my original request is still valid.

i'm not trying to troll you guys either. i'm just trying to understand if this is truly a vulnerability or a bug.

in this case, you see, a vulnerability would be present if:

• they introduced the vulnerability when they added the logic to escape the dangerous characters
• it was an attempt to fix some other piece of logic that was allowing an XSS to be possible that existed in their codebase (i.e., if handlebars was actually creating the DOM elements themselves instead of using the HTML developers are passing in to the library)

However, if the vulnerability was present before they ever attempted to escape a single bad character and the vulnerability was present only because of missing quotes around HTML element attribute values, then this is merely a bug on the handlebars team - not a full-fledged xss vulnerability. if anything, they were trying to help developers solve a problem that they really had no business attempting to solve in the first place as solving that problem is the responsibility of the developers who are using this library (to choose to solve by applying quotes or not).

Case in point, the characters that handlebars are escaping may be excluding the =, but they are also missing the unicode space characters (along with a few others). So, are we saying that unless they escape every known character sequence that has been singled out against HTML element attributes, that they will continue to be considered as introducing this vulnerability? if so, that may have been missed that the vulnerability report and they will need to be notified about that.

If this is indeed a bug, please realize that's why i'm making a semi-nerd-fuss about this particular one =P because when a xss vulnerability alarm is raised, we're all hands on deck.

thanks a lot for taking the time to look into this!

[grnd]

Thank you @infolock. Yes, your understanding of the issue is correct. Indeed the vulnerability exists only if the attribute of an HTML tag is left unquoted like you showed.

I think what you're saying that this is not an "XSS vulnerability", but rather an "insufficient escaping" issue. And I agree. When presenting this issue we mean "There's an improper escaping happening in handlebars that can possibly lead to XSS vulnerability". Perhaps the title of the issue ("XSS") is misleading in this case? Would make more sense to call it "Improper escaping", more inline with CWE-79

For what it worth, the description correctly mentions the exact use case:

When using attributes without quotes in a handlebars template, an attacker can manipulate the input to introduce additional attributes, potentially executing code. This may lead to a Cross-site Scripting (XSS) vulnerability, assuming an attacker can influence the value entered into the template. If the handlebars template is used to render user-generated content, this vulnerability may escalate to a persistent XSS vulnerability.

[infolock]

@grnd thank you for the response. Yep, that would make a lot more sense (although I would also be very much in favor of removing it entirely...).

Last question (I swear 😄 )

can we instead have this filed as a PEBCAK at the very least? Stupid Humans misusing libraries to introduce a vulnerability doesn't exactly qualify as a medium security level XSS Vulnerability.

[infolock]

I guess we should we just keep this open until we can update that for tracking purposes yeah?

[infolock]

Before you guys/gals decide (one way or another), Please Take this Into Consideration as for why I'm trying to say that this is a bug...

If we want to say this is a vulnerability, then any library that offers the same type of functionality would also have this same vulnerability (wether they attempt to sanitize any part of it or not).

Case in Point would be underscore.js's Template..

Steps To Reproduce for Underscore:

1. Go to https://underscorejs.org
2. Open up the console
3. Enter the following:

var value = document.getElementById('template').innerHTML;
var template = _.template(`<img src=<%- value %> />`);

template();

Observe that you will see the following outputted as a result:

"<img src=
        &lt;b class=&quot;header&quot;&gt;template&lt;/b&gt;&lt;code&gt;_.template(templateString, [settings])&lt;/code&gt;
        &lt;br&gt;
        Compiles JavaScript templates into functions that can be evaluated
        for rendering. Useful for rendering complicated bits of HTML from JSON
        data sources. Template functions can both interpolate values, using
        &lt;tt&gt;&amp;lt;%= … %&amp;gt;&lt;/tt&gt;, as well as execute arbitrary JavaScript code, with
        &lt;tt&gt;&amp;lt;% … %&amp;gt;&lt;/tt&gt;. If you wish to interpolate a value, and have
        it be HTML-escaped, use &lt;tt&gt;&amp;lt;%- … %&amp;gt;&lt;/tt&gt;. When you evaluate a
        template function, pass in a &lt;b&gt;data&lt;/b&gt; object that has properties
        corresponding to the template&#x27;s free variables. The &lt;b&gt;settings&lt;/b&gt; argument
        should be a hash containing any &lt;tt&gt;_.templateSettings&lt;/tt&gt; that should be overridden.
       />"

Note that the attributes are not quoted, which introduce the same vulnerability that is being reported against Handlebars.

We can easily say that "well, handlebars tried to sanitize this, but they missed x chars". And that's fair - but that does not introduce any new vulnerability. If anything, it doesn't fix the vulnerability - which would make it a bug on their escape characters rules.

The vulnerability is only a vulnerability when the developer provides HTML that allows the vulnerability to be possible. which is (as you can see) possible in just about any templating library. Not because those libraries create this vulnerability - but because the developer using the library introduces it - which isn't something the library should be held accountable for.

[sam-github]

My impression ATM is different. Handlebars has functionality to protect the user by escaping some unsafe chars. Users may, reasonably, assume they can rely on handlebars doing that right, but in fact, one of the unsafe chars was missing. Oops.

This is analogous to the difference between a boat captain not supplying life vests, and supply life vests that don't work. In the first case, its the passengers fault if they don't bring a life vest, in the second, its the captains fault for providing a faulty one.

[infolock]

we can argue against each others analogies until we're in an all out shouting match i guess. But that doesn't actually get us anywhere - nor is it what I think either one of us truly wants at the end of the day (especially on such a public floor).

that said, there has been enough evidence and information provided above to justify the ask here and I would hope it has been enough to persuade whoever ultimately makes the decision on what is being asked.

the only other thing I can suggest we consider is this:

that info found in nsp data, this security advisory repo, snyk - all are one of many sources where developers can get their information that they obtain because they feel they can trust it. I can only assume that was the intension of creating these services in the first place.

To make these services effective and an asset to developers, there is a level of trust that has be earned, which comes from the integrity of these reports.

When the reports aren't truly representing the severity of concern that is being delivered (such as this report), it severely reduces the trust aspect that is necessary for it to be effective and reliable. Otherwise, it just becomes more noise and a liability for developers - when they already have more than enough of this in their workdays.

I personally trust snyk a lot because I need to trust someone if I don't want to keep wasting 5 days looking into a security vulnerability - just to find out it wasn't a vulnerability at all.. that's why i went out of my way to open this. i don't think it was the intent of anyone to break down its credibility. quite the opposite. and truly, the hope is that everyone can see that.

wether or not that is agreed upon is a debate that i think could go on for days - but I am exiting the debate of it.

I am only after one thing - helping to improve the integrity of these reports by acknowledging the good ones, and shedding light into the ones that might harm it.

[sam-github]

I'm sorry you didn't find the analogy useful, so let me attempt to be more specific.

If an API attempts to sanitize input, but it does not cover some cases, its a vulnerability, IMO and presumably snyk's, and probably most sec researchers. This appears to be handlebar's case.

If the API doesn't do any sanitization, and a user uses it assuming it does, the user has made a mistake, its on them, its not a vulnerability. Your underscore example appears to be of this variety.

If you disagree that handlebars implements faulty sanitization, then its worth making that case, I (at least) would find that compelling, but from what I understand so far, its sanitizer was missing a required character (=).

If you disagree that faulty sanitization is a vulnerability, because its not materially different than not doing sanitization at all, then we might be at an impasse.

[infolock]

ok - i remove that last statement, i'm very wrong as can clearly be seen here: https://github.com/wycats/handlebars.js/tree/1.0.0#escaping

Escaping
By default, the {{expression}} syntax will escape its contents. This helps to protect you against accidental XSS problems caused by malicious data passed from the server as JSON.

To explicitly not escape the contents, use the triple-mustache ({{{}}}). You have seen this used in the above example.

well - 💩 . they shouldn't have claimed that - and now i'm doing a 180 and saying I understand how this is now a valid issue hah..

i had myself convinced at least - so there is that. :)

to be fair - they did end up removing that statement and it does say they only helps - not prevents. but i think the more prudent path would have been to remove that logic as well. ahhh well lol. thanks for the chat. i learned more about handlebars than I ever would have otherwise =P