Local passwords with leading/trailing white-space can be created but not used to authenticate

taniwallach commented 5 years ago

Issue:

Release 2.14 added a trim() function to the code of lib/WeBWorK/Authen.pm and it is used to remove leading and trailing white-space from passwords submitted and processed by the "regular" local authentication code.

On the other hand, (most/all of???) the code which puts passwords into the database does no such trimming to the value before it is crypted and stored in the database. For example, the Classlist editor can be used to set a password which has leading/trailing white-space.

Due to this conflict, passwords may be set in the database which cannot actually be used to access the system.

Options:

Trim passwords in a similar manner before they are crypted but don't tell the users.
- It may be possible to do this by doing the trimming in cryptPassword() in lib/WeBWorK/Utils.pm.
  - Pro: Can probably be implemented with minimal disruption to the current mechanisms for setting passwords, and without needed to report errors or handle corrections.
  - Con: Many variants of a "password" with different leading and trailing white-space will all be treated as identical.
  - Con: Users may think their passwords are somewhat "stronger" (longer, etc.) than they really are.
Check passwords before they are crypted and reject proposed passwords with leading/trailing white-space with suitable error messages to the relevant users and providing a chance to enter a different new password.
- Testing must be done on the server side, and not only be on the user-side (ex. via JavaScript validity checking) as "clever" people can bypass that editing the form submission values using existing tools.
- Pro: Users will know the "real" password, and other password strength requirements can be tested.
- Con: Need to handle all the checks and the feedback, including in code used to set passwords in bulk (whether via loaded files or tables of records in the ClassList editor).

This issue was raised, somewhat in passing, in the discussed in https://github.com/openwebwork/webwork2/pull/911

Also related:

The commits in PR 911 totally deny the ability to log in with an empty or all white-space password using local authentication (except if a site_checkPassword() function exists and allows it).
However, there is no code to prevent such a password from being created by a user.
Such empty passwords are apparently automatically created for special settings such as wwassignment and LTI only users, and such special cases should still be allowed to set the access-forbidden empty password.

taniwallach commented 5 years ago

During followup work after a discussion with @mgage I noticed that

lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm
lib/WeBWorK/ContentGenerator/CourseAdmin.pm use trim_spaces() from lib/WeBWorK/Utils.pm to remove leading and trailing white-space from several form input values, including:
add_initial_password
student_id (used as an initial password)
new_user_id
add_initial_userID so at least those manners of creating a password (and a user_id) apparently do trim leading/trailing white-space.

Adding new users and setting passwords via the ClassList Editor apparently do not trim leading/trailing white-space.

taniwallach commented 5 years ago

The following seems to be a complete list of files where the DB modules addPassword() or putPassword() function is used on input which may need to be trimmed. These are the candidates for examination and possible modification.

bin/newpassword
bin/change_user_id

lib/WebworkWebservice/CourseActions.pm
lib/WebworkSOAP.pm

lib/WeBWorK/Authen.pm

lib/WeBWorK/ContentGenerator/Instructor/AddUsers.pm
lib/WeBWorK/ContentGenerator/Instructor/UserList.pm
lib/WeBWorK/ContentGenerator/Instructor/UserList2.pm
lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm
lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList2.pm
lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail.pm
lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail2.pm

lib/WeBWorK/ContentGenerator/CourseAdmin.pm
lib/WeBWorK/ContentGenerator/Options.pm

lib/WeBWorK/Utils/CourseManagement.pm
lib/WeBWorK/Utils/DBImportExport.pm

pstaabp commented 3 years ago

Did we make a decision about this for including in ww2.16?

taniwallach commented 3 years ago

Did we make a decision about this for including in ww2.16?

I think we decided that one of us should try to make the changes to be consistent. Too much other work was going on... but we can consider this for RC2.

drgrice1 commented 3 years ago

I think this does need to make it into ww2.16. I just did a test, and discovered that students can change their passwords on the user settings page to a password that starts or ends with a space (or more), and then can not sign in with that password.

So how did we decide to make this consistent? Do we make it always trim, or always not trim?

taniwallach commented 3 years ago

So how did we decide to make this consistent? Do we make it always trim, or always not trim?

I think we decided to trim everywhere.

Pros:

@mgage pointed out that people who copy/paste passwords from somewhere into the box can easily be copying a leading or trailing space without realizing it, and the consistent two-sided trim would gets such accidents to "disappear" in the passwords being processed.
As a result, it is likely to reduce on workload when staff gets emails about a password not working in such cases.

Cons:

External authentication systems (SSO) might allow such leading/trailing whites-pace, in which case we could DoS a user who can access the other systems which don't trim.

I think we decided that the rare cases where we DoS a user who intentionally set an external password with leading or trailing whits-space can be told to change their password to avoid that. I certainly feel that this is likely to cause less total human time-cost in the long run.

pstaabp commented 3 years ago

This is being fixed in #1290

openwebwork / webwork2