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

Replace readBlobAsDataUri with URL.createObjectURL #1820

Closed
wants to merge 2 commits into from

Conversation

walle303
Copy link

@walle303 walle303 commented Apr 2, 2018

This fixes element-hq/element-web#2678 and element-hq/element-web#2866 and provides an overall performance increase when handling embedded media in encrypted rooms.

@ara4n
Copy link
Member

ara4n commented Apr 3, 2018

We should review & consider this alongside element-hq/element-web#6173

@lukebarnard1
Copy link
Contributor

@ara4n, for those of us not "in the know", why does element-hq/element-web#6173 need considering here?

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

If we're creating object URLs, we'll need to revoke them at some point, otherwise they'll just accumulate and leak memory.

@dbkr
Copy link
Member

dbkr commented Apr 23, 2018

This ought to fix element-hq/element-web#2770

@dbkr
Copy link
Member

dbkr commented Apr 23, 2018

Another couple of points on this:

  1. You've removed the braces from the import of decryptFile which changes the meaning of the import which means this fix doesn't work as it stands.

  2. More importantly, whereas data URLs are from their own unique origin, blob URLs inherit the origin of the page they were created in which means if the user manages to open the URL in a browser tab their entire account is compromised, private keys and all. I engineered this by sending an HTML file as an m.audio then, as the user, right clicking, picking, "Copy Audio Address" and pasting that URL into a new tab, which is reasonably contrived but given the consequences, I don't think this is viable I'm afraid.

@ara4n
Copy link
Member

ara4n commented Apr 28, 2018

So, i was just looking at this in the context of element-hq/element-web#2866, and the thing that worries massively me is that the reason readBlobAsDataUri was added in the first place was: 8f778f5#diff-d3d5b76e1f38f26633799c4c6b61105a

whose commit message of "Use data:// URI rather than blob: URI to avoid XSS" sounds worthwhile not ignoring.

I assume this is because the blob URI could be sculpted to contain a javascript: URI or something? Anyone know the specific XSS?

Edit: I'm an idiot and had somehow totally missed @dbkr's second point in the previous comment, which spells out the vuln this introduces.

https://www.attacker-domain.com/2014/07/javascript-blobs.html also provides a nice quick overview of the problem.

@ara4n
Copy link
Member

ara4n commented Apr 28, 2018

Having looked into this a bit more, I can reproduce this attack but only if the mime-type passed in when instantiating the Blob is a type which can access JS when rendered in a browser (e.g. text/html or image/svg or application/pdf or whatever).

Might it be good enough just to whitelist the mime-types which we trust to be rendered from blobs (e.g. image/jpeg, image/png, image/gif, video/mp4) and for the rest of them set the mime-type to application/octet-stream so they just get downloaded as a plain old file?

@ara4n
Copy link
Member

ara4n commented Apr 29, 2018

@walle303 heads up that i'm experimenting with the mime-type whitelisting idea atm (lest we collide)

ara4n added a commit that referenced this pull request Apr 29, 2018
Based on @walle303's work at #1820
Deliberately reverts 8f778f5
Mitigates XSS by whitelisting the mime-types of the attachments so that malicious ones
should not be recognised and executed by the browser.
@ara4n
Copy link
Member

ara4n commented Apr 29, 2018

Right, i think i've finished the remaining bits here and have opened up a new PR for it (#1864), which supersedes this one. @walle303; sorry for picking it up; i assumed you had shelved it as you didn't respond to the earlier feedback. many thanks for doing all the original legwork!

@ara4n ara4n closed this Apr 29, 2018
@walle303 walle303 deleted the patch-bloburi branch April 29, 2018 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disasterous performance on FF on rooms with large encrypted images
4 participants