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

Security issue ? wireless information not protected #1587

Closed
matsube11 opened this issue Jan 13, 2018 · 20 comments
Closed

Security issue ? wireless information not protected #1587

matsube11 opened this issue Jan 13, 2018 · 20 comments
Labels
stale Action - Issue left behind - Used by the BOT to call for attention

Comments

@matsube11
Copy link

Sonoff-Tasmota: 5.11.1 20180107
Sonoff wifi

Hi,

I realized today while I was rebooting my wireless access point that my Sonoff device once disconnected was showing the wifi parameters page without requiring any password. Even though I setup a web admin password, password is only requested if changes are to be made on the page.

So it's trivial to get the wifi SSID and password information from someone using a sonoff tasmota, just disconnect it from wifi, connect to it and that's it.

Am I missing something here ???

Thanks

@davidelang
Copy link
Collaborator

davidelang commented Jan 13, 2018 via email

@matsube11
Copy link
Author

Thanks, changed successfully using WifiConfig 4

I'm new to Tasmota, this is amazing piece of software but this default behavior is a critical security breach imho.
Wonder how many people have their wifi crendentials open for anyone to access due to that :/

@ascillato
Copy link
Contributor

ascillato commented Jan 14, 2018

Hi,

I test this issue and this allows you to change the Wifi configuration so you can take yours neighbour's sonoff and connect it to your network. In that initial page you only can see the SSID but not the password.

After that, you can not control it. The Sonoff ask you for the user and password that you previously configure.

So, this is just an anoying issue that can take your sonoff from your network.

I consider that a possible solution is to automatically change the WIFI_CONFIG_TOOL to WIFI_RETRY after the sonoff connect to a network. Then you should be able to configure the WIFI_CONFIG_TOOL on the Webserver Menus.

I can take a look on doing this if this solution is considered ok.

@davidelang
Copy link
Collaborator

davidelang commented Jan 14, 2018 via email

@ascillato
Copy link
Contributor

ascillato commented Jan 14, 2018

The logic I follow was:

1- On the first time you power up the sonoff, it shows as AP so you can set up your WIFI with your phone
2- Sonoff connects to your Wifi
3- Now you can set up the features of your sonoff over your wifi.

From step 2 to 3, Sonoff can change the
WIFI_CONFIG_TOOL: WIFI_MANAGER
to
WIFI_CONFIG_TOOL: WIFI_RETRY

Because now your Sonoff is attached to your Network. Each time it get disconnected from your router, will reconnect to it. But, if you want that each time the sonoff get disconnected from your wifi the sonoff shows itself as AP,, you should be able to configure that from the EXTRA CONFIG menu of the webserver.

What do you think?

@davidelang
Copy link
Collaborator

davidelang commented Jan 14, 2018 via email

@ascillato
Copy link
Contributor

ascillato commented Jan 14, 2018

Yes, WIFI_MANAGER_ONCE is what I meant.

I found that the variable Settings.sta_config stores the value of WIFI_CONFIG_TOOL.

So this mode (WIFI_MANAGER_ONCE) just only need to change Settings.sta_config = WIFI_MANAGER to Settings.sta_config = WIFI_RETRY

But also you should be able to change this from the Config Extra Menu

EDIT:
@davidelang, I found and test this Button Usage
So, with this feature that is already on Tasmota you can get to the device if the network it's trying to connect to no longer exists

@ascillato
Copy link
Contributor

After testing some code to solve this issue, and also with the feedback from @davidelang, I think that the best solution to this is that: If the web admin password is set, then, when you try to connect to the Sonoff AP, it asks you for that password.

What do you think? I will try this.

@matsube11
Copy link
Author

matsube11 commented Jan 14, 2018

I would have expected what @ascillato suggests, meaning that access to Sonoff AP would be password protected.

I test this issue and this allows you to change the Wifi configuration so you can take yours neighbour's sonoff and connect it to your network. In that initial page you only can see the SSID but not the password.

Just use ShowPassword addon for Chrome for example and you'll see that you have instant access to the wifi password as well :/

@ascillato
Copy link
Contributor

ascillato commented Jan 14, 2018

Changes proposed for: webserver.ino so as to solve the security issue #1587 from @matsube11 (Security issue ? wireless information not protected)

-Stop sending password information to client
-Credenditals ask if are set when Sonoff is on AP mode.

1- Never send password information to client. Instead send 10 asterisks.

Line 207
original:

  "<br/><b>" D_AP1_PASSWORD "</b><br/><input id='p1' name='p1' type='password' placeholder='" STA_PASS1 "' value='{p1'><br/>"

proposed:

  "<br/><b>" D_AP1_PASSWORD "</b><br/><input id='p1' name='p1' type='password' placeholder='" D_AP1_PASSWORD "' value='{p1'><br/>"

Line 209
original:

  "<br/><b>" D_AP2_PASSWORD "</b><br/><input id='p2' name='p2' type='password' placeholder='" STA_PASS2 "' value='{p2'><br/>"

proposed:

  "<br/><b>" D_AP2_PASSWORD "</b><br/><input id='p2' name='p2' type='password' placeholder='" D_AP2_PASSWORD "' value='{p2'><br/>"

Line 239
original:

  "<br/><b>" D_WEB_ADMIN_PASSWORD "</b><br/><input id='p1' name='p1' type='password' placeholder='" WEB_PASSWORD "' value='{p1'><br/>"

proposed:

  "<br/><b>" D_WEB_ADMIN_PASSWORD "</b><br/><input id='p1' name='p1' type='password' placeholder='" D_WEB_ADMIN_PASSWORD "' value='{p1'><br/>"

Line 790
original:

  page.replace(F("{p1"), Settings.sta_pwd[0]);

proposed:

  page.replace(F("{p1"), "**********");

Line 792
original:

  page.replace(F("{p2"), Settings.sta_pwd[1]);

proposed:

  page.replace(F("{p2"), "**********");

Line 886
original:

  page.replace(F("{p1"), Settings.web_password);

proposed:

  page.replace(F("{p1"), "**********");

2- The original code that save the passwords, have a comparison to see if you enter nothing and, if that the case, it will put the default password. I think that if you put nothing, the password should be nothing as I learnt from @davidelang to avoid the "I know what's best for you" features hehe :)
Also, the following code has a comparison to see if the password has asterisks so as not to save anything

Line 967 to 970
original:

    strlcpy(Settings.sta_ssid[0], (!strlen(WebServer->arg("s1").c_str())) ? STA_SSID1 : WebServer->arg("s1").c_str(), sizeof(Settings.sta_ssid[0]));
    strlcpy(Settings.sta_pwd[0], (!strlen(WebServer->arg("p1").c_str())) ? STA_PASS1 : WebServer->arg("p1").c_str(), sizeof(Settings.sta_pwd[0]));
    strlcpy(Settings.sta_ssid[1], (!strlen(WebServer->arg("s2").c_str())) ? STA_SSID2 : WebServer->arg("s2").c_str(), sizeof(Settings.sta_ssid[1]));
    strlcpy(Settings.sta_pwd[1], (!strlen(WebServer->arg("p2").c_str())) ? STA_PASS2 : WebServer->arg("p2").c_str(), sizeof(Settings.sta_pwd[1]));

proposed:

    strlcpy(Settings.sta_ssid[0], (!strlen(WebServer->arg("s1").c_str())) ? "" : WebServer->arg("s1").c_str(), sizeof(Settings.sta_ssid[0]));    
    strlcpy(Settings.sta_pwd[0], (!strlen(WebServer->arg("p1").c_str())) ? "" : (strchr(WebServer->arg("p1").c_str(),'*')) ? Settings.sta_pwd[0] : WebServer->arg("p1").c_str(), sizeof(Settings.sta_pwd[0]));
    strlcpy(Settings.sta_ssid[1], (!strlen(WebServer->arg("s2").c_str())) ? "" : WebServer->arg("s2").c_str(), sizeof(Settings.sta_ssid[1]));
    strlcpy(Settings.sta_pwd[1], (!strlen(WebServer->arg("p2").c_str())) ? "" : (strchr(WebServer->arg("p2").c_str(),'*')) ? Settings.sta_pwd[1] : WebServer->arg("p2").c_str(), sizeof(Settings.sta_pwd[1]));

Line 1018
original:

    strlcpy(Settings.web_password, (!strlen(WebServer->arg("p1").c_str())) ? WEB_PASSWORD : (!strcmp(WebServer->arg("p1").c_str(),"0")) ? "" : WebServer->arg("p1").c_str(), sizeof(Settings.web_password));

proposed:

strlcpy(Settings.web_password, (!strlen(WebServer->arg("p1").c_str())) ? "" : (strchr(WebServer->arg("p1").c_str(),'*')) ? Settings.web_password : WebServer->arg("p1").c_str(), sizeof(Settings.web_password));

3- If there is a password set and you are not authenticated, then ask for credentials. (if there is a password set and you have WIFI_CONFIG_TOOL = WIFI_MANAGER on "user_config.h line 62", when Sonoff disconnects from wifi network it changes to AP. You can connect to that AP and it ask for user and password to continue / otherwise, if you do not have password set, it just shows the wifi config page)

Line 414
original:

  if((HTTP_ADMIN == webserver_state) && (Settings.web_password[0] != 0) && !WebServer->authenticate(WEB_USERNAME, Settings.web_password)) {

proposed:

  if((Settings.web_password[0] != 0) && !WebServer->authenticate(WEB_USERNAME, Settings.web_password)) {

obs: When the web password is NOT SET, connecting with a phone to the AP will show automatically the captive webpage <-- cool :)
When the web password is SET, connecting with a phone to the AP will NOT show anything. You have to manually open web browser and type 192.168.4.1 and after that it asks you for User and Pass to show the config webpage.
If you use a PC you have to open the browser in both cases and type 192.168.4.1 to show the config webpage.

Tested Ok.

What do you think of all this approaches?

What about having an initial page asking for credentials instead of the pop up asking for credentials? So when connecting with a phone to the AP will show automatically the captive webpage, having set or not the Web Admin Password.

@ascillato
Copy link
Contributor

ascillato commented Jan 15, 2018

Just tested an initial page asking for credentials to be used by the captive portal, and works better than the previous approach I post.

The webserver.ino file for the previous pull request (#1601) was updated with this change.

Hope to be useful.

I think, the issue #1587 can be closed now.

Nice work finding this issue. Congrats @matsube11

And Tasmota is an amazing software. Congrats @arendst

@ascillato
Copy link
Contributor

Screenshots of this changes:

If you set the web admin password, when you connect to the AP you will see:

1

so,

2

then,

3

so you can save your SSID and PASS and will connect to it.

@stefanbode
Copy link
Contributor

stefanbode commented Jan 16, 2018

I don't know when I checked the AP mode last time, but there you get to the normal webpage and could configure everything. Including having the webpassword request. The current behavior is for sure a leak and a second proof that for IoT devices you should have a separate network :-).

@ascillato I really like all your changes. Additionally, I would vote also for the AP mode that we use the normal UI and not the stripped down one currently available

@matsube11
Copy link
Author

Wow, impressed to see a nice fix coming that fast

Thanks @ascillato and all

arendst added a commit that referenced this issue Jan 20, 2018
5.11.1d
 * Add locale Decimal Separator to Web sensor page
 * Add
command State to retrieve device state information (same data as
teleperiod state and status 11 in slightly different JSON format)
 *
Extent state information with Light parameters
 * Fix IRSend parameter
translation (#1636)
 * Add optional login to Webserver AP mode (#1587,
#1635)
 * Fix BME680 teleperiod resistance measuring (#1647)
@Ivoz
Copy link

Ivoz commented Feb 5, 2018

Couldn't this also be solved in a somewhat simpler fashion by having the 'web admin' password actually be a password for the webserver to enable HTTP Basic Authentication? Thereafter, it should not be sending any passwords over HTTP forms to users who cannot auth with the password anyway.

@ascillato
Copy link
Contributor

ascillato commented Feb 5, 2018

The issue related to Login and sending the wifi password to the web page is already solved and implemented on Tasmota v5.11.1i

The only issue solved on PR#1727 that is not included on current Tasmota version is that Tasmota is sending MQTT password to the web page.

If @arendst decide that sending MQTT password is not an issue and that the WEB ADMIN password is enough, this issue and the PR should be closed

arendst added a commit that referenced this issue Feb 9, 2018
5.12.0 20180209
* Change library PubSubClient.h define MQTT_MAX_PACKET_SIZE from 512 to
1000 for Home Assistant  support
* Change relation of define MESSZ being dependent on PubSubClient.h
define MQTT_MAX_PACKET_SIZE
* Change command color parameter input checks to less strict for Home
Assistant support
* Change command Ina219Mode into command Sensor13
* Change commands HlwPCal, HlwUCal and HlwICal to PowerCal, VoltageCal
and CurrentCal to be used for both Pow and S31 calibration
* Change commands HlwPSet, HlwUSet and HlwISet to PowerSet, VoltageSet
and CurrentSet to be used for both Pow and S31 calibration
* Change uptime from hour to second resulting in a display of
123T13:45:21 where 123 is days
* Change module name Wemos D1 mini into Generic (#1220)
* Change HTML from width=100% to style=width:100% supporting HTML5
(#1358)
* Change OSWATCH_RESET_TIME (Blocked loop) from 30 to 120 seconds to
allow slow networks (#1556)
* Change WIFI_MANAGER_SEC into WIFI_CONFIG_SEC (#1616)
* Change function pointers code to save code space and memory (#1683)
* Change webserver argument processing gaining 5k code space (#1705)
* Change weblog memory usage (#1730, #1793, #1819)
* Update TasmotaSerial library to 1.1.0
* Update language files Italian (#1594), Dutch (#1723) and Spanish
(#1722)
* Fix Non-English JSON temperature unit attachement
* Fix Arilux RF induced exception by moving interrupt handler to iram on
non ESP8266/Arduino lib v2.3.0
* Fix truncated command names and wrong response for DomoticzSwitchIdx
(#1571)
* Fix %-sign issue as printf escape character in Humidity and Sonoff SC
(#1579)
* Fix DS18B20 temperature JSON decimal dot (#1561)
* Fix Energy JSON message (#1621)
* Fix IRSend parameter translation (#1636)
* Fix TSL2561 device detection (#1644, #1825)
* Fix BME680 teleperiod resistance measuring (#1647)
* Fix Energy Monitoring Energy Today and Energy Total reading after
restart (#1648)
* Fix IRReceive Data value (#1663)
* Fix Energy Monitoring Energy Period roll-over (#1688)
* Fix compiler warnings (#1774)
* Fix command PWM response if no PWM channel is configured (#1783)
* Add locale Decimal Separator to Web sensor page
* Add ColorTemperature to light status message
* Add command PowerOnState option 5 which inverts PulseTime and allows
for delayed always on after power on
* Add OtaMagic two step Web server OTA upgrade using filename-minimal
image if OTA free space is too small
* Add support for PMS5003 and PMS7003 particle concentration sensor
* Add command SetOption21 1 to allow Energy Monitoring when power is off
on Sonoff Pow and Sonoff S31 (#1420)
* Add Chinese language file (#1551)
* Add French language file (#1561)
* Add Spanish language file (#1589)
* Add HTTP Allow Cross Origin removed from ESP8266/Arduino lib v2.4.0
(#1572)
* Add Home Assistant MQTT Discovery for switch and light to be enabled
by command SetOption19 1 (#1534) or define
HOME_ASSISTANT_DISCOVERY_ENABLE in user_config.h (#1685)
* Add command State to retrieve device state information (same data as
teleperiod state and status 11 in slightly different JSON format)
* Add optional login to Webserver AP mode (#1587, #1635)
* Add command Sensor15 2 to start MHZ19(B) Zero Point Calibration
(#1643)
* Add support for Sonoff S31 Smart Socket with Power Consumption
Detection (#1626)
* Add command SetOption20 to allow update of Dimmer/Color/Ct without
turning power on (#1719, #1741)
* Add NTP sync time slot based on chip id (#1773)
* Add cursor pointer to web button (#1836)
@richrd
Copy link

richrd commented Feb 18, 2018

Why can't we add a WIFI password to the actual AP itself? Seems like the way to go IMHO. Why let anyone and everyone even get in to the AP?

@Ivoz
Copy link

Ivoz commented Feb 18, 2018

@richrd maybe some people want to allow guests on the AP, but not to be able to read server daemon logins just by visiting the web server of an IP that's also on the AP.

@richrd
Copy link

richrd commented Feb 19, 2018

@Ivoz Sounds reasonable. Could we add an option for a AP password for those that want it? I could happily write a PR for that.

@stale
Copy link

stale bot commented Jun 6, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Action - Issue left behind - Used by the BOT to call for attention label Jun 6, 2018
curzon01 pushed a commit to curzon01/Tasmota that referenced this issue Sep 6, 2018
5.11.1d
 * Add locale Decimal Separator to Web sensor page
 * Add
command State to retrieve device state information (same data as
teleperiod state and status 11 in slightly different JSON format)
 *
Extent state information with Light parameters
 * Fix IRSend parameter
translation (arendst#1636)
 * Add optional login to Webserver AP mode (arendst#1587,
arendst#1635)
 * Fix BME680 teleperiod resistance measuring (arendst#1647)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Action - Issue left behind - Used by the BOT to call for attention
Projects
None yet
Development

No branches or pull requests

7 participants