-
Notifications
You must be signed in to change notification settings - Fork 94
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: use contextBridge in favour of requiring nodeIntegration #300
Conversation
Due to security concerns related to usage of nodeIntegration flag, according to best electron practices, renderer functions should be exposed with contextBridge. This PR does exactly that. It also changes a bit API to accomodate for this feature
} | ||
|
||
try { | ||
contextBridge.exposeInMainWorld('__ElectronReduxBridge', bridge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do any of the functions exposed in the bridge allow a user to potentially gain access to the current store state, or any of the underlying BrowserWindow
objects? Like I think fetchInitialState
would be dangerous to expose as a global value. This could be worked around with the sort of solution used for preventDoubleInitialization
but then that creates more problems if a user wants to reload the Redux environment without starting.
I think also exposing subscribeToIPCAction
could be dangerous since it would let a malicious user pass any callback they wish to that function and read the data.
If I'm not mistaken about the dangers of exposing those functions, an alternative method would be a bindings approach, but I have to warn I've not actually tried implementing this for any library I've worked with yet.
I got the idea from: https://github.com/reZach/secure-electron-context-menu/blob/master/src/index.js
and then it could be used like this: https://github.com/reZach/secure-electron-context-menu#modify-your-preloadjs-file - although in the case of Electron-Redux I think we wouldn't need to expose anything via contextBridge
except for composeWithStateSync
- which I think is safe to expose?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're just finishing up a release cycle here and starting to think about this again. @matmalkowski any thoughts on a path forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got some internal review already, but I still don't understand the suggested bindings approach and how it would change the security vs this implementation. Would be great if you could provide some example with the difference in behavior we would see @slapbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to discuss @matmalkowski!
The overarching concern with this implementation is the exposure of potentially sensitive or dangerous methods in the global scope.
fetchInitialState
The first thing that stands out to me is exposing fetchInitialState
. Because this would end up mapped somewhere like window.api.fetchInitialState
. If malicious code gained access within the application, exposing the entire state in the global scope gives the attackers free reign to access any and all data, at least the initial values.
Since Redux is supposed to be the "single source of truth," it inherently has a large portion of the application's data. The danger of this is going to depend on what sort of data is stored in the store, of course. Personal data, health data, financial data, etc., would ideally never be accessible from a global scope.
With the bindings approach, no functions to access the store's state would be exposed in a global scope which would make access to the store's state dramatically more difficult for an attacker.
subscribeToIPCAction
There's also probably a risk to having subscribeToIPCAction
in the global scope. This potential attack I'll outline is just spitballing and could maybe be mitigated through other mechanisms, but there's sure to be other similar attacks someone more imaginative and intent on doing harm could find.
A potential attack I could envision is something like: an attacker could potentially register a function to send the action's payload via an xhr
request whenever the IPC channel triggers, which for this particular IPC channel will be for every single Redux action. This would let an attacker access not just the initial state, but also have ongoing access to how the state develops over time.
The fact that the event
arg is not being passed from the IPC channel here is very good to prevent access to Electron internals, but it still leaves application-specific vulnerabilities.
With that in mind, it appears my concern about a BrowserWindow
potentially being exposed is moot.
The current Electron-Redux (master branch and alpha branch) don't allow attacks of either sort, so it seems like introducing access to those functions in the name of improving Electron security would be trading one type of risk for another.
I was very glad to see the Redux store removed from the global state in the main
process on the alpha branch. It was my only security concern in the master branch, since it was trivial to access if an attacked managed to access the remote
functionality and ran remote.getGlobal('getReduxState')()
. The "hard" part of that attack was gaining access to remote
. Putting the ability to access the state in the global scope of the renderer
process makes it easier for an attacker to access the data than for them to need to gain access to the global scope of the main
process.
With the bindings approach I think the only thing that would need to be exposed is composeWithStateSync
which I can't imagine any potential attack vector for at this moment, but again, maybe a lack of imagination. If there is an attack vector involving composeWithStateSync
then maybe there's no way to entirely prevent putting something dangerous in the global scope, but even if that's the case, the bindings approach would reduce the attack surface a lot. Is there anywhere in composeWithStateStync
that an attacker would be able to access the store state or register a malicious action to the IPC listener that I'm overlooking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the writeup - I agree, the security risk created by this approach create actual regression and are not desired. I just didn't understand the bindings approach because I got confused by the idea that binding some functions will make things different from scope perspective. But the issues are basically addressed by making sure only the right functions are exposed, instead of the more raw ones. I took your example and moved to expose in preload script only the 2 required higher level functions as in composeWithStateSync
and stateSyncEnhancer
. I will commit the change in a sec, once I check the app is working as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matmalkowski I think this looks good! I'm sorry for the confusion I've caused by overthinking this/thinking that a minor tweak couldn't achieve the same end. I'm going to blame that on lack of intuition about how it all works together, though that's a pretty weak excuse!
I think that the only remaining point, from my perspective, is to make sure everyone agrees those functions don't expose any potential dangers. I don't see any potential dangers myself. I'm pretty sure that composeWithStateSync
doesn't return anything potentially valuable to an attacker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a security thing, but would supporting a custom value here, in place of __ElectronReduxBridge
, require changes to anything besides the preload function? I did a quick search and didn't see anything else that would need updating to support that.
By the way, thank you for your great work on this issue!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a security thing, but would supporting a custom value here, in place of __ElectronReduxBridge, require changes to anything besides the preload function? I did a quick search and didn't see anything else that would need updating to support that.
I don't know, probably could be done (but not as part of this PR though - what would be use case for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really just a curiosity on my side/a developer experience thing, I shouldn't even have raised the distraction.
I can envision a future where a year from now the window
object has 20 different keys added to it by various projects so I'm just trying to think ahead about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just a small question about the ipcRenderer
🎉 This PR is included in version 2.0.0-alpha.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Due to security concerns related to usage of nodeIntegration flag, according to best electron practices, renderer functions should be exposed with contextBridge. This PR does exactly that. It also changes a bit API to accomodate for this feature