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

disallow empty passwords ? #417

Closed
chipitsine opened this issue Apr 21, 2021 · 25 comments
Closed

disallow empty passwords ? #417

chipitsine opened this issue Apr 21, 2021 · 25 comments

Comments

@chipitsine
Copy link
Contributor

we observe the following pattern in user behaviour: when they setup new PC, sometimes they forgot to press TAB to switch to "password" prompt and type password right after the login, i.e. userpassword.

thus, password itself is empty, and sent as empty.

I'm not sure whether empty password is legitimate, maybe we should change OpenVPN-GUI behaviour to disallow empty passwords ?

I'd suggest to make "password" field red if OK is pressed on empty password. Thus we even do not have to translate it.

@selvanair , what do you think ?

@chipitsine
Copy link
Contributor Author

I cannot upload screenshot. Something changed in github upload :(

@selvanair
Copy link
Collaborator

selvanair commented Apr 21, 2021

In OpenVPN core, empty username is not allowed, but empty password is not an error. I guess passing only username has its use cases like SSO where the auth process is federated out, or done out of band by other means. In such cases one may not want to send the password to the server.

Ideally, we should have a way of indicating in the prompt process what inputs are required so that the rest can be greyed out in the dialog. Then it would be appropriate to demand a non-empty entries for all enabled inputs including password. In v2.6, we have some newer methods to support out of band authentication, but I don't think the initial username/password prompting has been improved.

We could stop making OK the default button as soon the username is filled-in so that hitting return would not submit the dialog if password is empty. Do this without disabling OK so that it could still be clicked with empty password.

@chipitsine
Copy link
Contributor Author

may I suggest the following approach ?

  1. default behaviour is changed, i.e. empty passwords are disallowed
  2. OK button is not disabled (people even do not press it, but hit Enter instead),
  3. if user hit Enter, password field becomes red on background, dialog is still there, connection is not started
  4. if empty password is supposed to be legitimate for some reason, it might be allowed, for example, in config file

or ....

we can keep current default behaviour, but introduce additional flag in config, if it is set, empty passwords become disallowed

@TinCanTech
Copy link

Could the GUI present a user dialogue window asking for empty password confirmation ?

@selvanair
Copy link
Collaborator

selvanair commented Apr 21, 2021

Let's not make it more user-unfriendly. Those who mistakenly submit empty passwords is no different from those who mistype passwords.

At best, we can make none of the buttons as default until all fields are filled in. That should take care of those who hit enter without filling in the password. At the same time allowing the user to press the OK button with empty password. Its either that or leave everything as is.

@TinCanTech
Copy link

Make the user select a button and not have OK/Enter selected by default. I see the logic now.

@chipitsine
Copy link
Contributor Author

disabling buttons and outlining mandatory fields as red would be nice

@cron2
Copy link
Contributor

cron2 commented Apr 22, 2021 via email

@chipitsine
Copy link
Contributor Author

Maybe Enter should change focus to password field if password is empty and OK otherwise?

@chipitsine
Copy link
Contributor Author

just to mention, this is actually pretty rare situation.
usually, both fields are filled, password is saved and login happens automatically (without even pressing Enter)

only when moving to new PC, user fills empty fields for the first time. and sometimes he did not press Tab button to switch fields

@cron2
Copy link
Contributor

cron2 commented Apr 22, 2021 via email

@chipitsine
Copy link
Contributor Author

I wonder whether "empty" password happens in case of 2FA. From my observation it should happen more often than I see for static passwords

@cron2
Copy link
Contributor

cron2 commented Apr 22, 2021 via email

@selvanair
Copy link
Collaborator

Turns out that there is no good way to do this while still allowing empty passwords. Having no default button doesn't seem to play very well in Windows dialogs.

I would have liked to keep allowing empty passwords -- it has its use cases and is supported by the core -- but may be the right way to do that would be have an explicit way for prompting only username.

So our options are (i) enable the OK button only if password is non-empty or (ii) leave this as is. (i) would be a regression but probably no one cares.

In all cases, enter will submit once the OK button is enabled as it's the default button.

@chipitsine
Copy link
Contributor Author

Is empty password a common case? We may introduce some setting for that

@TinCanTech
Copy link

TinCanTech commented Apr 22, 2021

Just a punt for an option, how about an empty checkbox:
[ ] Allow empty password.
And OK as default button on press Enter.

If the password and checkbox are both empty then throw an error.
If the password is filled in then the checkbox would not be required to continue.

And I guess the checkbox state would also be saved and reset only by user or re-install.
Or if a password is subsequently entered then the checkbox is auto-cleared.

@chipitsine
Copy link
Contributor Author

yeah, either checkbox or config setting (so, config would be delivered right with empty passwords)

@selvanair
Copy link
Collaborator

I decided to go with @chipitsine 's original suggestion -- i.e., require non-empty password (PR #419).

If the administrator doesn't want the user to submit a password, one should not prompt for it. So instead of making it even more difficult for the user having to ignore an input and also click additional check boxes, let's just say password input is required if prompted for.

The "submit only username" can be handled when OpenVPN grows proper support for indicating it.

@ilkondr
Copy link

ilkondr commented Jan 30, 2023

You've just made it absolutely impossible to connect using perfectly valid config (with empty password), because some people forget to press TAB. Did I get it right?
Are you crazy guys?

@cron2
Copy link
Contributor

cron2 commented Jan 30, 2023

Why is your config asking for a password if "empty" is a valid answer? Just leave auth {{{--auth-user-pass}}} from the config and be happy everafter.

@ilkondr
Copy link

ilkondr commented Jan 30, 2023

I think it is still asking for username as far as I understand.
My server is Mikrotik device and I can't connect with no auth-user-pass option.

@selvanair
Copy link
Collaborator

I was reluctant to not allow empty password, and was a compromise. Given that openvpn.exe expects a non-empty username, but accepts empty password, this was not a smart change.

@schwabe
Copy link

schwabe commented Jan 30, 2023

If a configuration requires an empty password you should not use auth-user-pass in the config. And if the UI is still asking for a password that is a bug. If the command line openvpn allows an empty password we might want to adjust that to also require a password.

@selvanair
Copy link
Collaborator

selvanair commented Jan 30, 2023

Empty password currently serves as a way of providing a username even when password is not in use or when password has to be submitted by other means. Eg., when the actual authentication is federated out, submitting just the username is a useful feature. In such case one doesn't want to expose the password to the server. There could be other uses of username-only.

So, unless we add a new option like auth-username, supporting empty password is a useful thing.

Not saying that showing a dialog with two fields when only one is required is good UX.

@darkbasic
Copy link

A security researcher friend of mine asked me how this issue is supposed to be linked to my CVE so I would like to point out to anybody else who stumbles upon this that they linked the wrong issues in the v2.6.7 release notes. The correct links were supposed to be the following (same numbers but different repository/type):
OpenVPN/openvpn#400
OpenVPN/openvpn#417

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

7 participants