Comment on attachment 9094356 [details]
Bug 1582857. Fix security checks around this-unwrapping. r=peterv
Security Approval Request
- How easily could an exploit be constructed based on the patch?: That is a good question.
The actual security impact here is that if two pages start out same-origin, but then set
document.domain differently so that they are no longer same-origin, this bug allows calling arbitrary DOM methods/getters/setters on the now-cross-origin window. You just have to grab the method/getter/setter from your own window, and
.call() it, passing the other window as
How to actually exploit that, while not having the ability to just inject script into the other window before your
document.domain values diverge, is an interesting question. And of course this only affects sites that use
document.domain and depend on the no-longer-same-origin aspect of it somehow.
With all that out of the way, I think it's pretty clear based on the function naming that this has to do with security checks for "this" values of DOM methods, and that the objects involved are the ones that are normally accessible cross-origin. I suspect anyone competent would figure out that means Window and Location, and likely guess that this has to do with using those as
this values for DOM calls, at which point they can pretty easily discover the problem. Whether they can then exploit it is an interesting question.
I could probably obfuscate the commit message some if I had to, but I am really not sure it buys us much given the way he code looks.
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
- Which older supported branches are affected by this flaw?: 67 and newer
- If not all supported branches, which bug introduced the flaw?: Bug 1514050
- Do you have backports for the affected branches?: Yes
- If not, how different, hard to create, and risky will they be?:
- How likely is this patch to cause regressions; how much testing does it need?: I think this should not cause any regressions, but a bit of shaking out would be nice, especially because this patch does make us throw exceptions in cases where we currently don't (though other browsers do).