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

Web UI does not accept wifi password with * #5231

Closed
dgmax opened this issue Feb 15, 2019 · 32 comments
Closed

Web UI does not accept wifi password with * #5231

dgmax opened this issue Feb 15, 2019 · 32 comments
Labels
bug Type - Confirmated Bug fixed Result - The work on the issue has ended

Comments

@dgmax
Copy link

dgmax commented Feb 15, 2019

wifi passwd can't used *

examples: Auik987222*

@andrethomas
Copy link
Contributor

andrethomas commented Feb 15, 2019

I think * is used by the webui to mask the current password and to detect if a change is needed when the save button is clicked so until that is changed it will not accept *

Issue #5010 seems to be related where @ascillato mentions that the characters referenced in that issue is not supported by the core... it is marked as a duplicate issue so my guess is if you search the issues you will find what the outcome was but I know that this is not the first time this issue is raised.

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019

Tasmota used to be happy with "*" in the password, and now people are saying it is "not supported"?

This needs to be resolved.

"Not supporting ''" has turned into a disaster for me ... yesterday, I OTA flashed a plug, set the wifi password - which in my network starts with '' and has ever since wifi arrived on this planet ...
So, I open the plug up with a hammer, solder on the debug cables and find that, as the password is wrong, the plug sits in the following loop:

`00:02:20 WIF: Connecting to AP1 ROGERS in mode 11N as sonoff-3959...

00:02:37 WIF: Connect failed with AP timeout

00:02:37 WIF: Connecting to AP1 ROGERS in mode 11N as sonoff-3959...

00:02:54 WIF: Connect failed with AP timeout`

So, first, it is a bug that "*" is not supported - the UI needs to be sorted out. Second (and I will raise a bug for this), it is a bug that Tasmota does not initiate the configuration AP when it cannot connect to the network.

@ascillato
Copy link
Contributor

ascillato commented Feb 15, 2019

Sorry, it is not supported. Some special characters are not supported for password.

Sorry if this is not what you want, but are deliberately not supported.

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019

Previous versions of Tasmota were happy with "*" in the password - and now it does not work. This makes it a bug, or the limitation should be noted in very large characters IMHO.

When, and why, was the decision made to deliberately remove support for reasonable characters? What API is not working?

The Arduino API accepts the characters - if I enter the password via the serial debug, the password is used correctly.

@Jason2866
Copy link
Collaborator

It is a design decision from the project owner.
If you dont like the way it works feel free to fork and change it.
It is Open Source :-)

@ascillato
Copy link
Contributor

Since the password ofuscation for the webmenu was inserted in version 5.10 the * was not supported. Sorry.

This is a known limitation. If you look in issues you will find the original issue.
We have postpone expanding the available characters due it requires major changes in the webUI. In the future may be supported. Sorry that now it is not supported.

@ascillato2 ascillato2 added the duplicated Result - Duplicated Issue label Feb 15, 2019
@ascillato2
Copy link
Collaborator

Closing issue as it is duplicated and as this is a known limitation that was already marked to be worked on later.

@andrethomas
Copy link
Contributor

The people on TV use ******** as their passwords... you can see them type it sometimes :)

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019 via email

@andrethomas
Copy link
Contributor

You actually don't need the *** to be displayed and submitted when the password is not changing. If its length is 0 then just ignore it and keep the original password - if it has something in it then update it accordingly. This was the original proposal from @ascillato - just nobody gotten around to it yet.

There are some issues with the ESP8266 core though relating to special characters... I can't remember all of them though.

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019 via email

@ascillato
Copy link
Contributor

ascillato commented Feb 15, 2019

Great!

Remember:

  • the password should not be sent to the webpage because it can be seen with a password viewer for Chrome
  • the user should be able to use a blank password (yes, some people use that)
  • the user should be able to modify its password
  • the user should be able to change SSID but left its password unchanged.
  • the user should be able to change any data of just one SSID and be able to left the other SSID unchanged

@ascillato
Copy link
Contributor

Just for reference, this issue is duplicated from #3654

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019 via email

@ascillato
Copy link
Contributor

The newer core (2.5.0) supports now some special characters. If you search in issues in arduino repository you will find it. But the issue is if you use an old core like 2.3.0 that does not supports some special characters.

As some users are using 2.3.0, those characters also needs to be discarded, so passwords containing those, should be rejected. They produce software watchdogs.

Please, check in issues of arduino esp8266 core. Thanks.

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019

Understood.

Meanwhile, I have 2 ideas for simple fixes which should improve matters:

  • Compare the value returned in the html form with D_ASTERIX - which actually will then not allow 8 asterisks ("********") as a password. This is probably better than not allowing a password with an asterisk included?

  • Set the value on the form to be less than 8 characters (for example 7 asterisks). If the length of the password field is 0, this means an open network. Otherwise if the length is less than 8, it's invalid in WPA2 so in the real world user interface there would be an error message, but this is embedded. For the purpose of a Tasmota device, we could silently use the default password as now.

Thoughts? Should I fix this throughout xdrv_01_webserver.ino and make a pull request ? We then could add a specific note to the wiki to say that this one password is ignored ? Alternatively, we say that passwords must be 8 characters or more - and then we can accept "blank" passwords for open wifi networks.

The alternative to this fix would be to put some fairly tricky javascript together - and this would be overkill for what is a tricky problem. In fairness, we should look at a wholly different approach if this is the case.

@andrethomas
Copy link
Contributor

andrethomas commented Feb 15, 2019

@spock64 check what @ascillato said - its unlikely that a PR for this will be merged to Sonoff-Tasmota development because we plan to continue support for 2.3.0 core. As a matter of fact, earlier communication between myself and @arendst is indicative that I will even be pushing 2.3.0 based development binaries.

The reason for continued support for the 2.3.0 core is because some wifi setup's are not happy with the 2.4.2 or 2.5.0 implementation.

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019

So, I get the idea that there are issues with 2.3.0 core. My proposal does not make things worse for anyone using this version - things would be as good or bad as they are now.

What I am proposing to fix is the needless Tasmota-imposed limitation around passwords containing the asterisk - right now any password is accepted by the code, provided it does not contain an asterisk. I understand that there are problems with the Arduino libraries and special characters no doubt due to html header processing issues - but the current Tasmota implementation does not do anything about these problems. It simply adopts a rather simplistic algorithm that "If password contains "*", it's bad, if not it's good". My proposal is that zero length passwords are used to mean "", other passwords of less than 8 characters are ignored and overwritten by the default or the existing password. Passwords of 8 characters or more are saved for use.

Why would a simple PR like this not be accepted when it improves the product without any new side effects?

@andrethomas
Copy link
Contributor

If you write it properly you could possibly make it core independent I guess.

@spock64
Copy link
Contributor

spock64 commented Feb 15, 2019

The current code is core-independent - all the action is in xdrv_01_webserver.ino.

I'm proposing to change ...

strlcpy(Settings.sta_pwd[0], (!strlen(tmp)) ? "" : (strchr(tmp,'*')) ? Settings.sta_pwd[0] : tmp, sizeof(Settings.sta_pwd[0]));

to ...
strlcpy(Settings.sta_pwd[0], (!strlen(tmp)) ? "" : (!strcmp(tmp, D_ASTERIX)) ? Settings.sta_pwd[0] : tmp, sizeof(Settings.sta_pwd[0]));

This would be done both for both passwords.

This change would refuse passwords that matched D_ASTERIX.

We could instead have ...

strlcpy(Settings.sta_pwd[0], (strlen(tmp)>=8) ? tmp : (strlen(tmp) ==0) ? "" : Settings.sta_pwd[0], sizeof(Settings.sta_pwd[0]));

This change would allow zero length passwords or passwords of 8 or more characters. Passwords from 1 to 7 characters in length would be ignored.

Either of my suggestions is an improvement on the arbitrary filtering of the asterisk.

BTW, I know that asterisks in passwords are not a problem in 2.3.0 because I have hardware using Tasmota pre 5.10 that are working with my asterisk-laden password.

What do you think?

@Jason2866
Copy link
Collaborator

It is only important what the project owner thinks.
Just do a PR and you will see.

@andrethomas
Copy link
Contributor

@spock64 A good place to start would be to test the change on all the cores and make sure it does not have any impact. Then you can add that information to your PR for the owner to decide if he wants to merge it or not.

@ascillato2
Copy link
Collaborator

@spock64

Remember that the solution need to be fool proof (I didn't make a typo there), so if someone had a password like abcd1234 and he/she wanted to change to abcd1235, may be he/she will just delete the last * and add a 5 thinking it is going to work, ending in a incorrect password:*******5

Also, it has to be a backwards compatible solution. So it should accept passwords of any lenght (if someone do an upgrade and his/her password is no longer supported, when trying to change it will have also issues)

So, there are several conditions that needs to be met for a proper password check routine.

By now, you are realizing that it will not be a super simple fix just to accept the *.

Also, at this moment I don't recall exactly but we need to verify, if you use a * as part of a password in core 2.3.0, it don't work (it is a core issue, not Tasmota if I recall correctly) . If you use in latest core 2.5.0 I know it works.

That I think was the decision of not allowing * in the password.

Anyway, I agree with you that this password routine needs an enhancement but it is not super simple that. I think it can be achieved but with a major rewrite and meeting several conditions as the ones stated before.

Anyway, if you want to work on that, you have our support, but all those conditions have to be met.

@spock64
Copy link
Contributor

spock64 commented Feb 16, 2019 via email

@joba-1
Copy link
Contributor

joba-1 commented Feb 16, 2019

how can you argument the logic of spock :) you have my thumbs up, but would you care showing me the good reasons for not letting me update my obscured password (i.e uppercase the third character). As a user I always hated this.

@spock64
Copy link
Contributor

spock64 commented Feb 16, 2019 via email

@arendst
Copy link
Owner

arendst commented Feb 16, 2019

@spock64 thx for the elaborate information.

Tasmota is all about as much functionality in a tight space.
As tasmota is a wifi client I think enforcing passwords of any lenght is not it's task as they need to be in accordance with the router's password. That removes code needed for input control.

@spock64
Copy link
Contributor

spock64 commented Feb 16, 2019

@arendst Thx for making your view clear. And also for this great project!

Would you be happy with a PR that removes the filtering of the asterisk? As you say, it’s up to the router to know whether it’s ok or not.

@arendst
Copy link
Owner

arendst commented Feb 16, 2019

@spock64 pls do.

@andrethomas2 andrethomas2 removed the duplicated Result - Duplicated Issue label Feb 16, 2019
@andrethomas2 andrethomas2 changed the title wifi passwd Web UI does not accept wifi password with * Feb 16, 2019
@andrethomas2 andrethomas2 added enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended labels Feb 16, 2019
@andrethomas
Copy link
Contributor

Fix merged to development with #5242

@andrethomas2 andrethomas2 added bug Type - Confirmated Bug and removed enhancement Type - Enhancement that will be worked on labels Feb 16, 2019
arendst added a commit that referenced this issue Feb 16, 2019
 * Fix GUI wifi password acception starting with asteriks (*) (#5231, #5242)
 * Add rule expression enabled  by define USE_EXPRESSION in my_user_config.h (#5210)
@ascillato
Copy link
Contributor

Just a note to myself for further testings.

I have just read that some other firmwares that also uses Arduino Esp8266 core are having issues with wifi passwords containing the following characters.

:"+$%{;*!"/\

Will test later to find out the exactly unsupported list.

@spock64
Copy link
Contributor

spock64 commented Feb 17, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type - Confirmated Bug fixed Result - The work on the issue has ended
Projects
None yet
Development

No branches or pull requests

9 participants