Make isNested safe #4515
Make isNested safe #4515
Conversation
When treating a foreign object (such as a frame window), access throws.
| do you have more context so that we can tell it's a mis-use or not? |
| When manipulating another frame's window that has a different domain, only some of its fields are accessible (e.g. I have some code that generates |
| IIUC -- for some object called `o`, even if it's not null/undefined, o.some_random_property will still be raised for security reasons, is that correct? do you have some cleaner patches in mind …On Fri, Jul 10, 2020 at 1:36 PM Matthias Le Brun ***@***.***> wrote: When manipulating another frame's window that has a different domain, only some of its fields are accessible (e.g. postMessage), and the other throw: https://html.spec.whatwg.org/multipage/browsers.html#integration-with-idl I have some code that generates Caml_option.some(window.parent), which throws because it tries to access BS_PRIVATE_NESTED_SOME_NONE. The previous method worked by chance, as it used to check the 0 property to compare it to undefinedHeader, andforeingWindow[0] is accessible without a security error. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4515 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFWMK7XHRNVO4PZ2XO4EULR22SFNANCNFSM4OUZYJJQ> . -- Regards -- Hongbo Zhang |
That's right. For these objects, only a certain list of properties are accessible without having an error being thrown.
We could check if the object is a window, but that seems a bit too specific. Something like: let isNested (x : Obj.t) : bool = (Obj.magic x)##window != x && (* false if the object is of window type *) Obj.repr ((Obj.magic x : nested).depth) != Obj.repr Js.undefined |
| @bobzhang After giving it some thoughts, I think the try catch approach seems a bit more solid as it can cover some edge cases with ES6 proxies too |
| so `obj.private_property` throws in some cases, will `obj instanceof xx` fail for security reasons? …On Tue, Jul 14, 2020 at 1:38 AM Matthias Le Brun ***@***.***> wrote: @bobzhang <https://github.com/bobzhang> After giving it some thoughts, I think the try catch approach seems a bit more solid as it can cover some edge cases with ES6 proxies too — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4515 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFWMK2WRWVBZW6U4BJ3YA3R3NBCTANCNFSM4OUZYJJQ> . -- Regards -- Hongbo Zhang |
| Will optional chaining (?.) throw in your case? …On Tue, Jul 14, 2020 at 10:05 AM Bob Zhang ***@***.***> wrote: so `obj.private_property` throws in some cases, will `obj instanceof xx` fail for security reasons? On Tue, Jul 14, 2020 at 1:38 AM Matthias Le Brun ***@***.***> wrote: > @bobzhang <https://github.com/bobzhang> After giving it some thoughts, I > think the try catch approach seems a bit more solid as it can cover some > edge cases with ES6 proxies too > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#4515 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAFWMK2WRWVBZW6U4BJ3YA3R3NBCTANCNFSM4OUZYJJQ> > . > -- Regards -- Hongbo Zhang -- Regards -- Hongbo Zhang |
| instanceof wouldn't work in my case cause the window object isn't an instance of the Window BS has access to. Optional chaining would fail (at least the polyfill would for sure). |
| @bobzhang do you have some other solution in mind? |
| sorry for the late reply. Ideally I would like to have a way to reproduce so that I can make a good judgement. Is there an easy way to reproduce the security issue on my side? …On Wed, Jul 29, 2020 at 4:47 PM Matthias Le Brun ***@***.***> wrote: @bobzhang <https://github.com/bobzhang> do you have some other solution in mind? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4515 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFWMK5SXCUL7DJD5XDUI43R57O3TANCNFSM4OUZYJJQ> . -- Regards -- Hongbo Zhang |
| @bobzhang https://github.com/bloodyowl/bucklescript-is-nested-repro With two examples that trigger that kind of error: https://github.com/bloodyowl/bucklescript-is-nested-repro/blob/master/src/Demo.re |
| hi @bloodyowl I cloned the repo, followed the instructions but could not reproduce such things. What I need is simple(does not have to contain ml code), a single html with iframe to demonstrate the security issues, thanks |
<!DOCTYPE html> <html> <iframe id="frame" src="https://bucklescript.github.io"></iframe> <script> document.querySelector("#frame").addEventListener("load", () => { console.log("that's ok:") console.log( document.querySelector("#frame").contentWindow.postMessage ); console.log("that's not:") console.log( document.querySelector("#frame").contentWindow.BS_PRIVATE_NESTED_SOME_NONE ); }); </script> </html> |
| thanks for the example, now I have a better understanding what's going on. If it is not the same origin, even this will raise (without even accessing the window): This is a very niche area and cross origin iframe is some thing you should be careful |
| Yeah I'm aware of the limitations and I know what I'm doing 😄
That'll raise only because you're not immediately accessing an authorised property (such as The issue is that now, the |
| @bloodyowl this is a niche case when do the cross origin iframe. There are lots of workaround, like instead of wrapping it as an option, you can wrap it using |
| @bobzhang It's a bit more inconvenient than what you describe (as It also prevents me from passing it directly in an optional labeled arguments) but I get it. I hope that'll be reconsidered some day 🙂 |
2 participants
When treating a foreign object (such as a frame window), access throws.