Introduce mitigations for CVE-2020-6506

[kelset]

Hey folks 👋

This PR wants to introduce the recommended mitigation for CVE-2020-6506 as indicated in the blogpost by the security researcher who disclosed it.

Enable multiwindow support. If needed, implementation options exist to mimic single-window behavior and minimize breaking changes. Does not require multi-tab UI. Suitable for browsers and frameworks.

The details

The final code that you see here is, basically, a mix my brain plus the following links:

• Expose the setSupportMultipleWindows in Android to fix CVE-2020-6506 #1663
• [webview_flutter] [url_launcher] Handle Multiwindows in WebViews  flutter/plugins#2991
• setSupportMultipleWindows
  • in particular this needed to override onCreateWindow
• https://stackoverflow.com/questions/48040319/setsupportmultiplewindows-and-setjavascriptcanopenwindowsautomatically-not-allow
• https://stackoverflow.com/questions/8817485/open-javascript-window-into-alertdialog

All of this to say, I haven't been doing Android for a while so I'm not entirely sure this implementation is the best ever.

That said, it actually works as expected: now, when...

...flag is set to true:

...flag is set to false:

Testing and stuff

Basically, as you'll quickly notice, I've added to the Examples folder a new example which is literally just the snippet @jamonholmgren used in the other PR to test if the code was working.
That way, you can easily test this PR by checkout-ing it, and doing yarn then yarn start:android and toggling the flag.

That's all, let me know if there's anything else I need to do to make it ready for release 👍

PS: I don't think this should be treated as a breaking change, from the perspective of releases. But as long as it get released, I'm happy either way :D

[tido64]

Until this vulnerability is fixed, would it make sense to prevent people from disabling it?

[kelset]

Technically the vulnerability in Android WebView is already fixed: https://alesandroortiz.com/articles/uxss-android-webview-cve-2020-6506#android-users (this is just a mitigation)

But since we can't really control nor force this kind of update (but I seem to recall google services being quite "aggressive" in self updating) I don't think we should lock it. In a way it, it will be already forced since it's a new setting that will be defaulted to "true", so by simply bumping the webview version you'll have this behaviour. But I think it'd be much better to allow folks who want to revert the behaviour change to be able to do so.

[jamonholmgren]

I agree with keeping it a flag, at least for now.

[Titozzz]

Hey @kelset thanks for spending some time on this. Do you think there is a way to keep the current behavior with multiple windows set to true ? (Opening link would just navigate to it in a new webview)

If not this is still a working fix but will be released as breaking. Let me know ,😉

[kelset]

Do you think there is a way to keep the current behavior with multiple windows set to true ? (Opening link would just navigate to it in a new webview)

I'm not sure 😅 I can try to take a look today, but if I can't find a solution for it by end of the day I'd recommend we first merge this as to fix the CVE, and then iterate on the implementation so to get to the optimal result (maybe by then some other more knowledgeable folks will be able to jump in and help with that 😅).

If not this is still a working fix but will be released as breaking.

👍

[kelset]

Hey @Titozzz, an update on the PR:

1. I'm glad I double checked the code locally to try and figure out if what you were asking was doable - when cleaning up my code for the PR after testing I removed one line too many 😂 But now I've fixed my mistake by pushing a new commit 👍

2. about what you were asking:

Do you think there is a way to keep the current behaviour with multiple windows set to true ?

The answer is not as simple as I was hoping for. I've done a quick pairing session with a colleague and there are a few points I wanted to share:

a) the behaviour currently implemented (of the app opening the new link outside) is a viable/expected Android behaviour.
b) While re-reading the documentation for onCreateWindow(which we are forced to override because it's required by setSupportMultipleWindows), one section stood out:

Applications should be careful how they display the new window: don't simply overlay it over the existing WebView as this may mislead the user about which site they are viewing. If your application displays the URL of the main page, make sure to also display the URL of the new window in a similar fashion. If your application does not display URLs, consider disallowing the creation of new windows entirely.

So tbh I think at - at least for now - we should go with this new behaviour.
Trying to keep the new webview inside would also, kind of, force a more complicated implementation which requires showing the URLs per both webview, as asked by the documentation (and I couldn't see any prop currently available for that - but if there actually is already a "show url" flag of sort please let me know 🤗).

[jamonholmgren]

Ready to go on this.

[jamonholmgren]

Thanks to everyone who contributed to this fix! Release should be out shortly.

[react-native-community-bot]

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[mrcoinbase]

Thanks all!

[abumalick]

Thank you all for working on that 👍

[awkpagong]

@kelset @jamonholmgren @Titozzz thanks for working on this! If I'd like to handle target="_blank" links manually, do I need to set setSupportMultipleWindows to false? Prior to this PR my app was inspecting urls in onShouldStartLoadWithRequest and if the url wasn't from my domain I'd show an alert to the user asking them if they'd like to open the link in their browser. After this PR, target="_blank" links bypass onShouldStartLoadWithRequest and open directly in the browser. Are there any security concerns with doing something like this?:

<WebView 
   setSupportMultipleWindows={false}
   // ...
   onShouldStartLoadWithRequest={({ url }) => {
       if (isUrlFromMyDomain(url)) {
         return true;
      } else {
         Alert.alert("Unsupported URL", "Leave app?", [{ text: "No", style: "cancel" }, { text: "Yes", onPress: () => { Linking.openURL(url); }}], { cancelable: false });
         return false;
      }
   }}
/>

Ideally I'd want to continue prompting the user before booting them from the webview into their browser, but I want to make sure I'm not opening myself up to any security holes by doing this...

[TfADrama]

@kelset @jamonholmgren @Titozzz thanks for working on this! If I'd like to handle target="_blank" links manually, do I need to set setSupportMultipleWindows to false? Prior to this PR my app was inspecting urls in onShouldStartLoadWithRequest and if the url wasn't from my domain I'd show an alert to the user asking them if they'd like to open the link in their browser. After this PR, target="_blank" links bypass onShouldStartLoadWithRequest and open directly in the browser. Are there any security concerns with doing something like this?:

<WebView 
   setSupportMultipleWindows={false}
   // ...
   onShouldStartLoadWithRequest={({ url }) => {
       if (isUrlFromMyDomain(url)) {
         return true;
      } else {
         Alert.alert("Unsupported URL", "Leave app?", [{ text: "No", style: "cancel" }, { text: "Yes", onPress: () => { Linking.openURL(url); }}], { cancelable: false });
         return false;
      }
   }}
/>

Ideally I'd want to continue prompting the user before booting them from the webview into their browser, but I want to make sure I'm not opening myself up to any security holes by doing this...

Hey
You found any solution for your situation?