Problem/Motivation

Would be a good check to compare a users username to the password making sure they don't equal each other

Steps to reproduce

NA

Proposed resolution

Compare username to the password to see if they equal

Remaining tasks

Review

User interface changes

NA

API changes

NA

Data model changes

NA

Original post

We could easily include a few weak password tests like:

1. Commonly used password lists for the top 100, or 1,000 most commonly used passwords.
2. Whether it's the same as their username :(

CommentFileSizeAuthor
#30 830970-30.patch3.9 KBsmustgrave
#28 830970-28.patch3.86 KBsmustgrave
#26 interdiff-20-26.txt778 bytessmustgrave
#26 830970-26.patch3.6 KBsmustgrave
#24 interdiff_20-24.txt951 bytesasad_ahmed
#24 2888699-24.patch2.96 KBasad_ahmed
#20 security_review-name_password_check-830970-20-8.patch3.59 KBvisabhishek
#17 security_review-name_password_check-830970-17-8.patch3.73 KBtlyngej
#15 security_review-name_password_check-830970-14-8.patch3.59 KBtlyngej
#13 security_review-name_password_check-830970-13-8.patch0 bytestlyngej
#10 security_review-name-passwords-830970-10.patch3.73 KBdsnopek
#8 830970-security-review-passwords_8.patch5.31 KBcoltrane
#7 830970-security-review-passwords_7.patch5.39 KBcoltrane
#4 830970-security-review-passwords_4.patch5.35 KBgreggles
#3 830970-security-review-passwords.patch4.38 KBcoltrane

Issue fork security_review-830970

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Ideally you could combine this with the list of "trusted roles" to find users with bad passwords and advanced permissions.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Here's a query to do that:

select u.uid, count(rid) from users u left join users_roles ur on u.uid = ur.uid and ur.rid in (2, 3, 4) where pass = md5(name) group by uid;

It gives you results like:

+-----+------------+
| uid | count(rid) |
+-----+------------+
|  11 |          1 | 
|  12 |          0 | 
|  13 |          1 | 
|  14 |          2 | 
|  15 |          1 | 
+-----+------------+

So, if it's a count of zero you know that the user doesn't have one of the advanced roles.

coltrane’s picture

coltrane
he/him
commented
Status: Active » Needs work
StatusFileSize
new 830970-security-review-passwords.patch4.38 KB

Attached patch implements greggles' check for users with certain roles whose passwords are their username. It needs work because it needs more code comments, documentation, and expanded integration with forthcoming rainbow table check.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 830970-security-review-passwords_4.patch5.35 KB

This all looks awesome to me.

One other thought I had is that theoretically it's possible for UID to not have any roles, still have full permissions of course, and have a weak password. I think uid 1 should be special cased in the processing here.

I also fixed a variable typo and a typo in the message for IP's with lots of failed logins.

jleinenbach’s picture

jleinenbach commented

As an idea, bad passwords could be tested by checking against a cracked hashes list, downloadable e. g. here:
http://opencrack.hashkiller.com/

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: test password strength against a few commonly known weak passwords » test password strength by comparing the password to the username

@jleinbach, I generally like that idea, but we want to keep the security_review module download relatively small. My current thinking is to use a really small list of popular passwords and provide a way for sites to use an alternate set of passwords for a more complete check.

This particular issue has kind of become about checking username against username-as-password. I think we should change the UI so it is really about just that and create a new issue about how to do checking of commonly used passwords: #897960: test password strength against an extensible list of passwords that defaults to really common ones.

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 830970-security-review-passwords_7.patch5.39 KB

Small reroll, I only added some comments.

coltrane’s picture

coltrane
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new 830970-security-review-passwords_8.patch5.31 KB

Update for new check help and modified UI to only talk about usernames as passwords.

coltrane’s picture

coltrane
he/him
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

Committed http://drupalcode.org/project/security_review.git/commit/0b90323 for 6.x-1.x

Port to Drupal 7 will require hashing in PHP but it's still a valuable check.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes
Status: Patch (to be ported) » Needs review
StatusFileSize
new security_review-name-passwords-830970-10.patch3.73 KB

Attached is a patch that ports this check to Drupal 7! A bunch of the D6 code was present in the D7 branch, so rather than adding it new, it modifies what's there.

  • dsnopek committed ef2b680 on 
    Issue #830970 by coltrane, greggles, dsnopek: test password strength by...
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

Committed patch to 7.x-1.x! Needs to be ported to Drupal 8.

tlyngej’s picture

tlyngej commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new security_review-name_password_check-830970-13-8.patch0 bytes

I have ported this patch to version 8.1.

Basically just added a new Check extending class (Drupal\security_review\Checks\NamePasswords) and amended the security_review_security_review_checks() function in security_review.module.

Status: Needs review » Needs work

The last submitted patch, 13: security_review-name_password_check-830970-13-8.patch, failed testing.

tlyngej’s picture

tlyngej commented
Status: Needs work » Needs review
StatusFileSize
new security_review-name_password_check-830970-14-8.patch3.59 KB

And, hey, why not submit a patch with content!

tlyngej’s picture

tlyngej commented
Assigned: Unassigned » tlyngej
tlyngej’s picture

tlyngej commented
StatusFileSize
new security_review-name_password_check-830970-17-8.patch3.73 KB

New patch, where the displayed list of users are properly escaped.

tlyngej’s picture

tlyngej commented
Assigned: tlyngej » Unassigned
vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Re-roll is needed.

visabhishek’s picture

visabhishek commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new security_review-name_password_check-830970-20-8.patch3.59 KB

Just re-rolled the patch security_review-name_password_check-830970-17-8.patch

smustgrave’s picture

smustgrave commented

As much as I like this idea is that out of the realm for this module? Shouldn't that fall under https://www.drupal.org/project/password_policy

Will let one of the maintainers to decide but not sure if it should be added.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

coltrane, dsnopek and I are all maintainers who did work on this to bring it to the module.

I think password_policy should keep a site safe for new password changes, but this module gives a sense of whether a site has a problem regardless of when the problem was introduced.

If password_policy has a feature like this I would definitely be willing to not add this to Drupal 8.

I also don't see it as a top priority. If you aren't motivated by the idea you don't have to work on it :)

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Since the work has already been done don't see any harm. Just needs to be updated.

getUsername() has been deprecated and removed.

asad_ahmed’s picture

asad_ahmed commented
Status: Needs work » Needs review
StatusFileSize
new 2888699-24.patch2.96 KB
new interdiff_20-24.txt951 bytes

Made changes as per #23, replaced deprecated getUsername() with getDisplayname(). Thanks

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Patch #24 removes code from #20 for the module file.

Also when viewing the details this error is thrown Warning: foreach() argument must be of type array|object, string given in template_preprocess_item_list() (line 1147 of core/includes/theme.inc).

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
StatusFileSize
new 830970-26.patch3.6 KB
new interdiff-20-26.txt778 bytes

Reverted back to #20. Fixed the getUsername issue and error reported in #25

smustgrave’s picture

smustgrave commented
Version: 8.x-1.x-dev » 2.0.x-dev
smustgrave’s picture

smustgrave commented
StatusFileSize
new 830970-28.patch3.86 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 28: 830970-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
StatusFileSize
new 830970-30.patch3.9 KB
smustgrave’s picture

smustgrave commented
smustgrave’s picture

smustgrave commented
Issue summary: View changes

ednark made their first commit to this issue’s fork.

ednark’s picture

ednark commented
Status: Needs review » Reviewed & tested by the community

I have tested this and it functions as expected without errors.

Tested on 10.1.x-dev with a standard install
module drupal/security_review:2.0.x-dev@dev
branch 830970-test-password-strength
patch applied https://www.drupal.org/files/issues/2023-01-13/830970-30.patch

Test procedure:
Create new User
Set Username to TEST and password to TEST
rerun security_review tests
see failure for "Username same as password"
goto details and see the TEST user listed as the problem

smustgrave opened merge request !33

  • smustgrave committed de902d59 on 2.0.x 
    Issue #830970: test password strength by comparing the password to the...
smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Fixed
vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.