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

HTML Injection issue via msgId #2770

Closed
seisvelas opened this issue May 21, 2020 · 2 comments
Closed

HTML Injection issue via msgId #2770

seisvelas opened this issue May 21, 2020 · 2 comments

Comments

@seisvelas
Copy link
Contributor

This is issue "FLO-02-004 Extension: HTML Injection in error message on certain pages" from the Cure53 report. From the report itself:

A HTML injection was found on pgp_block.htm via the error message. This is becausethe returned message from the API call is rendered as HTML. However, this issue doesnot introduce XSS because the rendered content is sanitized and only allowed safe tags.Besides, CSP of WebExtension will block this attack nevertheless. At the same time, thiscould still be used for Phishing.PoC (Replace the logged-in email address in the highlighted area):chrome-extension://emghkmengffbgcnpcajidfdkffaikghk/chrome/elements/pgp_block.htm?frameId=&hasPassword=cu_false&msgId=%3Cs%3E&senderEmail=&isOutgoing=cu_true&acctEmail=EMAIL_ADDRESS&parentTabId=

Screenshot_2020-05-21 FLO-02-report pdf

Currently we are sanitizing this, which prevents it from being a much more serious issue. Nevertheless, this should be fixed by escaping the output so user input is not rendered as HTML in the error message.

@tomholub
Copy link
Collaborator

The issue is not limited just to msgid - the larger issue is that the stringified exception text should be Xss sanitised when passing it to the rendering functuion

@seisvelas
Copy link
Contributor Author

Okay, so one might expect the relevant sanitization to be here:

Xss.sanitizeRender('body', `
<br>
<div data-test="container-err-title" style="width: 900px;display:inline-block;">${ApiErr.eli5(e)}</div>
<br><br>
<div data-test="container-err-text" style="width: 900px;display:inline-block;">${String(e)}</div>
<br><br>
${Ui.retryLink()}
`);

That code does render other errors. For example, in the case of the API traversal error, we see that the rendered divs are the ones created by the above:

<div style="width: 900px;display:inline-block;" data-test="container-err-title">FlowCrypt encountered an error with unknown cause.</div>
<br><br>
<br><div style="width: 900px;display:inline-block;" data-test="container-err-text">Error: API path traversal forbidden</div>

However, the error that is leading to renderable HTML does not originate there. We can tell by looking at the div tag that is generated:

<div class="error">Error: Bad Request: 400 when GET-ing https://www.googleapis.com/gmail/v1/users/me/messages/<s> object: format -&gt; Invalid id value</s></div>

This div has very different properties. It originates here:

await this.view.renderModule.renderContent(`<div class="error">${errBoxContent.replace(/\n/g, '<br>')}</div>${showRawMsgPrompt}`, true);

If so, I should be able to prevent tags from rendering by simply escaping errBoxContent. Simply adding another call to .replace() ought to suffice. I'll submit a PR momentarily.

tomholub pushed a commit that referenced this issue May 22, 2020
* sanitize error messages by removing all tags

* trying to make test work by changing type

* more attempts to fix tests

* put escape in proper location and use Xss.escape

* improving tests per Wiktors advice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants