33 ms·

My main takeaway from this article is the question why conextIsolation was introduced defaulting to false. This is one huge lever to help with the maxime “XSS

by pilif 6y ago

My main takeaway from this article is the question why conextIsolation was introduced defaulting to false. This is one huge lever to help with the maxime “XSS is RCE in electron” and yet they default to not helping. I know this is about backwards compatibility, but they could easily have decided to throw if the property is unset. Security-minded people would have set it to true and dealt with the fallout, whereas others could have set it to false and shipped their update still. But by defaulting to false, this security tool is hidden from both existing and new users. Old code will not even have the chance to get fixed and new code will be written in an insecure state. I’m sure the release notes talked about this feature, but who reads release notes? Especially not past release notes (when starting fresh today). The backwards compatibility cost of throwing and in the message even suggesting setting to false as an emergency out would have been minimal compared to the fallout this is causing.

Semaphor 6y ago
Maybe because they still wanted it to be easy to use? From further down: > Getting everything to work in a context isolated world was a many month effort, which had begun long before this bug was reported to our team. -- https://news.ycombinator.com/item?id=24823460 https://news.ycombinator.com/item?id=24823460
- pilif 6y ago
  I am well aware of the compatibility issues and the associated difficulties. I wasn't saying the option of turning it off shouldn't be given. I'm saying that the setting should have been mandatory. People wanting to do it the "easy" way (using scare quotes because turning any XSS into an RCE feels like hard mode to me) set contextIsolation to false and they are done. Also consider newly created applications: when the limitations exist from the start, it's much easier to build this correctly (the same way it's easier to get to 100% test coverage if you start with TDD than if you have to refactor years of static methods and global state and implicit dependencies). By having a default setting and having it default to insecure, even newly created applications will be unsafe and will require refactoring in the future.
smaddock 6y ago
For what it's worth, 'contextIsolation' will become enabled by default in Electron v12 [0]. [0] https://www.electronjs.org/docs/tutorial/context-isolation#what-is-it https://www.electronjs.org/docs/tutorial/context-isolation#w...
- stefan_ 6y ago
  The idea that this is even an option is hilarious. Call it "enableRce" maybe?
  - jrwr 6y ago
    Under most modes, I agree the setting should be turned on, but some advanced local only code bases / programs might require this option turned off/on
    - falcolas 6y ago
      How common are 'local only' electron apps anymore? I can't think of any program that I've run recently (even a large number of my non-vim terminal applications) which don't make some kind of remote calls.
  - megous 6y ago
    I can write secure apps even with this option disabled, thank you. Really it's no harder than preventing SQL injection. All I do is that I don't use APIs that can lead to interpreting data as code. So I assign to textContent liberally, don't manipulate URLs as text, but via browser APIs, and avoid everything that allows code execution. It's really stupid simple to follow safe practices for absolute majority of apps. And this makes writing a electron app with direct access to local PostgreSQL database trivial, fun, and makes writing useful apps without a separate HTTP backend a breeze. I don't have a need to split privileged code out to separate process, and go back to acting like there's trusted/untrusted split, just like with regular website. Having to do that would kill all the allure of Electron, for me. I use electron precisely because I don't have to do this, and can depend on the HTML/JS code to be trusted.
dvt 6y ago
I hate to be the "RTFM guy," but contextIsolation is pretty well-covered in their docs[1] (and it will be enabled by default beginning with Electron 12). [1] https://www.electronjs.org/docs/tutorial/security https://www.electronjs.org/docs/tutorial/security
beshrkayali 6y ago
This might be true, and in certain contexts I’d agree that defaulting to the secure option -while potentially breaking- could be the safer one. But I have to disagree with your point about reading release notes, this should be the expected behavior from developers working on the product, maybe not every single dependency but at least the main ones. Being an Electron app mainly, discord devs should’ve known and understood what they’re working with. Of course mistakes happen, but the main issue here is developers missed reading the docs properly, not whatever the default value is.
anaisbetts 6y ago
Loading a remote page and executing it in Electron is not the only, or even recommended, way to build Desktop apps using Electron. Electron was designed for creating Desktop apps, where the content is shipped in the app itself - in that case, contextIsolation is much less necessary because every way you could break out of the sandbox isn't interesting, it's just an obscure way to hack yourself. It's also a huge compromise to the developer experience, and is a big roadblock to anyone other than Electron experts. Unfortunately, the world disagrees - they really want to wrap websites using Electron, so now we need to default to options that make Electron apps far less compelling as a platform
- efdee 6y ago
  I would assume the danger is not your own content breaking out of the sandbox, but displaying unsanitized content from a remote database, which sadly still happens a lot more than you would expect. Definitely not something that only happens when wrapping a website using Electron.
- pilif 6y ago
  > it's just an obscure way to hack yourself. that's only true for applications that strictly only ever work on and display content that the user has created themselves. This is an increasingly rare type of application in the age of the internet and social media to the point where I would argue that most applications these days interact with data created by other users (think opening an image file, displaying a chat message, displaying an email, etc). With Electron you're setting yourself up in a way where a single case of a forgotten input escaping automatically means a remote code execution attack. On websites, XSS certainly are a bad thing, but they are still somewhat limited in scope and generic mitigations exists in many browsers. In Electron apps, an XSS means an RCE unless you make use of features like `conextIsolation`. Reading your comment makes it obvious to me that this isn't already a well-known issue. People generally aren't aware or believe their type of application to be immune and thus it would, and that's the point of my initial comment, make sense to either default to secure or at least default to asking the user to specifically make the decision to be insecure. Defaulting to turning every XSS attack into an RCE seems to me like a very bad decision.
- 6y ago

by pilif 6y ago

Semaphor 6y ago

Maybe because they still wanted it to be easy to use? From further down: > Getting everything to work in a context isolated world was a many month effort, which had begun long before this bug was reported to our team. -- https://news.ycombinator.com/item?id=24823460 https://news.ycombinator.com/item?id=24823460

pilif 6y ago
I am well aware of the compatibility issues and the associated difficulties. I wasn't saying the option of turning it off shouldn't be given. I'm saying that the setting should have been mandatory. People wanting to do it the "easy" way (using scare quotes because turning any XSS into an RCE feels like hard mode to me) set contextIsolation to false and they are done. Also consider newly created applications: when the limitations exist from the start, it's much easier to build this correctly (the same way it's easier to get to 100% test coverage if you start with TDD than if you have to refactor years of static methods and global state and implicit dependencies). By having a default setting and having it default to insecure, even newly created applications will be unsafe and will require refactoring in the future.

smaddock 6y ago

For what it's worth, 'contextIsolation' will become enabled by default in Electron v12 [0]. [0] https://www.electronjs.org/docs/tutorial/context-isolation#what-is-it https://www.electronjs.org/docs/tutorial/context-isolation#w...

stefan_ 6y ago
The idea that this is even an option is hilarious. Call it "enableRce" maybe?
- jrwr 6y ago
  Under most modes, I agree the setting should be turned on, but some advanced local only code bases / programs might require this option turned off/on
  - falcolas 6y ago
    How common are 'local only' electron apps anymore? I can't think of any program that I've run recently (even a large number of my non-vim terminal applications) which don't make some kind of remote calls.
- megous 6y ago
  I can write secure apps even with this option disabled, thank you. Really it's no harder than preventing SQL injection. All I do is that I don't use APIs that can lead to interpreting data as code. So I assign to textContent liberally, don't manipulate URLs as text, but via browser APIs, and avoid everything that allows code execution. It's really stupid simple to follow safe practices for absolute majority of apps. And this makes writing a electron app with direct access to local PostgreSQL database trivial, fun, and makes writing useful apps without a separate HTTP backend a breeze. I don't have a need to split privileged code out to separate process, and go back to acting like there's trusted/untrusted split, just like with regular website. Having to do that would kill all the allure of Electron, for me. I use electron precisely because I don't have to do this, and can depend on the HTML/JS code to be trusted.

dvt 6y ago

I hate to be the "RTFM guy," but contextIsolation is pretty well-covered in their docs[1] (and it will be enabled by default beginning with Electron 12). [1] https://www.electronjs.org/docs/tutorial/security https://www.electronjs.org/docs/tutorial/security

beshrkayali 6y ago

This might be true, and in certain contexts I’d agree that defaulting to the secure option -while potentially breaking- could be the safer one. But I have to disagree with your point about reading release notes, this should be the expected behavior from developers working on the product, maybe not every single dependency but at least the main ones. Being an Electron app mainly, discord devs should’ve known and understood what they’re working with. Of course mistakes happen, but the main issue here is developers missed reading the docs properly, not whatever the default value is.

anaisbetts 6y ago

Loading a remote page and executing it in Electron is not the only, or even recommended, way to build Desktop apps using Electron. Electron was designed for creating Desktop apps, where the content is shipped in the app itself - in that case, contextIsolation is much less necessary because every way you could break out of the sandbox isn't interesting, it's just an obscure way to hack yourself. It's also a huge compromise to the developer experience, and is a big roadblock to anyone other than Electron experts. Unfortunately, the world disagrees - they really want to wrap websites using Electron, so now we need to default to options that make Electron apps far less compelling as a platform

efdee 6y ago
I would assume the danger is not your own content breaking out of the sandbox, but displaying unsanitized content from a remote database, which sadly still happens a lot more than you would expect. Definitely not something that only happens when wrapping a website using Electron.
pilif 6y ago
> it's just an obscure way to hack yourself. that's only true for applications that strictly only ever work on and display content that the user has created themselves. This is an increasingly rare type of application in the age of the internet and social media to the point where I would argue that most applications these days interact with data created by other users (think opening an image file, displaying a chat message, displaying an email, etc). With Electron you're setting yourself up in a way where a single case of a forgotten input escaping automatically means a remote code execution attack. On websites, XSS certainly are a bad thing, but they are still somewhat limited in scope and generic mitigations exists in many browsers. In Electron apps, an XSS means an RCE unless you make use of features like `conextIsolation`. Reading your comment makes it obvious to me that this isn't already a well-known issue. People generally aren't aware or believe their type of application to be immune and thus it would, and that's the point of my initial comment, make sense to either default to secure or at least default to asking the user to specifically make the decision to be insecure. Defaulting to turning every XSS attack into an RCE seems to me like a very bad decision.
6y ago