Bug 10177 - Koha Accepts Seven "Space" Characters as a Valid Password
Summary: Koha Accepts Seven "Space" Characters as a Valid Password
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Galen Charlton
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-02 20:01 UTC by Greg
Modified: 2017-06-14 22:11 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10177 - whitespace is a valid password causing pernicious perturbations. (1.33 KB, patch)
2015-01-14 22:46 UTC, Brandon
Details | Diff | Splinter Review
Bug 10177 - Whitespace is accepted within the password but leading and trailing spaces are not. (1.61 KB, patch)
2015-01-15 22:51 UTC, Brandon
Details | Diff | Splinter Review
[Signede-off] Bug 10177 - whitespace is a valid password causing pernicious perturbations. (1.52 KB, patch)
2015-01-18 08:48 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 10177 - Whitespace is accepted within the password but leading and trailing spaces are not. (1.82 KB, patch)
2015-01-18 08:50 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 10177 - whitespace is a valid password causing pernicious perturbations. (1.57 KB, patch)
2015-01-19 06:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10177 - Whitespace is accepted within the password but leading and trailing spaces are not. (1.87 KB, patch)
2015-01-19 06:18 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Greg 2013-05-02 20:01:37 UTC
Koha 3.10.03 and presumably earlier versions accept seven "space" characters as a valid password.

Concerns:
   1. Some third party software using SIP to check the Koha database to authenticate users may interpret all blank characters, or even a single terminal blank character, as an invalid password.
   2. gmcharlt suggests that patches submitted in the future may strip trailing whitespace characters in form input, causing pernicious perturbations.
Comment 1 Martin Renvoize 2014-12-31 06:59:48 UTC
Verifying this is still an issue in current master
Comment 2 Marc Véron 2015-01-02 14:44:42 UTC
This bug is similar to Bug 13441 and should be fixed the same way as Bug 13441 (prevent new passwords with whitespace characters, but do not touch functionalliy for existing passwords that already could contain spaces characters).
Comment 3 Marc Véron 2015-01-05 16:47:47 UTC
Maybe something for the Catalyst academy (starting today)
Comment 4 Brandon 2015-01-14 22:46:43 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-01-15 09:24:18 UTC
I did not test, but I compared with Bug 13441 and I think this patch should behave the same way resulting in a message like "The password entered contains whitespace characters. Please remove any whitespace characters."

With other words: There should be no whitespace characters accepted at all (or at least no leading and/or trailing whitespace characters).

Test in Bug 13441 is:
match(/\s/)

Test in this patch is:
match(/^\s*$/)
Comment 6 Chris Cormack 2015-01-15 09:26:56 UTC
(In reply to Marc Véron from comment #5)
> I did not test, but I compared with Bug 13441 and I think this patch should
> behave the same way resulting in a message like "The password entered
> contains whitespace characters. Please remove any whitespace characters."
> 
> With other words: There should be no whitespace characters accepted at all
> (or at least no leading and/or trailing whitespace characters).
> 
> Test in Bug 13441 is:
> match(/\s/)
> 
> Test in this patch is:
> match(/^\s*$/)

I disagree, whitespace in passwords is totally valid, a strong password is a phrase.

However having a password that is entirely whitespace (which is what we are testing) is not a strong password.

This patch will allow you to have whitespace in your password, but you must have other characters also.
Comment 7 Marc Véron 2015-01-15 09:33:49 UTC
Hi Chris,

I agree with whitespace inside of passwords is OK.

What about leading / trailing whitespace? See comment #1:
---
2. gmcharlt suggests that patches submitted in the future may strip trailing whitespace characters in form input, causing pernicious perturbations.
---
Comment 8 Martin Renvoize 2015-01-15 09:47:27 UTC
I'de vote toward allowing whitespace within password, but not allowing it to be leading or trailing..  that allows for strong passphrases without giving ourselves headaches in the future.
Comment 9 Marc Véron 2015-01-15 11:11:13 UTC
I agree with Martin.
Comment 10 Brandon 2015-01-15 22:51:20 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2015-01-18 08:26:12 UTC
Patch #35320 does not apply, it seems that it needs obsoleted #35265 first
Comment 12 Marc Véron 2015-01-18 08:48:30 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2015-01-18 08:50:57 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2015-01-19 06:17:50 UTC
Created attachment 35356 [details] [review]
[PASSED QA] Bug 10177 - whitespace is a valid password causing pernicious perturbations.

Test plan:
Go to your patron, go to change username and/or password. change
password to whitespace, notice it is accepted, apply this patch,
change password again, to whitespace, notice password is not valid.

Followed test plan. Patch behaves as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2015-01-19 06:18:06 UTC
Created attachment 35357 [details] [review]
[PASSED QA] Bug 10177 - Whitespace is accepted within the password but leading and trailing spaces are not.

Test plan:
1: Go to your patron
2: Go to change username and/or password.
3: Change password to something with spaces in the middle. Notice it is invalid.
4: Apply this patch.
5: Change password again using spaces in the middle. Notice it is accepted.
6: Change password with leading and/or trailing spaces, notice they are not accepted.

Applied bothe patches. Followed test plan. Patch behaves as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2015-01-19 06:19:36 UTC
Hi Brandon, thx for your patch, works great!

I suggest to leave this bug open after pushing the patch as there are 2 more pages where you can chane the password: the patron edit screen and in the OPAC. I think adding the check to the OPAC too would be good.
Comment 17 Martin Renvoize 2015-01-19 07:47:30 UTC
Pushing this to the authentication module maintainers branch.

https://github.com/mrenvoize/Koha/tree/authentication

Cheers Brandon :)
Comment 18 Tomás Cohen Arazi 2015-02-09 20:19:29 UTC
Patches pushed to master.

Thanks Brandon!
Comment 19 Katrin Fischer 2015-05-25 10:27:55 UTC
This change is pushed to master, but only covers part of the problem - you can still set your password to whitespace in the OPAC and from the full patron edit screen without any error.
Comment 20 Liz Rea 2015-11-02 21:00:17 UTC
Pushed to 3.18.x is in 3.18.12