Bug 25622

Summary: Change way MySQL password is generated by koha-create
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Installation and upgrade (command-line installer)Assignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Victor Grousset/tuxayo <victor>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, aleisha, arthur.suzuki, fridolin.somers, jonathan.druart, julian.maurice, katrin.fischer, kyle, lucas, martin.renvoize, nick, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.04, 21.11.11
Bug Depends on: 23250    
Bug Blocks:    
Attachments: Bug 25622: Use special chars in DB password (koha-create)
Bug 25622: Use special chars in DB password (koha-create)
Bug 25622: use -r paramter to blacklist some special chars
Bug 25622: Use special chars in DB password (koha-create)
Bug 25622: Use special chars in DB password (koha-create)

Description Jonathan Druart 2020-05-29 14:10:44 UTC
On bug 23250 we decided to generate a password without special chars then add a '@' at the end to comply with MySQL policy.
That is wrong, we should handle correctly the special chars we don't want to be part of the password.
Comment 1 Jonathan Druart 2020-05-29 14:16:18 UTC
Created attachment 105436 [details] [review]
Bug 25622: Use special chars in DB password (koha-create)

On bug 23250 we decided to generate a password without special chars then add a
'@' at the end to comply with MySQL policy.
That is wrong, we should handle correctly the special chars we don't want to be
part of the password.

Test plan:
1.
mysqlpwd="$(pwgen -s -y 32 1)"
echo $mysqlpwd
mysqlpwd="$(echo $mysqlpwd | tr -d :\'\&\\\\\<\>\/ | cut -c1-16)"
echo $mysqlpwd

Confirm that in the second line you don't see one of the following chars : ' & \ < > /

2.
Copy from src and edit /usr/sbin/koha-create to add an echo $mysqlpwd
Create several instances, like:
koha-create --create-db x
koha-create --create-db xx
koha-create --create-db xxx
...

When you see a password with a special chars, do:
koha-shell xxx
grep '<pass>' $KOHA_CONF
And make sure the password does not contain "__DB_PASS__"
Comment 2 Bernardo Gonzalez Kriegel 2020-06-02 20:53:31 UTC
Created attachment 105504 [details] [review]
Bug 25622: Use special chars in DB password (koha-create)

On bug 23250 we decided to generate a password without special chars then add a
'@' at the end to comply with MySQL policy.
That is wrong, we should handle correctly the special chars we don't want to be
part of the password.

Test plan:
1.
mysqlpwd="$(pwgen -s -y 32 1)"
echo $mysqlpwd
mysqlpwd="$(echo $mysqlpwd | tr -d :\'\&\\\\\<\>\/ | cut -c1-16)"
echo $mysqlpwd

Confirm that in the second line you don't see one of the following chars : ' & \ < > /

2.
Copy from src and edit /usr/sbin/koha-create to add an echo $mysqlpwd
Create several instances, like:
koha-create --create-db x
koha-create --create-db xx
koha-create --create-db xxx
...

When you see a password with a special chars, do:
koha-shell xxx
grep '<pass>' $KOHA_CONF
And make sure the password does not contain "__DB_PASS__"

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 3 Julian Maurice 2020-06-05 13:43:10 UTC
pwgen has a --remove-chars option that seems to do exactly what you want to do with the tr command.

Try this:
    pwgen -s -y -r ":'&\\<>/" 16 1
Comment 4 Jonathan Druart 2020-06-08 09:47:35 UTC
Created attachment 105626 [details] [review]
Bug 25622: use -r paramter to blacklist some special chars
Comment 5 Jonathan Druart 2020-06-08 09:48:05 UTC
(In reply to Julian Maurice from comment #3)
> pwgen has a --remove-chars option that seems to do exactly what you want to
> do with the tr command.
> 
> Try this:
>     pwgen -s -y -r ":'&\\<>/" 16 1

Thanks Julian, that's definitely the way to go!
Comment 6 Julian Maurice 2020-06-12 09:05:59 UTC
Created attachment 105779 [details] [review]
Bug 25622: Use special chars in DB password (koha-create)

On bug 23250 we decided to generate a password without special chars
then add a '@' at the end to comply with MySQL policy.
That is wrong, we should handle correctly the special chars we don't
want to be part of the password.

Test plan:
1.
mysqlpwd=$(pwgen -s -y -r ":'&\\<>/" 16 1)
echo $mysqlpwd

Confirm that you don't see one of the following chars : ' & \ < > /

2.
Copy from src and edit /usr/sbin/koha-create to add an echo $mysqlpwd
Create several instances, like:
koha-create --create-db x
koha-create --create-db xx
koha-create --create-db xxx
...

When you see a password with a special chars, do:
koha-shell xxx
grep '<pass>' $KOHA_CONF
And make sure the password does not contain "__DB_PASS__"

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 7 Julian Maurice 2020-06-12 09:07:27 UTC
I squashed the patches, and removed the comment about the previous method (pwgen 32 | tr | cut)
Comment 8 Aleisha Amohia 2020-08-17 23:17:24 UTC
Hey team, this doesn't work for me.

-r is not a valid option for pwgen on stretch, jessie or Ubuntu 16.04.
Comment 9 Jonathan Druart 2020-08-18 08:35:47 UTC
pwgen (2.08-1)
  * Add new option --remove-chars which removes chars from the set of
    characters used to generate passwords (Closes: #836334)

Buster has it, Stretch has 2.07-1

Good catch Aleisha!

We are postponing this one.
Comment 10 Tomás Cohen Arazi 2022-06-15 14:24:05 UTC
We don't care about Stretch anymore. Can we give it a new QA round please?
Comment 11 Victor Grousset/tuxayo 2022-06-18 02:49:43 UTC
> Insecure MySQL password generated by koha-create

How is that insecure currently? It's 89 bit strong.
python: import math ; math.log((26+26+10)**15, 2)
It's totally impractical for online bruteforcing (even from localhost). And for offline attack on the file where the password is stored, assuming it's hashed (otherwise the complexity of the password doesn't matter) it's totally impractical also.
Digits + lowercase + uppercase on 15 chars (length matters the most) is super strong.

Assuming the special symbol pool is 20 (it looks less than that), we go to 95 bits.
And we go from 15 chars to 16. Which ups from 95 to 102 bits. We see that +1 length had at least the same impact as using specials chars.
Changing only the length from 15 to 20 gives us 119 bit strength.

I can still QA that[1] because it doesn't hurt.
Even though it's overkill and doesn't change much the strength. It's an example of how password strength is unintuitive.

Here is an online calculator, it's in french but guessing the meaning should be doable (Longueur = length)
https://www.ssi.gouv.fr/administration/precautions-elementaires/calculer-la-force-dun-mot-de-passe/
It's the national IT security agency, on the same page they recommend 82 bits for secure passwords. And currently we are 128 times (7 bits) above that.

[1] Available versions are good, no issue in sight https://repology.org/project/pwgen/versions
Comment 12 Jonathan Druart 2022-06-20 07:09:24 UTC
(In reply to Victor Grousset/tuxayo from comment #11)

Do you think adding the same special character at the end of a generated password is the way to go? We want to generate a 16 characters length password, this is what this patch is doing.
If you think the bug title is misleading, feel free to adjust it.
Comment 13 Victor Grousset/tuxayo 2022-06-26 20:22:52 UTC
(In reply to Jonathan Druart from comment #12)
> Do you think adding the same special character at the end of a generated
> password is the way to go? 

That's a valid way to deal with mandatory rules such as the one of mysql.

> We want to generate a 16 characters length
> password, this is what this patch is doing.

Indeed it's also a valid to do it now that we have the right pwgen version in the relevant versions of distros.

> If you think the bug title is misleading, feel free to adjust it.

Let's move on with that. I'm not much inspired but at least that removes the idea that it's a needed security change.
Comment 14 Victor Grousset/tuxayo 2022-06-26 20:24:30 UTC
Created attachment 136559 [details] [review]
Bug 25622: Use special chars in DB password (koha-create)

On bug 23250 we decided to generate a password without special chars
then add a '@' at the end to comply with MySQL policy.
That is wrong, we should handle correctly the special chars we don't
want to be part of the password.

Test plan:
1.
mysqlpwd=$(pwgen -s -y -r ":'&\\<>/" 16 1)
echo $mysqlpwd

Confirm that you don't see one of the following chars : ' & \ < > /

2.
Copy from src and edit /usr/sbin/koha-create to add an echo $mysqlpwd
Create several instances, like:
koha-create --create-db x
koha-create --create-db xx
koha-create --create-db xxx
...

When you see a password with a special chars, do:
koha-shell xxx
grep '<pass>' $KOHA_CONF
And make sure the password does not contain "__DB_PASS__"

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Victor Grousset/tuxayo 2022-06-26 20:32:03 UTC
Code looks good, passing QA :)

This should be moved out of the security project.
There is no actual security concern (comment 11) but we didn't notice it earlier. This doesn't make any harm and replaces a hardcoded "@" concatenation in the generated password which could look like a hacky workaround. So let's go with this.

Not worth backporting IMHO. And certainly not to 21.05 since Ubuntu 16.04 is supported by it.
Comment 16 Tomás Cohen Arazi 2022-07-01 19:58:58 UTC
Is this a security bug or just a security enhancement?
Comment 17 Martin Renvoize 2022-07-01 20:21:19 UTC
I would say it's an enhancement really myself
Comment 18 Tomás Cohen Arazi 2022-07-05 12:23:17 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 19 Lucas Gass 2022-08-09 22:06:25 UTC
Backported to 22.05.x for 22.05.04
Comment 20 Arthur Suzuki 2022-08-12 07:30:54 UTC
pushed to 21.11.x for 21.11.11
Comment 21 Victor Grousset/tuxayo 2022-08-17 19:47:20 UTC
Not backported to oldoldstable (21.05.x). See comment 15

Nothing to document, marking resolved.