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

Add sandbox option and support native window.open #6919

Merged
merged 15 commits into from Sep 27, 2016

Conversation

Projects
None yet
7 participants
@tarruda
Contributor

tarruda commented Aug 21, 2016

This PR adds a sandboxed option that can be passed to webPreferences of BrowserWindow constructor. Passing this option will completely change the renderer implementation of the new window: It won't have any node.js integration, and all system interaction happens through IPC.

It also adds a --enable-sandbox command-line option that enables chromium OS-level sandbox for all renderers, and if this option is passed, then all BrowserWindow instances will have the sandboxed option passed automatically(since they would not work otherwise).

This PR also restores chromium native window.open API for sandboxed windows as there should be no issue in having multiple webContents for one sandboxed renderer(since there's no node.js context)

While the PR is relatively big, it is mostly made of required infrastructure changes to support the new code. The implementation is relatively simple: For renderers with the sandboxed option enabled, the AtomSandboxedRendererClient class is used. This class is unrelated to AtomRendererClient as it takes a very different approach to inject preload scripts and expose the ipcRenderer module(I've tried to reuse as much as possible, though). To support a basic commonjs environment in sandboxed renderers, browserify is used and integrated into the build system.

I've tried to make commits small and self contained as possible, so reading each commit individually is the recommended way to review.

Close #1865

@tarruda tarruda changed the title from Add sandbox option and support native window.open to [WIP] Add sandbox option and support native window.open Aug 22, 2016

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Aug 30, 2016

Contributor

Hi again

I'm done making changes and adding tests, so this is ready for review.

This PR currently has some issues:

  • The C++ implementation of sandboxed renderer shares little code with the non-sandboxed renderer. As a consequence, sandboxed renderers have some missing functionality(setting background color doesn't work, for example). This can be fixed by extracting a base common class for AtomRendererClient and AtomSandboxedRendererClient.
  • The javascript code that supports sandboxed renderers(lib/sandboxed_renderer/init.js) only shares part of ipcRenderer with the non-sandboxed version(lib/renderer/init.js), but nothing stops more code to be shared by the two. For example, I believe it is possible to expose the most electron/node.js API to sandboxed renderers by augmenting the browser-renderer RPC module.
  • Right now, the code which determines which navigations will create a new SiteInstance is reimplementing some logic from content module. This is necessary because renderers spawned from sandboxed navigations should also be sandboxed, and I've found no way of associating newly spawned renderers(AppendExtraCommandLineSwitches) with sandboxed parents other than reusing the pending process logic in OverrideSiteIntanceForNavigation. The ideal implementation would probably require patching libchromiumcontent to notify AtomBrowserClient whenever a new SiteInstance is created, and populating pending_processes_ with the renderer id for the new instance.

I want to fix all of these issues, but I prefer to do it in separate PRs as this one is very big already.

Another possible issue(depending on the POV) is that browserify is now a build dependency and is used to provide commonjs for preload scripts in sandboxed renderers. I've opted to use browserify because is a very mature browser commonjs implementation and also was the fastest path to get a working commonjs environment in sandboxed renders.

An alternative to using browserify would be to implement a commonjs environment that uses IPC to read files from disk using the main process, though I'd prefer to stick with browserify as it is cleaner than repeatedly loading a bunch of files via IPC.

Finally think its worth mentioning that this PR adds a lot of code but the existing use cases of electron should not be affected by the changes made here. My goal is to allow electron to be used in more scenarios, such as implementing a web browser that shares some of chromium security features.

Contributor

tarruda commented Aug 30, 2016

Hi again

I'm done making changes and adding tests, so this is ready for review.

This PR currently has some issues:

  • The C++ implementation of sandboxed renderer shares little code with the non-sandboxed renderer. As a consequence, sandboxed renderers have some missing functionality(setting background color doesn't work, for example). This can be fixed by extracting a base common class for AtomRendererClient and AtomSandboxedRendererClient.
  • The javascript code that supports sandboxed renderers(lib/sandboxed_renderer/init.js) only shares part of ipcRenderer with the non-sandboxed version(lib/renderer/init.js), but nothing stops more code to be shared by the two. For example, I believe it is possible to expose the most electron/node.js API to sandboxed renderers by augmenting the browser-renderer RPC module.
  • Right now, the code which determines which navigations will create a new SiteInstance is reimplementing some logic from content module. This is necessary because renderers spawned from sandboxed navigations should also be sandboxed, and I've found no way of associating newly spawned renderers(AppendExtraCommandLineSwitches) with sandboxed parents other than reusing the pending process logic in OverrideSiteIntanceForNavigation. The ideal implementation would probably require patching libchromiumcontent to notify AtomBrowserClient whenever a new SiteInstance is created, and populating pending_processes_ with the renderer id for the new instance.

I want to fix all of these issues, but I prefer to do it in separate PRs as this one is very big already.

Another possible issue(depending on the POV) is that browserify is now a build dependency and is used to provide commonjs for preload scripts in sandboxed renderers. I've opted to use browserify because is a very mature browser commonjs implementation and also was the fastest path to get a working commonjs environment in sandboxed renders.

An alternative to using browserify would be to implement a commonjs environment that uses IPC to read files from disk using the main process, though I'd prefer to stick with browserify as it is cleaner than repeatedly loading a bunch of files via IPC.

Finally think its worth mentioning that this PR adds a lot of code but the existing use cases of electron should not be affected by the changes made here. My goal is to allow electron to be used in more scenarios, such as implementing a web browser that shares some of chromium security features.

@tarruda tarruda changed the title from [WIP] Add sandbox option and support native window.open to Add sandbox option and support native window.open Aug 30, 2016

content::WebContents* web_contents = GetWebContentsFromProcessID(process_id);
if (WebContentsPreferences::IsSandboxed(web_contents)) {
AddSandboxedRendererId(host->GetID());

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

Can this be moved to BrowserClient::SiteInstanceGotProcess? So we can get rid of the locker.

@zcbenz

zcbenz Sep 2, 2016

Contributor

Can this be moved to BrowserClient::SiteInstanceGotProcess? So we can get rid of the locker.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

I only started playing with chromium content for this PR, so you tell me 😄

The locker is there because the sandboxed_renderers_ set is read from CanCreateWindow(IO thread)/ShouldCreateNewSiteInstance(UI thread) and written by RenderProcessWillLaunch/RenderProcessHostDestroyed which are both UI thread. BTW what is the simplest logic for determining which thread a method is called on, when no header comments are available? (other than printing the thread id to the console)

If you really want to get rid of the locker I suppose we could have a duplicate sandboxed_renderers_ set that is used by IO thread only(and updated with PostTask)

@tarruda

tarruda Sep 2, 2016

Contributor

I only started playing with chromium content for this PR, so you tell me 😄

The locker is there because the sandboxed_renderers_ set is read from CanCreateWindow(IO thread)/ShouldCreateNewSiteInstance(UI thread) and written by RenderProcessWillLaunch/RenderProcessHostDestroyed which are both UI thread. BTW what is the simplest logic for determining which thread a method is called on, when no header comments are available? (other than printing the thread id to the console)

If you really want to get rid of the locker I suppose we could have a duplicate sandboxed_renderers_ set that is used by IO thread only(and updated with PostTask)

This comment has been minimized.

@zcbenz

zcbenz Sep 6, 2016

Contributor

Thanks for the explanation, we can just use lock for now and see if we can do better optimization in future.

@zcbenz

zcbenz Sep 6, 2016

Contributor

Thanks for the explanation, we can just use lock for now and see if we can do better optimization in future.

@@ -140,8 +185,7 @@ void AtomBrowserClient::OverrideSiteInstanceForNavigation(
return;
}
// Restart renderer process for all navigations except "javacript:" scheme.
if (url.SchemeIs(url::kJavaScriptScheme))
if (!ShouldCreateNewSiteInstance(browser_context, current_instance, url))

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

Returning without changing new_instance will make Chromium use the original logic, so there is no need to duplicate the default behavior in ShouldCreateNewSiteInstance.

@zcbenz

zcbenz Sep 2, 2016

Contributor

Returning without changing new_instance will make Chromium use the original logic, so there is no need to duplicate the default behavior in ShouldCreateNewSiteInstance.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

This is what I did in my initial implementation, however I've hit the following problem:

Right now, the code which determines which navigations will create a new SiteInstance is reimplementing some logic from content module. This is necessary because renderers spawned from sandboxed navigations should also be sandboxed, and I've found no way of associating newly spawned renderers(AppendExtraCommandLineSwitches) with sandboxed parents other than reusing the pending process logic in OverrideSiteIntanceForNavigation. The ideal implementation would probably require patching libchromiumcontent to notify AtomBrowserClient whenever a new SiteInstance is created, and populating pending_processes_ with the renderer id for the new instance.

@tarruda

tarruda Sep 2, 2016

Contributor

This is what I did in my initial implementation, however I've hit the following problem:

Right now, the code which determines which navigations will create a new SiteInstance is reimplementing some logic from content module. This is necessary because renderers spawned from sandboxed navigations should also be sandboxed, and I've found no way of associating newly spawned renderers(AppendExtraCommandLineSwitches) with sandboxed parents other than reusing the pending process logic in OverrideSiteIntanceForNavigation. The ideal implementation would probably require patching libchromiumcontent to notify AtomBrowserClient whenever a new SiteInstance is created, and populating pending_processes_ with the renderer id for the new instance.

This comment has been minimized.

@zcbenz

zcbenz Sep 6, 2016

Contributor

👍

@zcbenz

zcbenz Sep 6, 2016

Contributor

👍

This comment has been minimized.

@tarruda

tarruda Sep 6, 2016

Contributor

What do you think about patching libchromiumcontent to notify AtomBrowserClient whenever a new RenderProcessHost is created?

I suggest we add a new method(eg: ContentBrowserClient::OnProcessCreatedForNavigation) that is called called after the RenderProcessHost is created but before the process is launched(and passed arguments similar to OverrideSiteInstanceForNavigation), so we have the process ID and can populate pending_process_ before AppendExtraCommandLineSwitches is called.

If you agree I can send a PR to https://github.com/electron/libchromiumcontent

@tarruda

tarruda Sep 6, 2016

Contributor

What do you think about patching libchromiumcontent to notify AtomBrowserClient whenever a new RenderProcessHost is created?

I suggest we add a new method(eg: ContentBrowserClient::OnProcessCreatedForNavigation) that is called called after the RenderProcessHost is created but before the process is launched(and passed arguments similar to OverrideSiteInstanceForNavigation), so we have the process ID and can populate pending_process_ before AppendExtraCommandLineSwitches is called.

If you agree I can send a PR to https://github.com/electron/libchromiumcontent

This comment has been minimized.

@zcbenz

zcbenz Sep 12, 2016

Contributor

What do you think about patching libchromiumcontent to notify AtomBrowserClient whenever a new RenderProcessHost is created?

It depends on how large the patch would be since we need to maintain the patch for every Chrome update, if it just inserts a simple hook like other patches, then I'm 👍 with it.

@zcbenz

zcbenz Sep 12, 2016

Contributor

What do you think about patching libchromiumcontent to notify AtomBrowserClient whenever a new RenderProcessHost is created?

It depends on how large the patch would be since we need to maintain the patch for every Chrome update, if it just inserts a simple hook like other patches, then I'm 👍 with it.

This comment has been minimized.

@ahmedmohamedali

ahmedmohamedali Sep 27, 2016

Contributor

Hi @tarruda,

My understanding of your implementation is the navigation to a different site from a sandboxed renderer will not be sanboxed but the renderer will run in a separate process. If I'm wrong can you explain how the sandboxing is enabled in that case ? Thanks.

@ahmedmohamedali

ahmedmohamedali Sep 27, 2016

Contributor

Hi @tarruda,

My understanding of your implementation is the navigation to a different site from a sandboxed renderer will not be sanboxed but the renderer will run in a separate process. If I'm wrong can you explain how the sandboxing is enabled in that case ? Thanks.

This comment has been minimized.

@tarruda

tarruda Sep 27, 2016

Contributor

@ahmedmohamedali new renderers created as a result of navigating from a sandboxed renderer should always be sandboxed.

@tarruda

tarruda Sep 27, 2016

Contributor

@ahmedmohamedali new renderers created as a result of navigating from a sandboxed renderer should always be sandboxed.

Show outdated Hide outdated lib/browser/api/browser-window.js
@@ -26,6 +27,33 @@ BrowserWindow.prototype._init = function () {
ipcMain.emit('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', event, url, frameName, disposition, options)
})
this.webContents.on('-web-contents-created', (event, webContents, url,
frameName) => {
pendingWebContents.set(webContents, {url, frameName})

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

We usually use v8Util.setHiddenValue to do this, so there is no need to worry about object's lifetime.

@zcbenz

zcbenz Sep 2, 2016

Contributor

We usually use v8Util.setHiddenValue to do this, so there is no need to worry about object's lifetime.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

👍

@tarruda

tarruda Sep 2, 2016

Contributor

👍

Show outdated Hide outdated atom/browser/web_contents_preferences.cc
base::DictionaryValue& web_preferences = self->web_preferences_;
bool sandboxed = false;
web_preferences.GetBoolean("sandboxed", &sandboxed);

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

Maybe just sandbox? The ed suffix seems redundant.

And can you also add this option to docs? We should also add a new page on which features are allowed in the sandboxed page and how to use it.

@zcbenz

zcbenz Sep 2, 2016

Contributor

Maybe just sandbox? The ed suffix seems redundant.

And can you also add this option to docs? We should also add a new page on which features are allowed in the sandboxed page and how to use it.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

Maybe just sandbox? The ed suffix seems redundant.

👍

And can you also add this option to docs?

Sure, I was planning to update the docs after reviews.

We should also add a new page on which features are allowed in the sandboxed page and how to use it.

As I mentioned in my previous comments, I think it is possible to implement most feature available for normal node.js enabled renderers(even node.js API, which can be proxied through ipcRenderer), but I'd rather do it in future PRs to limit the size of this PR.

If its ok with you, I'd rather document sandbox option limitations in a future PR, after I've refactored enough to put as many features in sandboxed renderers as possible.

@tarruda

tarruda Sep 2, 2016

Contributor

Maybe just sandbox? The ed suffix seems redundant.

👍

And can you also add this option to docs?

Sure, I was planning to update the docs after reviews.

We should also add a new page on which features are allowed in the sandboxed page and how to use it.

As I mentioned in my previous comments, I think it is possible to implement most feature available for normal node.js enabled renderers(even node.js API, which can be proxied through ipcRenderer), but I'd rather do it in future PRs to limit the size of this PR.

If its ok with you, I'd rather document sandbox option limitations in a future PR, after I've refactored enough to put as many features in sandboxed renderers as possible.

This comment has been minimized.

@zcbenz

zcbenz Sep 6, 2016

Contributor

👍 on documentation in future.

@zcbenz

zcbenz Sep 6, 2016

Contributor

👍 on documentation in future.

height: height || 600,
webContents: webContents
}
ipcMain.emit('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', event, url, frameName, disposition, options)

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

I think we should let NativeWindow manage the webContents, letting guest-window-manager do the management makes the whole logic too complicated.

It can be done by overriding a method of WebContentsObserver in NativeWindow.

@zcbenz

zcbenz Sep 2, 2016

Contributor

I think we should let NativeWindow manage the webContents, letting guest-window-manager do the management makes the whole logic too complicated.

It can be done by overriding a method of WebContentsObserver in NativeWindow.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

OK, I will have a look into this

@tarruda

tarruda Sep 2, 2016

Contributor

OK, I will have a look into this

This comment has been minimized.

@tarruda

tarruda Sep 26, 2016

Contributor

@zcbenz It's not clear what you want me to do. Should new BrowserWindow() be invoked directly from here?

@tarruda

tarruda Sep 26, 2016

Contributor

@zcbenz It's not clear what you want me to do. Should new BrowserWindow() be invoked directly from here?

ipcMain.on('ELECTRON_BROWSER_READ_FILE', function (event, file) {
fs.readFile(file, (err, data) => {
if (err) event.returnValue = {err: err.message}
else event.returnValue = {data: data.toString()}

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

The buffer can be sent directly, there is no need to convert it to string.

@zcbenz

zcbenz Sep 2, 2016

Contributor

The buffer can be sent directly, there is no need to convert it to string.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

👍

@tarruda

tarruda Sep 2, 2016

Contributor

👍

This comment has been minimized.

@tarruda

tarruda Sep 26, 2016

Contributor

Actually, the code that handles Buffer deserialization is in lib/renderer/api/remote.js, which is not used yet by sandboxed renderers. I will address this in another PR, as described by #6712

@tarruda

tarruda Sep 26, 2016

Contributor

Actually, the code that handles Buffer deserialization is in lib/renderer/api/remote.js, which is not used yet by sandboxed renderers. I will address this in another PR, as described by #6712

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Sep 2, 2016

Contributor

Can you enable this for WebView tags (Guest Views) as well?

Contributor

paulcbetts commented Sep 2, 2016

Can you enable this for WebView tags (Guest Views) as well?

Show outdated Hide outdated atom/renderer/atom_sandboxed_renderer_client.cc
ss << "})";
std::string preload_wrapper = ss.str();
// Compile the wrapper and run it to get the function object
auto script = v8::Script::Compile(v8::String::NewFromUtf8(

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

You can use mate::ConvertToV8 helper to convert things.

@zcbenz

zcbenz Sep 2, 2016

Contributor

You can use mate::ConvertToV8 helper to convert things.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

👍

@tarruda

tarruda Sep 2, 2016

Contributor

👍

Show outdated Hide outdated atom/renderer/atom_sandboxed_renderer_client.cc
v8::NewStringType::kNormal).ToLocalChecked()
};
// Execute the function with proper arguments
(void)func->Call(context, v8::Null(isolate), 2, args);

This comment has been minimized.

@zcbenz

zcbenz Sep 2, 2016

Contributor

There is a ignore_result helper to ignore results, the (void) hack should be avoided.

@zcbenz

zcbenz Sep 2, 2016

Contributor

There is a ignore_result helper to ignore results, the (void) hack should be avoided.

This comment has been minimized.

@tarruda

tarruda Sep 2, 2016

Contributor

👍

@tarruda

tarruda Sep 2, 2016

Contributor

👍

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 2, 2016

Contributor

Can you enable this for WebView tags (Guest Views) as well?

Yes I want to do that, but I'm not sure the amount of work required, I'm still trying to figure out how the content module components connect to each other.

If this is a simple task and you can show me the sections of code that handle webview tags, I can do it here, but I'd rather implement in a future PR due to the size of this one.

About this request: I assume you mean we should provide a way to allow a sandboxed <webview> tag to be created from a non-sandboxed electron window, or there would be a security hole if any page could create these tags.

Since one of the reasons I'm adding the sandboxed option is to allow electron to be run with OS-level sandbox enabled, would it be possible to enable a webview tag only to the top-level sandboxed window, the one created from the main process?

I ask this because it would be nice to implement a sandboxed web browser in electron using <webview> to display the web contents and javascript/html to implement the browser UI such as tabs and menus, but clearly this top-level sandboxed renderer would need to have some privileges not available to pages displayed by <webview>

Contributor

tarruda commented Sep 2, 2016

Can you enable this for WebView tags (Guest Views) as well?

Yes I want to do that, but I'm not sure the amount of work required, I'm still trying to figure out how the content module components connect to each other.

If this is a simple task and you can show me the sections of code that handle webview tags, I can do it here, but I'd rather implement in a future PR due to the size of this one.

About this request: I assume you mean we should provide a way to allow a sandboxed <webview> tag to be created from a non-sandboxed electron window, or there would be a security hole if any page could create these tags.

Since one of the reasons I'm adding the sandboxed option is to allow electron to be run with OS-level sandbox enabled, would it be possible to enable a webview tag only to the top-level sandboxed window, the one created from the main process?

I ask this because it would be nice to implement a sandboxed web browser in electron using <webview> to display the web contents and javascript/html to implement the browser UI such as tabs and menus, but clearly this top-level sandboxed renderer would need to have some privileges not available to pages displayed by <webview>

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 2, 2016

Contributor

@zcbenz It's not clear whats the rebase/merge policy I should follow: Do I keep this branch updated by rebasing or merging master branch?

Contributor

tarruda commented Sep 2, 2016

@zcbenz It's not clear whats the rebase/merge policy I should follow: Do I keep this branch updated by rebasing or merging master branch?

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Sep 6, 2016

Contributor

@zcbenz It's not clear whats the rebase/merge policy I should follow: Do I keep this branch updated by rebasing or merging master branch?

I'm good with either, we are currently doing a Chrome upgrade so you probably want to do it later when we merged the Chrome upgrade pull request.

Contributor

zcbenz commented Sep 6, 2016

@zcbenz It's not clear whats the rebase/merge policy I should follow: Do I keep this branch updated by rebasing or merging master branch?

I'm good with either, we are currently doing a Chrome upgrade so you probably want to do it later when we merged the Chrome upgrade pull request.

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Sep 6, 2016

Contributor

Also I think we need opinions from Brave browser team on this since they are already doing a similar thing in their fork, /cc @bridiver @bbondy @darkdh.

And does anyone know whom from OpenFin to ping? I heard their are also doing a similar patch.

Contributor

zcbenz commented Sep 6, 2016

Also I think we need opinions from Brave browser team on this since they are already doing a similar thing in their fork, /cc @bridiver @bbondy @darkdh.

And does anyone know whom from OpenFin to ping? I heard their are also doing a similar patch.

GregoryGatellier added a commit to thomsonreuters/electron that referenced this pull request Sep 15, 2016

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Sep 20, 2016

Contributor

@tarruda Do you mind rebasing the branch on master? Since no one is opposed against this I would like to get it merged.

Contributor

zcbenz commented Sep 20, 2016

@tarruda Do you mind rebasing the branch on master? Since no one is opposed against this I would like to get it merged.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 20, 2016

Contributor

@zcbenz Ok but I still need to address some of your comments. I should be able to finish the requested fixes + rebase by tomorrow.

About this concern:

Right now, the code which determines which navigations will create a new SiteInstance is reimplementing some logic from content module. This is necessary because renderers spawned from sandboxed navigations should also be sandboxed, and I've found no way of associating newly spawned renderers(AppendExtraCommandLineSwitches) with sandboxed parents other than reusing the pending process logic in OverrideSiteIntanceForNavigation. The ideal implementation would probably require patching libchromiumcontent to notify AtomBrowserClient whenever a new SiteInstance is created, and populating pending_processes_ with the renderer id for the new instance.

Next week I might be able to start looking into the libchromiumcontent patch that enables fully reusing chromium cross site navigation logic, and will send another PR after it is ready.

Contributor

tarruda commented Sep 20, 2016

@zcbenz Ok but I still need to address some of your comments. I should be able to finish the requested fixes + rebase by tomorrow.

About this concern:

Right now, the code which determines which navigations will create a new SiteInstance is reimplementing some logic from content module. This is necessary because renderers spawned from sandboxed navigations should also be sandboxed, and I've found no way of associating newly spawned renderers(AppendExtraCommandLineSwitches) with sandboxed parents other than reusing the pending process logic in OverrideSiteIntanceForNavigation. The ideal implementation would probably require patching libchromiumcontent to notify AtomBrowserClient whenever a new SiteInstance is created, and populating pending_processes_ with the renderer id for the new instance.

Next week I might be able to start looking into the libchromiumcontent patch that enables fully reusing chromium cross site navigation logic, and will send another PR after it is ready.

@bridiver

This comment has been minimized.

Show comment
Hide comment
@bridiver

bridiver Sep 21, 2016

Contributor

might be better to split into 2 PRs to separate window.opener from sandbox. There are a lot of subtle issues with window.opener on different platforms and with different methods of opening and most of them are on windows. We've had opener support for a while and have tweaked it several times to deal with crashes resulting from different types of opener triggers. You've taken a different approach which will leave fixing most of those issues to the app, but I don't think it will be possible to handle them all unless you return false from ShouldResumeRequestsForCreatedWindow in atom_api_web_contents.cc' and this ugly hack inatom_api_web_frame.cc`

void WebFrame::AttachGuest(int id) {
  // This is a workaround for a strange issue on windows with background tabs
  // libchromiumcontent doesn't appear to be making the check for
  // params.disposition == NEW_BACKGROUND_TAB in WebContentsImpl
  // This results in the BrowserPluginGuest trying to access the native
  // window before it's actually ready.
  //
  // It's also possible that the guest is being treated as
  // visible because the "embedder", which is the same for all tabs
  // in the window, is always visible.
  //
  // This hack works around the issue by always
  // marking it as hidden while attaching
  content::BrowserPluginManager::Get()->GetBrowserPlugin(id)->
    updateVisibility(false);
  content::RenderFrame::FromWebFrame(web_frame_)->AttachGuest(id);
  content::BrowserPluginManager::Get()->GetBrowserPlugin(id)->
    updateVisibility(true);
}

The second is specific to the use of guest webviews vs the main window, but the first one covers both. You will also find a memory leak with spellcheck provider that only appears when the renderer process persists across page loads and/or contains more than one webcontents. There are some other issues we have found as well, but those are the ones that come to mind.

Contributor

bridiver commented Sep 21, 2016

might be better to split into 2 PRs to separate window.opener from sandbox. There are a lot of subtle issues with window.opener on different platforms and with different methods of opening and most of them are on windows. We've had opener support for a while and have tweaked it several times to deal with crashes resulting from different types of opener triggers. You've taken a different approach which will leave fixing most of those issues to the app, but I don't think it will be possible to handle them all unless you return false from ShouldResumeRequestsForCreatedWindow in atom_api_web_contents.cc' and this ugly hack inatom_api_web_frame.cc`

void WebFrame::AttachGuest(int id) {
  // This is a workaround for a strange issue on windows with background tabs
  // libchromiumcontent doesn't appear to be making the check for
  // params.disposition == NEW_BACKGROUND_TAB in WebContentsImpl
  // This results in the BrowserPluginGuest trying to access the native
  // window before it's actually ready.
  //
  // It's also possible that the guest is being treated as
  // visible because the "embedder", which is the same for all tabs
  // in the window, is always visible.
  //
  // This hack works around the issue by always
  // marking it as hidden while attaching
  content::BrowserPluginManager::Get()->GetBrowserPlugin(id)->
    updateVisibility(false);
  content::RenderFrame::FromWebFrame(web_frame_)->AttachGuest(id);
  content::BrowserPluginManager::Get()->GetBrowserPlugin(id)->
    updateVisibility(true);
}

The second is specific to the use of guest webviews vs the main window, but the first one covers both. You will also find a memory leak with spellcheck provider that only appears when the renderer process persists across page loads and/or contains more than one webcontents. There are some other issues we have found as well, but those are the ones that come to mind.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 21, 2016

Contributor

@bridiver is it possible that these issues you have experienced are caused by node.js running in the renderer? It would be great if you could provide a couple of test cases that reproduce these crashes

Contributor

tarruda commented Sep 21, 2016

@bridiver is it possible that these issues you have experienced are caused by node.js running in the renderer? It would be great if you could provide a couple of test cases that reproduce these crashes

// Disable renderer sandbox for most of node's functions.
command_line->AppendSwitch(switches::kNoSandbox);
if (command_line->HasSwitch(switches::kEnableSandbox)) {
// Disable setuid sandbox since it is not longer required on linux(namespace

This comment has been minimized.

@bridiver

bridiver Sep 21, 2016

Contributor

what happens if this runs on a distro that doesn't support the namespace sandbox? Does it fail or just disable the sandbox?

@bridiver

bridiver Sep 21, 2016

Contributor

what happens if this runs on a distro that doesn't support the namespace sandbox? Does it fail or just disable the sandbox?

This comment has been minimized.

@tarruda

tarruda Sep 26, 2016

Contributor

Just disable

@tarruda

tarruda Sep 26, 2016

Contributor

Just disable

@bridiver

This comment has been minimized.

Show comment
Hide comment
@bridiver

bridiver Sep 21, 2016

Contributor

@tarruda we disabled node on sandboxed renderer processes. I do really need to add a set of test cases to cover all the different ways that the opener can be used, I'll see if I can at least come up with a list of examples and go from there. You can take a look at our fork as well https://github.com/brave/electron I think we can both learn something from the two different implementations. Our opener support is actually limited to guest webviews because that was all we needed, but it could easily be extended to the main window.

Contributor

bridiver commented Sep 21, 2016

@tarruda we disabled node on sandboxed renderer processes. I do really need to add a set of test cases to cover all the different ways that the opener can be used, I'll see if I can at least come up with a list of examples and go from there. You can take a look at our fork as well https://github.com/brave/electron I think we can both learn something from the two different implementations. Our opener support is actually limited to guest webviews because that was all we needed, but it could easily be extended to the main window.

@bridiver

This comment has been minimized.

Show comment
Hide comment
@bridiver

bridiver Sep 21, 2016

Contributor

I'm trying to remember the exact problem with the spell check client, but I think the issue was that the webFrame never got destroyed and so the spellcheck client also never got destroyed. I never came up with a good fix in atom_api_web_frame and we handled the problem by moving spellcheck to web_frame_bindings which uses extensions, but obviously that's not available to you here

Contributor

bridiver commented Sep 21, 2016

I'm trying to remember the exact problem with the spell check client, but I think the issue was that the webFrame never got destroyed and so the spellcheck client also never got destroyed. I never came up with a good fix in atom_api_web_frame and we handled the problem by moving spellcheck to web_frame_bindings which uses extensions, but obviously that's not available to you here

@bridiver

This comment has been minimized.

Show comment
Hide comment
@bridiver

bridiver Sep 21, 2016

Contributor

I shouldn't say that webFrame never got destroyed. It (they in this case) did get properly destroyed when the renderer process shut down, but within the life of a renderer process a new one was created for each navigation without destroying the previous one.

Contributor

bridiver commented Sep 21, 2016

I shouldn't say that webFrame never got destroyed. It (they in this case) did get properly destroyed when the renderer process shut down, but within the life of a renderer process a new one was created for each navigation without destroying the previous one.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 26, 2016

Contributor

@zcbenz The branch is rebased but I still need to squash the fixup commits, which I will do after you approve the PR. I've updated #6712 to keep track of the remaining tasks the will be done in future PRs.

Contributor

tarruda commented Sep 26, 2016

@zcbenz The branch is rebased but I still need to squash the fixup commits, which I will do after you approve the PR. I've updated #6712 to keep track of the remaining tasks the will be done in future PRs.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 26, 2016

Contributor

@bridiver I have looked into brave/electron master branch, but the diff from upstream electron is quite big, so I can only comment on some parts which relate to what I've changed in this PR.

I've noticed that you subclassed AtomBrowserClient to provide a custom implementation to CanCreateWindow, which is the main gateway to native window.open API. I couldn't use the same approach here because electron must still support its custom window.open implementation which returns the BrowserWindowProxy(In other words, it must support both sandboxed and non-sandboxed renderers in the same browser)

The best solution I found was use the sandbox option as a flag to determine when CanCreateWindow should fork into native window.open behavior. This introduced the problem of determining when the CanCreateWindow call comes from a renderer which has sandbox enabled, which is why I used a set containing all ids of sandboxed renderers.

Another problem is that the content API doesn't seem to provide a way to map AppendExtraCommandLineParameters calls with the renderer which originated the navigation, so I also had to duplicate some chromium logic for determining cross-site navigations in ShouldCreateNewSiteInstance.

It should be noted that in the current state, sandboxed windows are very limited because I've created a new subclass from ContentRendererClient, but I plan to fix this in another PR.

I also looked into CEF code and noticed that it doesn't override ShouldResumeRequestsForCreatedWindow, even though it has native window.open fully working in all platforms, but maybe this is because CEF takes a different approach to creating native windows.

If I override ShouldResumeRequestsForCreatedWindow, how do I unblock the renderer after the window is created? Can you point me to which parts of the brave code I can extract information regarding this issue?

Contributor

tarruda commented Sep 26, 2016

@bridiver I have looked into brave/electron master branch, but the diff from upstream electron is quite big, so I can only comment on some parts which relate to what I've changed in this PR.

I've noticed that you subclassed AtomBrowserClient to provide a custom implementation to CanCreateWindow, which is the main gateway to native window.open API. I couldn't use the same approach here because electron must still support its custom window.open implementation which returns the BrowserWindowProxy(In other words, it must support both sandboxed and non-sandboxed renderers in the same browser)

The best solution I found was use the sandbox option as a flag to determine when CanCreateWindow should fork into native window.open behavior. This introduced the problem of determining when the CanCreateWindow call comes from a renderer which has sandbox enabled, which is why I used a set containing all ids of sandboxed renderers.

Another problem is that the content API doesn't seem to provide a way to map AppendExtraCommandLineParameters calls with the renderer which originated the navigation, so I also had to duplicate some chromium logic for determining cross-site navigations in ShouldCreateNewSiteInstance.

It should be noted that in the current state, sandboxed windows are very limited because I've created a new subclass from ContentRendererClient, but I plan to fix this in another PR.

I also looked into CEF code and noticed that it doesn't override ShouldResumeRequestsForCreatedWindow, even though it has native window.open fully working in all platforms, but maybe this is because CEF takes a different approach to creating native windows.

If I override ShouldResumeRequestsForCreatedWindow, how do I unblock the renderer after the window is created? Can you point me to which parts of the brave code I can extract information regarding this issue?

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 26, 2016

Contributor

Regarding @paulcbetts request for <webview> tag, the following patch enables the sandbox option:

diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js
index df2589a..267fdeb 100644
--- a/lib/browser/guest-view-manager.js
+++ b/lib/browser/guest-view-manager.js
@@ -177,6 +177,7 @@ const attachGuest = function (embedder, elementInstanceId, guestInstanceId, para
   webPreferences = {
     guestInstanceId: guestInstanceId,
     nodeIntegration: (ref1 = params.nodeintegration) != null ? ref1 : false,
+    sandbox: !!params.sandbox,
     plugins: params.plugins,
     zoomFactor: params.zoomFactor,
     webSecurity: !params.disablewebsecurity,
diff --git a/lib/renderer/web-view/web-view-attributes.js b/lib/renderer/web-view/web-view-attributes.js
index 3bbc833..3234135 100644
--- a/lib/renderer/web-view/web-view-attributes.js
+++ b/lib/renderer/web-view/web-view-attributes.js
@@ -321,6 +321,7 @@ WebViewImpl.prototype.setupWebViewAttributes = function () {
   this.attributes[webViewConstants.ATTRIBUTE_HTTPREFERRER] = new HttpReferrerAttribute(this)
   this.attributes[webViewConstants.ATTRIBUTE_USERAGENT] = new UserAgentAttribute(this)
   this.attributes[webViewConstants.ATTRIBUTE_NODEINTEGRATION] = new BooleanAttribute(webViewConstants.ATTRIBUTE_NODEINTEGRATION, this)
+  this.attributes[webViewConstants.ATTRIBUTE_SANDBOX] = new BooleanAttribute(webViewConstants.ATTRIBUTE_SANDBOX, this)
   this.attributes[webViewConstants.ATTRIBUTE_PLUGINS] = new BooleanAttribute(webViewConstants.ATTRIBUTE_PLUGINS, this)
   this.attributes[webViewConstants.ATTRIBUTE_DISABLEWEBSECURITY] = new BooleanAttribute(webViewConstants.ATTRIBUTE_DISABLEWEBSECURITY, this)
   this.attributes[webViewConstants.ATTRIBUTE_ALLOWPOPUPS] = new BooleanAttribute(webViewConstants.ATTRIBUTE_ALLOWPOPUPS, this)
diff --git a/lib/renderer/web-view/web-view-constants.js b/lib/renderer/web-view/web-view-constants.js
index 5aed6d5..31b0112 100644
--- a/lib/renderer/web-view/web-view-constants.js
+++ b/lib/renderer/web-view/web-view-constants.js
@@ -10,6 +10,7 @@ module.exports = {
   ATTRIBUTE_SRC: 'src',
   ATTRIBUTE_HTTPREFERRER: 'httpreferrer',
   ATTRIBUTE_NODEINTEGRATION: 'nodeintegration',
+  ATTRIBUTE_SANDBOX: 'sandbox',
   ATTRIBUTE_PLUGINS: 'plugins',
   ATTRIBUTE_DISABLEWEBSECURITY: 'disablewebsecurity',
   ATTRIBUTE_ALLOWPOPUPS: 'allowpopups',

Which works but causes a crash when javascript running in the <webview> invokes window.open. The crash happens inside libcontent(content::WebContentsImpl::CreateNewWindow). I wasn't able to determine the cause of crash because there are no debug symbols in the prebuilt libcontent, so I will leave <webview> out for now.

Contributor

tarruda commented Sep 26, 2016

Regarding @paulcbetts request for <webview> tag, the following patch enables the sandbox option:

diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js
index df2589a..267fdeb 100644
--- a/lib/browser/guest-view-manager.js
+++ b/lib/browser/guest-view-manager.js
@@ -177,6 +177,7 @@ const attachGuest = function (embedder, elementInstanceId, guestInstanceId, para
   webPreferences = {
     guestInstanceId: guestInstanceId,
     nodeIntegration: (ref1 = params.nodeintegration) != null ? ref1 : false,
+    sandbox: !!params.sandbox,
     plugins: params.plugins,
     zoomFactor: params.zoomFactor,
     webSecurity: !params.disablewebsecurity,
diff --git a/lib/renderer/web-view/web-view-attributes.js b/lib/renderer/web-view/web-view-attributes.js
index 3bbc833..3234135 100644
--- a/lib/renderer/web-view/web-view-attributes.js
+++ b/lib/renderer/web-view/web-view-attributes.js
@@ -321,6 +321,7 @@ WebViewImpl.prototype.setupWebViewAttributes = function () {
   this.attributes[webViewConstants.ATTRIBUTE_HTTPREFERRER] = new HttpReferrerAttribute(this)
   this.attributes[webViewConstants.ATTRIBUTE_USERAGENT] = new UserAgentAttribute(this)
   this.attributes[webViewConstants.ATTRIBUTE_NODEINTEGRATION] = new BooleanAttribute(webViewConstants.ATTRIBUTE_NODEINTEGRATION, this)
+  this.attributes[webViewConstants.ATTRIBUTE_SANDBOX] = new BooleanAttribute(webViewConstants.ATTRIBUTE_SANDBOX, this)
   this.attributes[webViewConstants.ATTRIBUTE_PLUGINS] = new BooleanAttribute(webViewConstants.ATTRIBUTE_PLUGINS, this)
   this.attributes[webViewConstants.ATTRIBUTE_DISABLEWEBSECURITY] = new BooleanAttribute(webViewConstants.ATTRIBUTE_DISABLEWEBSECURITY, this)
   this.attributes[webViewConstants.ATTRIBUTE_ALLOWPOPUPS] = new BooleanAttribute(webViewConstants.ATTRIBUTE_ALLOWPOPUPS, this)
diff --git a/lib/renderer/web-view/web-view-constants.js b/lib/renderer/web-view/web-view-constants.js
index 5aed6d5..31b0112 100644
--- a/lib/renderer/web-view/web-view-constants.js
+++ b/lib/renderer/web-view/web-view-constants.js
@@ -10,6 +10,7 @@ module.exports = {
   ATTRIBUTE_SRC: 'src',
   ATTRIBUTE_HTTPREFERRER: 'httpreferrer',
   ATTRIBUTE_NODEINTEGRATION: 'nodeintegration',
+  ATTRIBUTE_SANDBOX: 'sandbox',
   ATTRIBUTE_PLUGINS: 'plugins',
   ATTRIBUTE_DISABLEWEBSECURITY: 'disablewebsecurity',
   ATTRIBUTE_ALLOWPOPUPS: 'allowpopups',

Which works but causes a crash when javascript running in the <webview> invokes window.open. The crash happens inside libcontent(content::WebContentsImpl::CreateNewWindow). I wasn't able to determine the cause of crash because there are no debug symbols in the prebuilt libcontent, so I will leave <webview> out for now.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Sep 26, 2016

Contributor

Squashed commits and rebased on master. The original unsquashed branch was copied https://github.com/tarruda/electron/tree/support-chromium-sandbox-save2.

Contributor

tarruda commented Sep 26, 2016

Squashed commits and rebased on master. The original unsquashed branch was copied https://github.com/tarruda/electron/tree/support-chromium-sandbox-save2.

tarruda added some commits Aug 15, 2016

Add "sandboxed" option to "webPreferences".
When "sandboxed" is passed as a web preference for `BrowserWindow`, the newly
created renderer won't run any node.js code/integration, only communicating with
the system via the IPC API of the content module. This is a requirement for
running the renderer under chrome OS-level sandbox.

Beyond that, certain behaviors of AtomBrowserClient are modified when dealing
with sandboxed renderers:

- `OverrideSiteInstanceNavigation` no longer create a new `SiteInstance` for
  every navigation. Instead, it reuses the source `SiteInstance` when not
  navigating to a different site.
- `CanCreateWindow` will return true and allow javascript access.
Expose `--enable-sandbox` command-line switch.
When `--enable-sandbox` is passed, electron will use chromium sandbox to spawn
all renderers, and every new BrowserWindow will automatically have "sandboxed"
passed as a web preference(since the renderer would not work properly
otherwise).
Add support for native chromium popups on sandboxed renderers.
- Allow `api::Window` instances to be created from existing `api::WebContents`.
- Override `WebContentsCreated` and `AddNewContents` to wrap renderer-created
  `content::WebContents` into `api::WebContents`.
- For `content::WebContents` that should be displayed in new windows, pass the
  wrapped `api::WebContents` object to window manager.
Allow api::WebContents to fully wrap an existing content::WebContents.
- Add an overload to `WebContents::CreateFrom` that accepts a type parameter. If
  type is `REMOTE`, initialization is the same as before(a thin wrapper). If
  not, the `api::WebContents` will be fully initialized, as if it was created by
  `api::WebContents::Create`.
- Move common initialization code to `InitWithSessionAndOptions`.
Move EmitIPCEvent into AtomRenderViewObserver.
Refactor this function as a method so it is possible to inherit most behavior
from AtomRenderViewObserver and override EmitIPCEvent.
Refactor the atom_js2c target to include javascript from multiple dirs.
Before invoking js2c, copy all files that must be embedded into the shared
intermediate directory, and modify the js2c wrapper script to include all files
from that directory(which is passed as argument).

This allows the build system to embed files that don't share a common base
directory, such as javascript generated at build time.

tarruda added some commits Aug 20, 2016

Embed setup bundle for preload scripts in sandboxed renderers.
Add a gyp target that creates a browserify bundle starting with
`lib/sandboxed_renderer/init.js`, which is embedded into the executable using
the `atom_js2c` target.

The goal of this bundle is to provide a very basic environment for preload
scripts where a `require` function is available.
Improve AtomSandboxedRendererClient to support preload scripts.
Add RenderFrameObserver/RenderViewObserver subclasses that implement the
necessary support for preload scripts in sandboxed renderers.
Use the routing id on api::WebContents::GetID
The sandbox option allows multiple webContents in one renderer process, so using
the only the renderer id to identify WebContents instances is no longer an
option.

WebContents::GetID now returns a 64-bit integer, which is composed of both the
process id(high 32), and the RenderViewHost routing id(low 32). Also add a
`GetProcessID` that retrieves the renderer process id, a requirement in some of
our javascript code.
@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Sep 27, 2016

Contributor

👍

Contributor

zcbenz commented Sep 27, 2016

👍

@zcbenz zcbenz merged commit 47fd417 into electron:master Sep 27, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Sep 30, 2016

Contributor

@tarruda this seems interesting, are you going to add some docs to explain the option and what it gived to developers. There are some information in this thread, but it is not easy to find.

Contributor

YurySolovyov commented Sep 30, 2016

@tarruda this seems interesting, are you going to add some docs to explain the option and what it gived to developers. There are some information in this thread, but it is not easy to find.

@ahmedmohamedali

This comment has been minimized.

Show comment
Hide comment
@ahmedmohamedali

ahmedmohamedali Oct 20, 2016

Contributor

Hi @tarruda,

Understood ipcRenderer is exposed in the sandboxed renderer. Is there a plan to extend the list of exposed modules? for example websocket ?

Thanks.

Contributor

ahmedmohamedali commented Oct 20, 2016

Hi @tarruda,

Understood ipcRenderer is exposed in the sandboxed renderer. Is there a plan to extend the list of exposed modules? for example websocket ?

Thanks.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Oct 20, 2016

Contributor

@tarruda this seems interesting, are you going to add some docs to explain the option and what it gived to developers. There are some information in this thread, but it is not easy to find.

I will add documentation later

Understood ipcRenderer is exposed in the sandboxed renderer. Is there a plan to extend the list of exposed modules? for example websocket ?

It is possible that we'll eventually expose every module available to non-sandboxed renderers, but you can already achieve anything with ipcRenderer alone. From a security POV, it is better that only ipcRenderer is exposed.

Contributor

tarruda commented Oct 20, 2016

@tarruda this seems interesting, are you going to add some docs to explain the option and what it gived to developers. There are some information in this thread, but it is not easy to find.

I will add documentation later

Understood ipcRenderer is exposed in the sandboxed renderer. Is there a plan to extend the list of exposed modules? for example websocket ?

It is possible that we'll eventually expose every module available to non-sandboxed renderers, but you can already achieve anything with ipcRenderer alone. From a security POV, it is better that only ipcRenderer is exposed.

@ahmedmohamedali

This comment has been minimized.

Show comment
Hide comment
@ahmedmohamedali

ahmedmohamedali Oct 20, 2016

Contributor

@tarruda I agree for the security constraints and this the reason I'm interested in your contribution. However I also need to establish a communication with a standalone node.js process (non electron process). Websocket seems an easy option for a electron renderer to communicate with the node.js process. Is it possible to require the websocket module in the preload in js or this can only be achievable by modifying electron code ? Thanks

Contributor

ahmedmohamedali commented Oct 20, 2016

@tarruda I agree for the security constraints and this the reason I'm interested in your contribution. However I also need to establish a communication with a standalone node.js process (non electron process). Websocket seems an easy option for a electron renderer to communicate with the node.js process. Is it possible to require the websocket module in the preload in js or this can only be achievable by modifying electron code ? Thanks

@ahmedmohamedali

This comment has been minimized.

Show comment
Hide comment
@ahmedmohamedali

ahmedmohamedali Oct 21, 2016

Contributor

I think the list of modules to expose should be passed with a new parameter such as sandboxAllowedModules. The default electron behavior will not be impacted and the change will give an opportunity for those who needs to support some more modules. Not sure if there is way to raise a change request here or if I need to create a PR. Thanks.

Contributor

ahmedmohamedali commented Oct 21, 2016

I think the list of modules to expose should be passed with a new parameter such as sandboxAllowedModules. The default electron behavior will not be impacted and the change will give an opportunity for those who needs to support some more modules. Not sure if there is way to raise a change request here or if I need to create a PR. Thanks.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Oct 22, 2016

Contributor

However I also need to establish a communication with a standalone node.js process (non electron process). Websocket seems an easy option for a electron renderer to communicate with the node.js process. Is it possible to require the websocket module in the preload in js or this can only be achievable by modifying electron code ? Thanks

I'm not an expert in the chrome sandbox architecture, but if I understood it correctly, the renderer is only allowed to communicate with the browser process, so even if we allow arbitrary node/electron modules to be required from the renderer, the entire API would be proxied using IPC(it would simply be hidden from you). Even network access is done via the browser process, so the renderer is never opening any network connections directly.

If you simply want to use WebSocket web api, then it should already be available to sandboxed renderers, there's no need to modify anything.

Contributor

tarruda commented Oct 22, 2016

However I also need to establish a communication with a standalone node.js process (non electron process). Websocket seems an easy option for a electron renderer to communicate with the node.js process. Is it possible to require the websocket module in the preload in js or this can only be achievable by modifying electron code ? Thanks

I'm not an expert in the chrome sandbox architecture, but if I understood it correctly, the renderer is only allowed to communicate with the browser process, so even if we allow arbitrary node/electron modules to be required from the renderer, the entire API would be proxied using IPC(it would simply be hidden from you). Even network access is done via the browser process, so the renderer is never opening any network connections directly.

If you simply want to use WebSocket web api, then it should already be available to sandboxed renderers, there's no need to modify anything.

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Oct 22, 2016

Contributor

Not sure if there is way to raise a change request here or if I need to create a PR. Thanks.

No need to create an issue a, simply use #6712 to track progress. I plan to implement it as soon as I get a free weekend, work/family have been demanding a lot lately :)

Contributor

tarruda commented Oct 22, 2016

Not sure if there is way to raise a change request here or if I need to create a PR. Thanks.

No need to create an issue a, simply use #6712 to track progress. I plan to implement it as soon as I get a free weekend, work/family have been demanding a lot lately :)

@ahmedmohamedali

This comment has been minimized.

Show comment
Hide comment
@ahmedmohamedali

ahmedmohamedali Oct 22, 2016

Contributor

That's fine, many thanks.
Exposing ipcRenderer by default in sandboxed mode will still be unsafe as security hole in the main process can be exploited to harm the user machine.
So,I think ipcRenderer should be disabled by default and it will be the responsibility of the developer to select it in the allowed modules if he wills. Another option is to expose a new safeIpcRender module that will only process user defined messages.

Contributor

ahmedmohamedali commented Oct 22, 2016

That's fine, many thanks.
Exposing ipcRenderer by default in sandboxed mode will still be unsafe as security hole in the main process can be exploited to harm the user machine.
So,I think ipcRenderer should be disabled by default and it will be the responsibility of the developer to select it in the allowed modules if he wills. Another option is to expose a new safeIpcRender module that will only process user defined messages.

@GregoryGatellier

This comment has been minimized.

Show comment
Hide comment
@GregoryGatellier

GregoryGatellier Oct 24, 2016

@tarruda I would like your opinion about having a default preload used when we are in sandbox mode (and in that case no need to have preload attribute in webpreferences).
We are very interested by using the sandbox but having a preload script for each application is not so easy to maintain (depending if the application need specific code in the preload)

@tarruda I would like your opinion about having a default preload used when we are in sandbox mode (and in that case no need to have preload attribute in webpreferences).
We are very interested by using the sandbox but having a preload script for each application is not so easy to maintain (depending if the application need specific code in the preload)

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Nov 2, 2016

Contributor

@tarruda I would like your opinion about having a default preload used when we are in sandbox mode (and in that case no need to have preload attribute in webpreferences).
We are very interested by using the sandbox but having a preload script for each application is not so easy to maintain (depending if the application need specific code in the preload)

Not sure if I understand what you mean by "default preload". Can you give an example?

If you want to have a single preload API that exports all electron/node.js APIs to the renderer, then the sandbox won't help much(assuming you are using it for security purposes).

Contributor

tarruda commented Nov 2, 2016

@tarruda I would like your opinion about having a default preload used when we are in sandbox mode (and in that case no need to have preload attribute in webpreferences).
We are very interested by using the sandbox but having a preload script for each application is not so easy to maintain (depending if the application need specific code in the preload)

Not sure if I understand what you mean by "default preload". Can you give an example?

If you want to have a single preload API that exports all electron/node.js APIs to the renderer, then the sandbox won't help much(assuming you are using it for security purposes).

@GregoryGatellier

This comment has been minimized.

Show comment
Hide comment
@GregoryGatellier

GregoryGatellier Nov 3, 2016

Today with your solution all the developers who want to use sandbox have to manage on their side a preload for initiliazing some code (if they want to use it) for each application and depending if they want to activate or not the sandbox.
My main concern is for having window.open works in sandbox mode, we need some specific code in the preload and it's not very helpful to adding manually the code every time we need to use sandbox (it should be automatic).

Today with your solution all the developers who want to use sandbox have to manage on their side a preload for initiliazing some code (if they want to use it) for each application and depending if they want to activate or not the sandbox.
My main concern is for having window.open works in sandbox mode, we need some specific code in the preload and it's not very helpful to adding manually the code every time we need to use sandbox (it should be automatic).

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Nov 3, 2016

Contributor

There's no need to have a preload, you can simply use new BrowserWindow({webPreferences: {sandbox: true}}) and it will create a sandboxed window. This should work for loading most web applications and offer the same security as chrome. window.open will also work without a preload.

Contributor

tarruda commented Nov 3, 2016

There's no need to have a preload, you can simply use new BrowserWindow({webPreferences: {sandbox: true}}) and it will create a sandboxed window. This should work for loading most web applications and offer the same security as chrome. window.open will also work without a preload.

@ahmedmohamedali

This comment has been minimized.

Show comment
Hide comment
@ahmedmohamedali

ahmedmohamedali Nov 3, 2016

Contributor

@tarruda,

I think security whole in the master process can be exploited via IpcRenderer. If I need a pre-load file to expose ipcRenderer this is fine, I will just avoid to expose it. But if it is automatically exposed what is the best way to disable it before starting the renderer process ?
Thanks.

Contributor

ahmedmohamedali commented Nov 3, 2016

@tarruda,

I think security whole in the master process can be exploited via IpcRenderer. If I need a pre-load file to expose ipcRenderer this is fine, I will just avoid to expose it. But if it is automatically exposed what is the best way to disable it before starting the renderer process ?
Thanks.

@GregoryGatellier

This comment has been minimized.

Show comment
Hide comment
@GregoryGatellier

GregoryGatellier Nov 3, 2016

Yes but for having window.open works correctly in sandbox mode I need to have some code to be loaded. That's why I ask the question (if some was interesting to have a fix for window.open).

GregoryGatellier commented Nov 3, 2016

Yes but for having window.open works correctly in sandbox mode I need to have some code to be loaded. That's why I ask the question (if some was interesting to have a fix for window.open).

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Nov 3, 2016

Contributor

But if it is automatically exposed what is the best way to disable it before starting the renderer process ?

It is not automatically exposed. Since one of the main use cases of sandbox mode is enabling chromium sandbox for security, you have to explicitly create any extra APIs that use ipcRenderer to communicate with the main process.

Contributor

tarruda commented Nov 3, 2016

But if it is automatically exposed what is the best way to disable it before starting the renderer process ?

It is not automatically exposed. Since one of the main use cases of sandbox mode is enabling chromium sandbox for security, you have to explicitly create any extra APIs that use ipcRenderer to communicate with the main process.

@ahmedmohamedali

This comment has been minimized.

Show comment
Hide comment
@ahmedmohamedali

ahmedmohamedali Nov 3, 2016

Contributor

I already got that. I need that ipcRenderer not even be available in the Renderer process.
How to achieve that ?

Contributor

ahmedmohamedali commented Nov 3, 2016

I already got that. I need that ipcRenderer not even be available in the Renderer process.
How to achieve that ?

@GregoryGatellier

This comment has been minimized.

Show comment
Hide comment
@GregoryGatellier

GregoryGatellier Nov 3, 2016

In order to give more details: 'default preload' means for me specific code in init.js for renderer sandboxed

In order to give more details: 'default preload' means for me specific code in init.js for renderer sandboxed

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Nov 3, 2016

Contributor

I already got that. I need that ipcRenderer not even be available in the Renderer process.
How to achieve that ?

ipcRenderer is only available if your preload script exposes it as a global variable. The preload script is executed in the scope of an anonymous function before any javascript from the web page is loaded. The only way ipcRenderer is available to javascript loaded by web page is if you do something like this:

// preload.js
window.ipcRenderer = require('electron').ipcRenderer
// ipcRenderer is a global variable visible to untrusted javascript.
Contributor

tarruda commented Nov 3, 2016

I already got that. I need that ipcRenderer not even be available in the Renderer process.
How to achieve that ?

ipcRenderer is only available if your preload script exposes it as a global variable. The preload script is executed in the scope of an anonymous function before any javascript from the web page is loaded. The only way ipcRenderer is available to javascript loaded by web page is if you do something like this:

// preload.js
window.ipcRenderer = require('electron').ipcRenderer
// ipcRenderer is a global variable visible to untrusted javascript.
@ahmedmohamedali

This comment has been minimized.

Show comment
Hide comment
@ahmedmohamedali

ahmedmohamedali Nov 3, 2016

Contributor

Great. This is what I wanted to confirm with you.
Thank you.

Contributor

ahmedmohamedali commented Nov 3, 2016

Great. This is what I wanted to confirm with you.
Thank you.

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