Password length argument

[offa]

This pull request adds a new argument for the password length to generate (see #25).

• -s <LENGTH> / --length <LENGTH> (-l and -n are already used)
• A minimum length of 8 is required to prevent short and unsecure passwords (this is open for discussion)
• If there's no length given, the default value of 64 is used (this maintains the current behaviour)

[coveralls]

Coverage decreased (-2.03%) when pulling ddc965542d9a3fff606b5fd1f59b4fd2910d76d5 on offa:password_length into 917854ec22ced56c5252c4ddc82562faead3ce2f on defuse:master.

[sarciszewski]

A minimum length of 8 is required to prevent short and unsecure passwords (this is open for discussion)

Can we set the minimum to 10, at the very least? :)

https://en.wikipedia.org/wiki/Password_strength#Common_guidelines

[offa]

Can we set the minimum to 10, at the very least? :)

Of course! If there are no opposing reasons i'll change it. But let's wait for @defuse first.

[defuse]

unsigned char result[pwLength]; - let's not use C's crappy dynamic stack allocator because there's no way to tell if it fails. This should be malloced (and return value checked!) and freed at the end.

We should also be super careful about integer overflow from the command line arguments -- we don't want someone requesting a 4GB password and getting a 1-character password (the -p option has this problem as well so I'll just fix both after merging).

This introduces a side channel which leaks the lengths of passwords, but I don't think I'm worried about that.

We should have a length minimum of 1. Because there are applications where shorter than 8 is actually what you want, like PIN numbers, phone screen unlock passwords, etc. Checking against a higher minimum is another error case we don't need, and only adds complexity. The password policy should be applied on the thing that's accepting passwords, not the thing that's generating them. @sarciszewski what do you think?

[sarciszewski]

Since the default is 64, that should be fine. I just don't want to read "but how did they guess my password? I used ./passgen -s 4 it should be secure!" :)

[offa]

let's not use C's crappy dynamic stack allocator because there's no way to tell if it fails. This should be malloced (and return value checked!) and freed at the end.

Ok, but we have to overwrite the memory with 0's before freeing.

we don't want someone requesting a 4GB password and getting a 1-character password

An upper bound for password lengths should solve that. I don't think a 4 GB password will work though.

This introduces a side channel which leaks the lengths of passwords, but I don't think I'm worried about that.

Is this solved if we protect us from an overflow?

The password policy should be applied on the thing that's accepting passwords, not the thing that's generating them.

That's true, but i see it as @sarciszewski - also a password of 1 doesn't make any sence too me. But you a right, yes ... this is not our responsibility.

Changing the minimum length is not a big effort, in fact it's just chaning the value. But the test for the minium length - even if set to 1 - should be kept, just in case someone wants to use it.

---

If we could summarise the things to change:

1. Use heap memory instead of stack (incl. free / bzero)
2. Further checks of the length arg
3. Setting min. length to 1

I don't think it's a good idea to merge my branch into master at the current state since those changes are not "stable" yet. But we can either use my branch for development or merge it into a separate dev branch.

If the changes above are ok i'll start implementing.

[defuse]

Checking the length won't solve the side channel. The side channel is: The loop only goes N times, where N is the length of the password, so by measuring the number of loop iterations, you have the length of the password. Before it was always 64 so no information was leaked.

I'm actually not sure about checking the length, since the overflow happens in sscanf, so how do we distinguish between "1" and "4294967297" (which is 1 after being parsed)? I'd say leave that fix out of this branch.

And yes, the memory has to be zeroed before freed (use memset_s)

[defuse]

Thanks, by the way :-) :rocket:

[offa]

Main changes pushed.

[offa]

I'm actually not sure about checking the length, since the overflow happens in sscanf, so how do we distinguish between "1" and "4294967297" (which is 1 after being parsed)? I'd say leave that fix out of this branch.

There are better ways than scanf() - but this should enter a new branch.

[coveralls]

Coverage decreased (-2.48%) when pulling 761ddb1a141e829a04eb1816577e2e99877c3f36 on offa:password_length into 917854ec22ced56c5252c4ddc82562faead3ce2f on defuse:master.