Skip to content

Commit

Permalink
limit password length to 100
Browse files Browse the repository at this point in the history
  • Loading branch information
@mkj
mkj committed Mar 20, 2019
1 parent 01cd1bd commit 8b4f60a
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 1 deletion.
11 changes: 10 additions & 1 deletion svr-authpasswd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ void svr_auth_password(int valid_user) {
}

password = buf_getstring(ses.payload, &passwordlen);
if (valid_user) {
if (valid_user && passwordlen <= DROPBEAR_MAX_PASSWORD_LEN) {
/* the first bytes of passwdcrypt are the salt */
passwdcrypt = ses.authstate.pw_passwd;
testcrypt = crypt(password, passwdcrypt);
Expand All @@ -80,6 +80,15 @@ void svr_auth_password(int valid_user) {
return;
}

if (passwordlen > DROPBEAR_MAX_PASSWORD_LEN) {
dropbear_log(LOG_WARNING,
"Too-long password attempt for '%s' from %s",
ses.authstate.pw_name,
svr_ses.addrstring);
send_msg_userauth_failure(0, 1);
return;
}

if (testcrypt == NULL) {
/* crypt() with an invalid salt like "!!" */
dropbear_log(LOG_WARNING, "User account '%s' is locked",
Expand Down
2 changes: 2 additions & 0 deletions sysoptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@
/* Required for pubkey auth */
#define DROPBEAR_SIGNKEY_VERIFY ((DROPBEAR_SVR_PUBKEY_AUTH) || (DROPBEAR_CLIENT))

#define DROPBEAR_MAX_PASSWORD_LEN 100

#define SHA1_HASH_SIZE 20
#define MD5_HASH_SIZE 16
#define MAX_HASH_SIZE 64 /* sha512 */
Expand Down

7 comments on commit 8b4f60a

@fhoshino
Copy link

@fhoshino fhoshino commented on 8b4f60a Aug 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I'm having a password of 160 characters and this change is affecting me.

https://gitlab.alpinelinux.org/alpine/aports/issues/10659

@MichaIng
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just faced this as well: MichaIng/DietPi#5849
If high SSH security is wanted, key authentication makes more sense, so >100 character passwords used for SSH login are likely very rare, but it can be surprising for these rare cases when the same password works well on OpenSSH (and local login anyway).

@mkj
There is a changelog about it, but I don't really understand why this limit was introduced. Could you explain, when you find time?

@mkj
Copy link
Owner Author

@mkj mkj commented on 8b4f60a Nov 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time taken for glibc to crypt() a password is proportional to the length of the password (a bad design, in retrospect). The problem is that we only know which salt to use for a user once we lookup the user, so can't (*) run a fake crypt() if the user doesn't exist. Instead Dropbear waits for a fixed time delay. But if the password is too long, the crypt() time will be longer than the time delay, so it's possible to tell if a particular username exists.

I think what OpenSSH does is use the salt of the root user instead (*), which would probably be OK in most cases (unless the salt has changed during the system's lifetime). The other option would be to not worry about leaking the existence of users ("security through obscurity"), though I guess there might be systems where usernames are somewhat sensitive. I'll have a look how much complexity there is if it uses another salt for the crypt().

Realistically I don't think there's any point to a very large password for online logins - you are limited to a few tries per second. Offline passwords for encryption are a different matter.

@MichaIng
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification. I agree that such a long password for SSH doesn't make so much sense, given that pubkey authentication can be used instead. It's probably relevant for some cases where a long password is used to protect other login ways and where pubkey authentication isn't feasible/possible for some reason. It isn't possible to pass the "Too-long password attempt" error to the client prompt, is it?

@mkj
Copy link
Owner Author

@mkj mkj commented on 8b4f60a Nov 10, 2022 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewhodel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make the limit very small, 100 characters?

I realize it needs to be allocated and that the password is stored as a salted hash that is much shorter than the possible length proving that you aren't able to allocate the exact length.

Why don't you read the input into memory to 5000 bytes instead of 100 bytes?

Your changelog says:

- Make failure delay more consistent to avoid revealing valid usernames, set server password 
  limit of 100 characters. Problem reported by usd responsible disclosure team

If you are trying to avoid revealing valid usernames, you would know they use a single character (or the minimum) password length. You should allocate a random amount between length and 5000 to require at least 5000 providing more time for temperature to change. That makes it more difficult to guess the logins.

@andrewhodel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the issue about this - #237

Please sign in to comment.