Skip to content
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

Issue: Empty ipWhitelist array does not allow any host to access MM #950

Closed
QNimbus opened this issue Jul 17, 2017 · 10 comments
Closed

Issue: Empty ipWhitelist array does not allow any host to access MM #950

QNimbus opened this issue Jul 17, 2017 · 10 comments

Comments

@QNimbus
Copy link
Contributor

QNimbus commented Jul 17, 2017

When you set ipWhitelist = [] in the config file, it should allow access from everyone to the MM instance according to the documentation. Instead with an empty whitelist (express-whitelist module) it doesn't allow any host to access the MM instance.

I think this could be solved by replacing the following line in server.js

var result = ipfilter(config.ipWhitelist, {mode: "allow", log: false})

with

var result = ipfilter(config.ipWhitelist, {mode: config.ipWhitelist.length === 0 ? "deny" : "allow", log: false})

This will result in ipWhitelist = [] in the config file to allow any and all hosts to access the MM instance.

QNimbus added a commit to QNimbus/MagicMirror that referenced this issue Jul 17, 2017
Changed 'server.js' to allow an empty ipwhitelist to allow any and all hosts instead of none as mentioned in the documentation
@QNimbus
Copy link
Contributor Author

QNimbus commented Jul 17, 2017

To prevent some confusion during configuration (users customizing their config.js file), I've changed the default address which is used to specify addresses/interfaces where the server listens on, to listen on all interfaces by default.

If a user changes the default ipWhitelist (which by default only allows localhost) he/she won't be hampered by the fact that they might not have the address property configured in their local config.

QNimbus added a commit to QNimbus/MagicMirror that referenced this issue Jul 17, 2017
Changed 'server.js' to allow an empty ipwhitelist to allow any and all hosts instead of none as mentioned in the documentation
QNimbus added a commit to QNimbus/MagicMirror that referenced this issue Jul 18, 2017
Changed 'server.js' to allow an empty ipwhitelist to allow any and all hosts instead of none as mentioned in the documentation
MichMich added a commit that referenced this issue Jul 20, 2017
@qistoph
Copy link
Contributor

qistoph commented Jul 20, 2017

From a security perspective I would opt to prevent users from accidentally opening up access to their mirror. Listening only on, or allowing access from localhost woudl be wise from that perspective.

Take a look at Mirai for example.

Raspberry did a major change regarding these issues, by disabling SSH by default.

Therefore I would suggest to keep the default to limited access.

@QNimbus
Copy link
Contributor Author

QNimbus commented Jul 20, 2017

I agree with you - I will try to come up with a more elegant solution, and send a PR with reference to this issue.

@roramirez
Copy link
Contributor

There a log when is open a full ipWhitelist.

@qistoph
Copy link
Contributor

qistoph commented Aug 25, 2017

@QNimbus, any updates?

@roramirez, what do you mean?

@roramirez
Copy link
Contributor

When you are using a full IpWhitelist, MM alert you with a warning log entry in console.
https://github.com/MichMich/MagicMirror/blob/develop/js/server.js#L30

@qistoph
Copy link
Contributor

qistoph commented Aug 25, 2017

My concern is that the default install & run will leave you with an application listening on all network devices on any IP (include IPv6). Given the increase in IoT devices being abused I think this is something to try and prevent as software developers.

So I think the default install & run should at least prevent one of the two;

  • listen only on the localhost interface, or
  • listen only on the localhost IPs 127.0.0.1 and ::1

Edit
Also, with the default install & run (serveronly), I don't see that log entry anywhere...

qistoph added a commit to qistoph/MagicMirror that referenced this issue Aug 25, 2017
Without config, listen only on looback interface. In sample config
listen on any interface, but use an IP whitelist.

Related to MagicMirrorOrg#950
@roramirez
Copy link
Contributor

I've done the test and the log is show it.

root@testing-rpi-iso:~/MagicMirror# grep white config/config.js
root@testing-rpi-iso:~/MagicMirror# grep White config/config.js
	ipWhitelist: [],
root@testing-rpi-iso:~/MagicMirror# node serveronly/
Starting MagicMirror: v2.1.3-dev
Loading config ...
Loading module helpers ...
No helper found for module: alert.
Initializing new module helper ...
Module helper loaded: updatenotification
No helper found for module: clock.
Initializing new module helper ...
Module helper loaded: calendar
No helper found for module: compliments.
No helper found for module: currentweather.
No helper found for module: weatherforecast.
Initializing new module helper ...
Module helper loaded: newsfeed
All module helpers loaded.
Starting server on port 8080 ... 
You're using a full whitelist configuration to allow for all IPs
Server started ...
Connecting socket for: updatenotification
Connecting socket for: calendar
Starting node helper for: calendar
Connecting socket for: newsfeed
Starting module: newsfeed
Sockets connected & modules started ...

Ready to go! Please point your browser to: http://localhost:8080

qistoph added a commit to qistoph/MagicMirror that referenced this issue Sep 4, 2017
Without config, listen only on looback interface. In sample config
listen on any interface, but use an IP whitelist.

Related to MagicMirrorOrg#950
@MichMich
Copy link
Collaborator

Fixed in next release.

@mstreicher
Copy link

This is already not fixed!

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

No branches or pull requests

5 participants