fix(security): no warning when sanitizing escaped html (#9392)

[wkwiatek]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
#9392

What is the new behavior?
No warning when properly escaped html is passed to sanitize

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

[mprobst]

Thanks for the PR!

This looks conceptually fine (we want to compare to the input, not to the intermediate parsed state), and the test looks good, but I don't understand why it works.

Shouldn't safeHtml, once we parsed and re-serialized it, also no longer contain the escaped entity, but rather the unicode code point directly? And if so, how or why is it different from the treatment of unsafeHtml (before your change), which was also just parsed and re-serialized?

Any idea what's going on there?

[wkwiatek]

The problem is produced by these lines:
https://github.com/angular/angular/pull/9413/files#diff-475057312e0ba8afc2f24a4df80d6eaaL249 https://github.com/angular/angular/pull/9413/files#diff-475057312e0ba8afc2f24a4df80d6eaaL243

First modifies the input so that it's no longer the initial value in some cases (as you can see unsafeHtml becomes parsedHtml). DOM.setInnerHTML(containerEl, unsafeHtml); leaves the treatment to the browser so in the end the value may be a little bit different than the one we passed in to the function.

[mprobst]

Assume this code, from a DevTools console session:

let d = document.createElement('div');
// <div>​</div>​
let unsafeHtml = 'hello &#x1f680;'; // the input to sanitize
d.innerHTML = unsafeHtml; // parse it
let safeHtml = d.innerHTML; // serialize it back
// "hello 🚀" -- safeHtml now contains the actual rocket character, unescaped.
safeHtml === unsafeHtml;
// false

Turns out our code explicitly encodes entities in safeHtml, in particular surrogate pairs. That means with this change, it'll work if entities in unsafeHtml are encoded originally, but then will fail if they were not because it encodes all entities (that's what I forgot, which was confusing me here).

E.g. if you add a test case for sanitizeHtml('hellö') (with the actual Unicode character in there), this will break as the result will be 'hellö', won't it? Fundamentally, this code is not aware of what is encoded and what is not in the input at the point of the comparison, so it seems to me that there is no real winning here, is there?

[wkwiatek]

The code you've just pasted is actually fine and reflects the situation except of naming. Look what safeHtml is in the source: https://github.com/angular/angular/pull/9413/files#diff-475057312e0ba8afc2f24a4df80d6eaaL253.

safeHtml at the end in condition is simply not related in any way with this whole stuff we're talking about. I refer to do {} while () block which I guess is only for mXSS protection. And was accidentally overwriting input of the function.

[mprobst]

Could you add this test case and see what happens?

    t.it('supports sanitizing escaped entities', () => {
      t.expect(sanitizeHtml('hellö')).toEqual('hellö');
      t.expect(logMsgs).toEqual([]);
    });

[wkwiatek]

No problem.

Here's the output:

Expected 'hellö' to equal 'hellö'.

Expected [ 'WARNING: sanitizing HTML stripped some content.' ] to equal [  ].

Both I've expected before starting tests.

[mprobst]

So, do you think this change is worth it, given that we fundamentally cannot fix the problem?

[wkwiatek]

I think that fundamentally it works pretty well. Look, sanitizeHtml('hellö') gives you the output: hellöwhich is fine (because really sanitizes the output) and also gives you warning that something was stripped. Then try to add a test like this:

t.it('supports sanitizing escaped entities', () => {
  t.expect(sanitizeHtml('hellö')).toEqual('hellö');
  t.expect(logMsgs).toEqual([]);
});

Now input and output are exactly the same. Both tests in this PR will succeed. However in the master second will fail because of warning message that should not be logged in this case.

I think it's still worth to add because in current version the information is just misleading.

Make sense?

[mprobst]

nit: could you rename this to unsafeHtmlInput? It's kind of important here to keep track of what's safe and what isn't.

[wkwiatek]

Done

[mprobst]

We'll still warn people about effectively harmless changes (input changes that are not actually changing anything), but I can see your reasoning. Also, the change is harmless enough.

Could you fix the parameter name? Otherwise good to go.

[mprobst]

Merged. Thanks for the contribution @wkwiatek!

[wkwiatek]

Thanks. You're welcome!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}