reZach / secure-electron-template

The best way to build Electron apps with security in mind.
MIT License
1.64k stars 154 forks source link

Discussion about security implications for offline-only apps or apps that only load secure content #110

Closed petef19 closed 2 years ago

petef19 commented 2 years ago

@reZach

as per my original post, and you were nice enough to offer insights, I would love to get clarification and your thoughts on the following.

Since Electron currently does NOT implement any security mechanism to protect the src code, it would be helpful for devs to get an explanation what security implications they are facing when changing certain recommended Electron settings.

Types of apps would include: (1) online only app (e.g. Slack) (2) app that connects to remote sources, but NOT to 3rd party sources, hence the app ONLY loads verified/validated content (3) offline only app (only loads local content)

Specifically, why is the contextIsolation setting "helpful" and/or "needed" for apps (2) and (3) ?

Thanks !

petef19 commented 2 years ago

bump 😀

reZach commented 2 years ago

Hi @petef19, thanks for being patient here. (Pulling some details from my earlier post in case it's helpful for others to reference).

I'm also publishing a larger post on an introduction to electron that you may find helpful to understand some context of security, and how the framework has changed.

nodeIntegration

Turning nodeIntegration to true allows your views/front-end code to access Node. This would mean a front-end view could call var fs = require('fs'); and access the file system as it would need to. The problem with this approach, is that if the front-end code had a XSS vulnerability, you may compromise your system. A lot of electron documentation is written assuming nodeIntegration is set to true, which can lead app developers to adopt insecure practices when writing apps.

I wouldn't ever recommend anyone use nodeIntegration set to true to anyone. It can be set to true if you are in a time crunch or if the code is a POC (proof-of-concept) that you know you'll throw away the code of/rewrite it. It isn't much more complicated to structure an app to rely on nodeIntegration: false, and I would gather most aren't going to scrap their POC electron app code once it's written, so I'd recommend always avoiding setting nodeIntegration: false except in rare cases.

rare cases

The key is that your app is never used by anyone else that I would see using nodeIntegration: true is ok, but still it's a gray line for me.

enableRemoteModule

It's deprecated, so don't use it. It's essentially another way similar to nodeIntegration where a view/front-end page could require what it needs. (It looks like the electron team removed the content on their website about this feature.) Just noting that you may see this in documentation across the web.

You accept the same risks as nodeIntegration when using remote in your electron apps.


contextIsolation

This should always be set to true. This option prevents prototype pollution attacks in your app. (Using contextIsolation assumes you are using a preload file in your electron app.)

By setting contextIsolation to false, you are allowing anyone who is able to get into your app the ability to change method definitions of functions defined on your contextBridge. What this means is that if you have any particularly capable libraries loaded in your preload file (ie. fs), a motivated individual could change the definition of a function into one that leverages a library into a destructive action (ie. removing all files on your computer).

If your preload file doesn't require any library that can interact with your operating system and your main electron file has sufficient checks for IPC messages from your renderer process, then you would likely be okay from the prototype pollution attack vector. (But why not set this to true to have extra protections? 😄).

eg. if your "verified" resource (1st party) gets a DNS takeover, then what you thought was safe, is now returning malicious content, your backend electron code might have a security hole on the fact that you were relying your resources were secure and in control by you. (granted, it's a low-possibility occurrence, but when you are thinking of security, a walled approach (many defenses)) is a better approach. If you are distributing this app to anyone besides yourself, contextIsolation should be set to true.

For offline apps, you'd only have to worry about a trojan or malware affecting your computer in such a way to exploit the prototype pollution but at this point your computer is likely hosed anyway.

Setting contextIsolation should likely not affect community-electron libraries you are using (but there may be exceptions).

sandbox

If you really want to be secure, you should set sandbox to true. This locks your preload file from only sending IPC messages, and you'll have to do all interactions with node libraries/other libraries in your electron backend file. This greatly protects your app, but will very likely cause most external libraries to break as a very high percentage of libraries do not work when sandbox is set to true.

Summary

In general, straying from the recommended values of nodeIntegration (default false), enableRemoteModule (default false), contextIsolation (default true) when building apps only you are using, is okay but not encouraged. You accept additional risk at the benefit of faster (albeit less framework-proper) code.

For any app that you intend to distribute to others, I would strongly recommend not straying for the defaults (and especially work towards setting sandbox: true in your app) as multiple layers of defense (IPC, context isolation, checks/controls in your backend electron file) are better than fewer.


@petef19 did I answer your question or do you need additional detail?

petef19 commented 2 years ago

@reZach thanks for the reply ! I did not get an email notification, but checked here periodically and saw it.

Turning nodeIntegration to false

this is typo, I think you meant "to true" (1st sentence under nodeIntegration section)

By setting contextIsolation to false, you are allowing anyone who is able to get into your app

could you elaborate on specific use case scenarios what you mean by "get into the app" ? does this refer to loading external, 3rd party content or somebody messing w/ the app locally once they downloaded it ?

ctxIso: But why not set this to true to have extra protections? 😄

b/c if you enable ctxIso you can NOT bytenode encode the renderer anymore, as the require fn is not available anymore in the renderer/view... depending on what is in your view code, and there will be some logic or maybe IP in there, this is a compromise... your code is exposed question is for for apps that do NOT load 3rd party content, so either offline only app or apps that only load their own secure content, could this be disabled, or are there any other side effects.... ?

eg. if your "verified" resource (1st party) gets a DNS takeover, then what you thought was safe, is now returning malicious content

that is one use case scenario for sure, question is what chance is higher: (1) DNS takeover happening, or (2) they can hack/modify the app more easily or at least get an idea what the ctxBridge sends/receives b/c the view is now not bytenode encoded... w/ that knowledge they could re-write ctxIso calls etc and then re-distribute the hacked app w/ malware...

IMO, what we've seen since the 80's in app hacking, (2) is way more likely - could be wrong.

In our specific case, we do NOT load any 3rd party content in the view. The view does not communicate w/ outside world at all. The main process (if the user is online) receives json data from our API server, so even if they stage a DNS takeover for that, they could at best send spoofed json data (as the app ONLY processes that), but that API json data won't be able to do anything other than maybe unlock an invalid license request. The json data is fully encrypted and verified when received, so even less likely.

and especially work towards setting sandbox: true

w/ sandbox set to true, you can NOT bytenode encode the preload file anymore (as require fn now won't be available in preload call), w/o this bytenode protection anybody w/ a copy of your app can see the allowed ctxIso bridge calls and start staging an attack w/ that knowledge... remember your view is now also NOT bytenode encoded, hence plain JS, although you should def obfuscate it, but still, that's easily de-obfuscated...

see... no matter how one turns this: this is the problem w/ Electron and the team not recognizing this, you are exposed either way. Their recommendation leaves you exposed, unless your app is online only (which is what they seem to push). Conversations about this have been shut down w/ absolute non-sense bogus arguments.

A built-in encryption protection option like NW.js would solve this particular problem, and add a massive layer of protection, and underneath you can then enable ctxIso and sandbox.

reZach commented 2 years ago

@petef19 Thanks - I've updated that typo.

I will preface this and say that I'm not a pen-tester, nor that I'm a security researcher, I've just tried to compile resources from those that are more knowledgeable than I. I will do my best to reply to your concerns.

ContextIsolation true would more err on the side of users playing around with your app rather than loading external 3rd party content. I found an example of someone using a method that would have been prevented if ContextIsolation were turned on.

Interesting, I have not explored bytecoding encoding the source files of my app with as much time as you appear to have. By require in this context, do you mean requiring local files inherently tied to the front-end of your app or do you mean something else? I feel I vaguely remember reading from somewhere that encoding the source files may introduce issues as you describe. I guess I haven't considered moving to encoding the source files, because if the person has access to the source files, it's pretty much game over already.

I mean, you can of course disable ContextIsolation for offline only apps or apps that load secure content. With it disabled, prototype pollution attacks can happen, but if it's more important to encrypt the source code, then I feel that's a decision you have to weigh for yourself.

Yes, I feel your reply about the DNS takeover shouldn't really be an issue for your app, granted you are encrypting and verifying [in the app].

Gotcha, so I feel you are using or building this app for a business or for other users. In that case the fear of leaking the source code would be more detrimental than protecting against prototype pollution attacks. I think I understand your use case a bit more.

Yeah, I'd be interested to know what you are trying to encode your source files with and why that doesn't keep require working the way it should? Otherwise I'd just like to say for the information that I've gained thus far about the Electron framework, it took some persistence (ie. bugging the team 🙂) to get the information I know today. My past thoughts were that documentation was lacking in the framework, and attempts to ask the best approach on making apps secure were met with an attitude of "don't ask me about this, I'm working on other things." It's probably a combination of a different things that I certainly can't judge, but my best bet for your situation would be to try to come up with a solution with the encoding library you are working with.

petef19 commented 2 years ago

w/ ctxIso or sandbox enabled (scenarios layed out previously), the require function itself is not available anymore. And you need the require function to load the bytenode encoded file(s).

Encoding files for protection or obfuscation is definitely important, to protect IP and/or the user itself (remember: a hacker can easily modify your unprotected app and re-distribute it w/ malware that compromises the user). Unfortunately Electron does not provide any means to protect the src code.

For anybody that release commercial apps, but also anybody who simply does not want to openly share the src code, this is a huge problem. Bytenode encoding is one step. It def creates a first huge barrier. Nothing is impossible to hack o/c, but in protection the name of the game is to create as many barriers as possible that time wise it won't be worth the effort to attempt to hack the app. And protection goes hand in hand w/ security.

So, for folks that do bytenode encode and would like to have the option to encode both the renderer and the main process, sandbox must be disabled, as sandbox removes the require fn everywhere. With ctxIso enabled you lose require fn in renderer but not in main.... hence the question:

is the extra security I gain using ctxIso relevant for me if I release offline-only apps or apps that only load secure 1st party content that is encrypted and then evaluated... ?

reZach commented 2 years ago

Thanks for your explanation, @petef19.

With contextIsolation:true, you can still "require" things your renderer process can use. I'm not sure though if you have tested and seen bytenode causes issues with this setting on - this may be what you are referring to. I am unsure of the internal details of how contextBridge works to pass things through to the renderer process.

is the extra security I gain using ctxIso relevant for me if I release offline-only apps or apps that only load secure 1st party content that is encrypted and then evaluated... ?

In my opinion, I feel malicious users would choose to exploit the fact that contextIsolation is off before trying to decompile your app. But due to the fact that you say your content is encrypted and then evaluated, I think its more secure that your use of bytenode to encrypt the renderer/main process code instead of turning contextIsolation on.

petef19 commented 2 years ago

With contextIsolation:true, you can still "require" things your renderer process can use

the require fn is not available in renderer process w/ ctxIso enabled, i.e. u can NOT use require in index.html, at least as far as I've tested (please report if you find otherwise), but I believe that is one of the main points of ctxIso, to remove that...

In my opinion, I feel malicious users would choose to exploit the fact that contextIsolation is off before trying to decompile your app. But due to the fact that you say your content is encrypted and then evaluated, I think its more secure that your use of bytenode to encrypt the renderer/main process code instead of turning contextIsolation on.

this is the same conclusion we came to (for our specific use case). But in the other thread, and another dedicated one before that, it was all dismissed and it was hammered over and over that you need ctxIso and sandbox which completely exposes the dev's code - zero protection. I was not debating the viability of these options, but rather was looking for elaboration/input for other use cases (such as ours), but all of this was dismissed w/ bogus, non-sense arguments.

ok, thanks for taking the time and chiming in, much appreciated. Always good to have another mind think through a specific use case.

And if anybody else wants to chime in, please do ! Def open to hear anybody else solution to how you protect your code in Electron !

Thanks !

reZach commented 2 years ago

@petef19 can you help me understand this?

the require fn is not available in renderer process w/ ctxIso enabled, i.e. u can NOT use require in index.html, at least as far as I've tested (please report if you find otherwise), but I believe that is one of the main points of ctxIso, to remove that...

Isn't it the case that nodeIntegration: false is what removes require from the renderer process? ContextIsolation from my knowledge is only concerned with your preload file (https://www.electronjs.org/docs/latest/tutorial/context-isolation/).

But otherwise yes, I agree that it doesn't seem important/high enough priority right now for the Electron team to tackle the fact that anyone can simply decompile your app and view the source code.

reZach commented 2 years ago

Closing for now, @petef19 feel free to get in touch if you want to re-open this conversation!

michael-ts commented 2 years ago

@reZach How about the case where you are writing a front end for a nodejs script that is normally run from the command line? In this case, it seems pointless to add any security measures as the whole point of the script was to directly access nodejs functionality, and the whole point of using Electron would be to create a nicer interface to the existing functionality. It seems like having to create a whole API would be a lot of extra unnecessary work as opposed to just dropping the existing code in. Am I missing something here?

reZach commented 2 years ago

@reZach How about the case where you are writing a front end for a nodejs script that is normally run from the command line? In this case, it seems pointless to add any security measures as the whole point of the script was to directly access nodejs functionality, and the whole point of using Electron would be to create a nicer interface to the existing functionality. It seems like having to create a whole API would be a lot of extra unnecessary work as opposed to just dropping the existing code in. Am I missing something here?

@michael-ts I would still recommend executing the script on the backend of the Electron app, if nothing else as it allows for extensibility if you decide to distribute it to others in the future, besides adhering to the frameworks current best-practices. However if what I'm reading between the lines here is that it's a tool for yourself as a QoL improvement, you would probably be fine dropping your script in an Electron app.