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

Sanitize urls in css style sheets #5189

Merged
merged 1 commit into from Jun 22, 2021

Conversation

st3iny
Copy link
Contributor

@st3iny st3iny commented Jun 16, 2021

Currently, CSS style sheets are not sanitized and allow tracker scripts to bypass our trusted senders feature. Furthermore, our style attribute sanitizer was not strict enough so I improved it to replace all url(...) values (not just those of background-* rules).

I added a new PHP dependency for parsing CSS style sheets. Using regex is not sufficient to parse style sheets and could be exploited by a future attacker.

Example

ul { list-style-image: url(https://my.tracker.com/some-image.png); }
div { background-image: url(https://my.tracker.com/some-image.png); }

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny force-pushed the fix/noid/sanitize-css-style-sheets branch from a616e20 to 4bdcc02 Compare June 16, 2021 11:37
iframeDoc
.querySelectorAll('style[data-original-content]')
.forEach((node) => {
node.innerHTML = node.getAttribute('data-original-content')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested this, but could I use this to bypass any other kind of sanitization we may be running on the HTML?

As in: what if someone sends an email that has something like:

<span data-original-content="<img src=&quot;doesnotexist&quot; onerror=&quot;alert(1)&quot;">

Or don't we have any kind of sanitization anymore regardless as we sandbox the iframe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selector only selects style elements with the attribute data-original-content so your example won't have any effect.

Nevertheless, an attacker could send a forged style tag containing some malicious content but our sanitation extracts, merges and then sanitizes all style sheets at once so theoretically this shouldn't be exploitable. The rendered html email will only have one style element right at the beginning.

@st3iny st3iny added this to the v1.10.0 milestone Jun 21, 2021
@st3iny
Copy link
Contributor Author

st3iny commented Jun 22, 2021

/backport to stable1.9

@ChristophWurst ChristophWurst merged commit e2b1f14 into master Jun 22, 2021
29 of 30 checks passed
@ChristophWurst ChristophWurst deleted the fix/noid/sanitize-css-style-sheets branch June 22, 2021 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants