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

[feat] Inject window.__TAURI__ in allowed remote URLs #5088

Closed
Sytten opened this issue Aug 29, 2022 · 20 comments
Closed

[feat] Inject window.__TAURI__ in allowed remote URLs #5088

Sytten opened this issue Aug 29, 2022 · 20 comments

Comments

@Sytten
Copy link

Sytten commented Aug 29, 2022

Describe the problem

Currently it is not really possible to communicate directly with tauri from an external website. There is a workaround if you have a static external URL you want to load, but it does not work if you need to inject in arbitrary URLs.

In our case, we have a first window where the user can manage their connection. They can then connect to a remote server which provides the UI. That UI needs to communicate with tauri to store credentials on the user machine, access notifications, etc.

Describe the solution you'd like

Basically I would like a setting in the WindowBuilder to allow injection of the window.__TAURI__ in any website loaded in that window. I do understand that it can be security risk if people are not careful so that could be gated with a feature flag or a general configuration option that needs to be enabled (in the context for example) before the setting has any effect.

Alternatives considered

We are currently investigating our options:

  • Create a local server so the remote url can talk to it and in turn execute stuff in the rust context
  • Use a static wrapper page with an iframe that loads the remote URL so that the static page can relay the messages from the iframe to tauri

Neither are very elegant solutions and I would prefer to be able to communicate directly with tauri.

Additional context

No response

@Sytten
Copy link
Author

Sytten commented Aug 29, 2022

It looks like __TAURI_POST_MESSAGE__ is always defined and can be used to send messages. This should probably be documented? It feels like a security loophole.

@FabianLars
Copy link
Member

Yeah it's not supposed to be defined. We just forget to remove it again and again x)

It's not really a security loophole tho because the webview's ipc channel is always defined and tauri_post_message is nothing more than a really slim wrapper around that https://github.com/tauri-apps/tauri/blob/dev/core/tauri/src/app.rs#L954

@Sytten
Copy link
Author

Sytten commented Aug 29, 2022

Yeah that is what I saw, I think my thing works because I start the window as a "local" window so the IPC handler is injected

if is_local {
let label = pending.label.clone();
pending = self.prepare_pending_window(
pending,
&label,
window_labels,
app_handle.clone(),
web_resource_request_handler,
)?;
pending.ipc_handler = Some(self.prepare_ipc_handler(app_handle));
}
and then I redirect to the external URL so the IPC handler is still valid. This is probably a "bug" but I am happy to exploit it until I can get a real __TAURI__.

@Sytten
Copy link
Author

Sytten commented Aug 29, 2022

For reference we are doing:

 let url = WindowUrl::App("/invalid".into());
 WindowBuilder::new(manager, "label", url).visible(false).maximized(true)
...
let main_window = window.get_window("label").unwrap()
let url = format!("http://{}:{}/", &host, port);
main_window.redirect(&url);

@FabianLars
Copy link
Member

Interesting, that was indeed a bug in beta which we fixed in a release candidate version (proabably the first one as part of the audit, not sure) 🤔

@Sytten
Copy link
Author

Sytten commented Aug 29, 2022

If you are going to fix it, I would very much like to be involved since we need the escape hatch for our application. Happy to contribute coding wise for it. Feel free to ping me on discord.

@Sytten
Copy link
Author

Sytten commented Aug 29, 2022

Just to be clear, the __TAURI__ property is removed on redirect, but the underlining window.ipc is still present and the handler is still the same so you can send commands/events to the tauri application. This works because the is_local variable was true at the time of window creation but is not checked again on direct so the ipc handler is not changed. This is problematic if the application has an open redirect as the attacker could talk with tauri directly. In our case we designed our commands to be safe and we benefit rom this behaviour but I don't think it's correct.

@tweidinger
Copy link
Contributor

Yeah it's correct that this is an security issue, which will get priority for a fix. We will probably (completely/temporarily) prevent such behavior in the next release/security patch, unless there is a feature addition available to optionally allow third party API access. We are completely open to this change but will prioritize the security of all users.

As you suggested to help out with contributions on this issue and to keep your application working in a similar fashion, it would be great if you could contribute a (draft) PR for this added functionality. If you have questions, comments, remarks etc. you can always ask in the contributor channels in discord.

For your use case would it be sufficient to have your 'trusted' external URLs in an allowlist addition in the security configuration and then toggle a configuration flag like dangerousExternalCommandAccess to enable this access for the configured sites in the scope? Obviously this requires changes in several places in the core functionality.

Example of the addition:

"dangerousExternalCommandAccess": {
         "scope": [
             {
               "name": "mytrustedcompany",
               "url": "https://www.mytrustedcompany.com",
              },
         ]
 }

This could be used to determine if the current host of the webview window is allowed to interact with the API.

On another note: Would you mind sharing how you currently workaround this?

@FabianLars
Copy link
Member

On another note: Would you mind sharing how you currently workaround this?

Idk about their case, but the easiest thing, if you don't wanna reconstruct tauri's request style, is to just inject the core.js file manually.

@Sytten
Copy link
Author

Sytten commented Sep 5, 2022

Thanks for the reply. Currently we didn't ship the feature yet so this fix won't impact us but it will prevent us from moving forward. I also think security is more important.

We do not know the urls at build time as they are entered by the user at runtime. So I don't think the proposal laid out will work for us. I think the idea of a flag in the configuration is good, but the whitelist need to be configurable per window at runtime and must allow a wildcard ideally. I will draft a PR for what we would need and we can discuss from there I think.

Note that must injecting the core.js script wont be enough once the patch for IPC will be released. Really the JS part is super basic, what we need is the window.ipc.

@FabianLars
Copy link
Member

I think this scope config would be a good fit for tauri v1, because we said time and time again that the ipc doesn't work on external pages (even tho it does) and that we don't support browser-like use-cases yet.

So this way we can fix v1 to match the behavior we always presented, and remove the need for weird workarounds for most use-cases to not make it a super harsh breaking change. And then we can tackle more loose scopes for v2, like planned.

@Sytten
Copy link
Author

Sytten commented Sep 5, 2022

Agreed, I will move toward that solution. But I will accept wildcards in the urls so it is easy for us to just accept every external website. I think there could be a better implementation for v2 to allow users to add dynamic checks at runtime but this will be good enough for now.

@FabianLars
Copy link
Member

But I will accept wildcards in the urls so it is easy for us to just accept every external website

I should have made that clearer in my previous comment, but the problem i see here is that the v1 audit was building on top of the "no apis on external sites" premise. The actual fix for this would be to disable the ipc for external sites without an opt-in config. But because quite a few people depend on this (even though we said again and again that it "doesn't work"), the scope @tweidinger prooposed is already quite generous (sounds super entitled but i don't know a better word) in the context of a v1 feature release, and i think we can't completely open it up (via wildcards) in a non-major release because this really looks like something that needs a large external audit (as it was planned all along).
Btw v2 is not that far in the future, we're not talking about years of waiting for the wildcard.

I guess this is more of a question for @tweidinger, @lucasfernog and @chippers, but i wanted to voice my point of view anyway 🤷

p.s. sorry for the triple ping, didn't want to talk about you guys without giving you a chance to react ✌️

@Sytten
Copy link
Author

Sytten commented Sep 5, 2022

I understand and it makes total sense, but this won't work for us and waiting is not really an option for a startup. So I will most likely fork tauri or stay on a "broken" version until v2 is released.
That being said I am working on my solution and I will push a draft PR today.

@Sytten
Copy link
Author

Sytten commented Sep 5, 2022

Sorry for the double ping, but I realized the IPC is used both for modules and for (developer defined) commands.
I am wondering if it would be more acceptable to the team if only commands were available vs all the modules (which include some pretty intense stuff). That would be enough for us and would really limit the scope of external attacks.
That means only part of TAURI would be accessible too.

@FabianLars
Copy link
Member

the modules and user commands are basically the same thing before the call reaches the code where the request is actually read. Preferably it wouldn't even get this far, for example by disabling webview.ipc altogether using something like this for example https://docs.microsoft.com/en-us/dotnet/api/microsoft.web.webview2.core.corewebview2settings.iswebmessageenabled?view=webview2-dotnet-1.0.1293.44#microsoft-web-webview2-core-corewebview2settings-iswebmessageenabled - idk if the other platforms have a similar setting.

That would be my ideal solution. The only drawback would be that the user wouldn't receive nicer custom errors but only whatever the webview would give them.
Also i don't have the everything about the ipc in my head right now, so maybeee this is not even compatible with it. Also there will be an ipc rewrite in v2 which won't rely much on the message ipc anymore so we'd have to extend that anyway 🤔

@Sytten
Copy link
Author

Sytten commented Sep 5, 2022

I did a kind dirty PoC for the access to commands (doesn't support isolation). We will most likely maintain a fork with this patchset for a while. One thing I was not sure was what to do with plugins, for now I disabled access.
I think my idea of IPCAccess has potential for v2. One thing that is currently annoying is that all the code assumes local == full access and external == no access. I think having a modular approach for v2 would be nicer/more flexible.

@JonasKruckenberg
Copy link
Member

JonasKruckenberg commented Sep 5, 2022

This sounds like something the window-specific state and commands idea would fix long-term.

@tweidinger and I had been talking about this. Basically instead of having one global store for commands and state, we would have potentially many and developers could decide which Store and IPC router to associate to a given window if any.

But as fabian already said, sorry this is not something we can quickly fix as anything IPC-related touches the very core of Tauri, but as long as maintaining a fork is acceptable you can rest assured improvements to the IPC system will be coming in v2 and v3 and beyond

@lukigarazus
Copy link

@lucasfernog where did this go? It's not available in Tauri 2.1.1

@FabianLars
Copy link
Member

https://v2.tauri.app/security/capabilities/#remote-api-access

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants