Skip to content

Fix ASP.NET backend listening on 0.0.0.0 #266

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

Merged
merged 5 commits into from
Jul 1, 2019
Merged

Fix ASP.NET backend listening on 0.0.0.0 #266

merged 5 commits into from
Jul 1, 2019

Conversation

gfs
Copy link
Contributor

@gfs gfs commented May 31, 2019

Why this change?
Default behavior for windows appears to be to resolve localhost to 0.0.0.0 which causes the Windows firewall to get triggered, but also exposes the ASP.NET backend to remote access which opens up the app to RCE risks unnecessarily.

Also

  1. Add a comment for why NodeIntegration is true.

  2. The version used in the build script is now a variable, only needs to be changed once.

I've validated that with this changed behavior, our App, Attack Surface Analyzer, encounters no issues. I'm unable to test it on the WebApp as the version on master currently does not work for me (even without my changes).

gfs added 3 commits May 30, 2019 17:33
This should be the default as it is more secure. It is trivial for applications which wish to take on the additional attack surface to set "            browserWindowOptions.WebPreferences.NodeIntegration = false;"
@GregorBiswanger GregorBiswanger self-assigned this May 31, 2019
@GregorBiswanger
Copy link
Member

Hi @gfs, thanks for your contribution!

I had deliberately activated NodeIntegration by default.
The problem is that otherwise no IPC communication can take place via the views to controllers. It's a common standard at Electron.NET to want that. I think that would limit the entry and compatibility with existing applications.

What do you think about this?

@robertmuehsig
Copy link
Collaborator

Preventing the Windows firewall dialog is always great - in a recent PR I thought I fixed the problem, but maybe I was just looking on the nodejs side 🤔
Besides that: I like the env in the build script 👌

@gfs
Copy link
Contributor Author

gfs commented May 31, 2019

@GregorBiswanger Understand now. I think it makes sense to keep your existing default in that case, it would be rough to break a bunch of people. I reverted the node integration change and added a note in the documentation.

gfs and others added 2 commits May 31, 2019 08:57
Add comment to note why it is true.
@robertmuehsig robertmuehsig merged commit 428e53e into ElectronNET:master Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants