Hi all,

after evaluating the SA-CONTRIB-2018-077 addressing only 7.x-1.x, because of 2.x branch is not covered by Drupal Security Team being unstable, I'm attaching now a patch that fixes the same flaw in that version.

Bests,
Balazs.

CommentFileSizeAuthor
#19 password_policy-8.x-3.x-add_safeguard_against_password_length_dos-3018480-19.patch887 bytesaohrvetpv
#15 interdiff.txt3.18 KBtatarbj
#15 password_policy-7.x-2.x-add_safeguards_against_password_length_dos-3018480-15.patch5.5 KBtatarbj
#14 password_policy-7.x-2.x-add_safeguards_against_password_length_dos-3018480-14.patch2.33 KBaohrvetpv
#10 forward-port-of-sa-contrib-2018-077-3018480-10.patch4.59 KBtatarbj
#8 forward-port-of-sa-contrib-2018-077-3018480-08.patch4.65 KBtatarbj
#6 forward-port-of-sa-contrib-2018-077-3018480-06.patch1.38 KBtatarbj
#4 forward-port-of-sa-contrib-2018-077-3018480-04.patch4.54 KBtatarbj
#2 forward-port-of-sa-contrib-2018-077-3018480-02.patch5.55 KBtatarbj

Comments

tatarbj created an issue. See original summary.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new forward-port-of-sa-contrib-2018-077-3018480-02.patch5.55 KB

Status: Needs review » Needs work

The last submitted patch, 2: forward-port-of-sa-contrib-2018-077-3018480-02.patch, failed testing. View results

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new forward-port-of-sa-contrib-2018-077-3018480-04.patch4.54 KB

Fixing the tests.

Status: Needs review » Needs work

The last submitted patch, 4: forward-port-of-sa-contrib-2018-077-3018480-04.patch, failed testing. View results

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new forward-port-of-sa-contrib-2018-077-3018480-06.patch1.38 KB

At least let's see the patch without the 1.x brought tests work on the 2.x branch, so here is the patch without tests.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: Unassigned » tatarbj
Status: Needs review » Needs work

Alright, code passes the current coverage, next step is to adapt the tests from the 1.x to 2.x

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Version: 7.x-2.x-dev » 7.x-2.0-alpha7
Status: Needs work » Needs review
StatusFileSize
new forward-port-of-sa-contrib-2018-077-3018480-08.patch4.65 KB

A different approach for the test coverage taking the 7.x-2.0-alpha7 as base.

Status: Needs review » Needs work

The last submitted patch, 8: forward-port-of-sa-contrib-2018-077-3018480-08.patch, failed testing. View results

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Version: 7.x-2.0-alpha7 » 7.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new forward-port-of-sa-contrib-2018-077-3018480-10.patch4.59 KB

Nop, last release is too far from dev branch, let's do it on it.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: tatarbj » Unassigned

Alright, tests are correct now, adapted to 2.x and increasing its coverage.
Under #6 is the patch that has no tests, under #10 the one that should be applied on the top of 2.x-dev.

aohrvetpv’s picture

aohrvetpv commented

tatarbj, thanks very much, I had actually already written patches for 7.x-2.x and 8.x-3.x, just hadn't committed. Your patch seems better though because it has a test, so I'll review for commit.

There is no known DoS vulnerability in 7.x-2.x/8.x-3.x, but it would be good to safeguard against the possibility of a password length DoS. Even if none of the packaged constraints are vulnerable, people can write/deploy their own custom constraints which could be vulnerable.

aohrvetpv’s picture

aohrvetpv commented

The AJAX check also needs to impose the password length limit, I think. I only have a few spare minutes now, so I'll post my patch and we can merge the two later...

aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Needs work
StatusFileSize
new password_policy-7.x-2.x-add_safeguards_against_password_length_dos-3018480-14.patch2.33 KB
tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-7.x-2.x-add_safeguards_against_password_length_dos-3018480-15.patch5.5 KB
new interdiff.txt3.18 KB

Thanks for raising this part with the AJAX check, I was not even thinking about it as just wanted to forward port the advisory to 7.x-2.x. If you agree, we could open a follow-up for bringing it even further to 8.x-3.x to separate these issues :)

Because of our patches are technically the same, yours have bigger coverage in the codebase, mine has the tests, I've taken yours then added the tests from mine (see interdiff).

Let's see how tests behave now!

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented

Alrighty, so we are good for further tests to arrive at RTBC - as I've done some on my localhost, they seem ok, but I don't want to put it to RTBC as written the patch :)

  • AohRveTPV committed 9792b4b on 7.x-2.x authored by tatarbj 
    Issue #3018480 by tatarbj, AohRveTPV: Forward port to 7.x-2.x of SA-...
aohrvetpv’s picture

aohrvetpv commented
Title: Forward port to 7.x-2.x of SA-CONTRIB-2018-077 » Forward port SA-CONTRIB-2018-077
Version: 7.x-2.x-dev » 8.x-3.x-dev
Status: Needs review » Patch (to be ported)

Thanks a lot. It's great to have the test. I will make a new alpha release of 7.x-2.x shortly.

aohrvetpv’s picture

aohrvetpv commented
Status: Patch (to be ported) » Reviewed & tested by the community
StatusFileSize
new password_policy-8.x-3.x-add_safeguard_against_password_length_dos-3018480-19.patch887 bytes

nerdstein previously reviewed this patch in the security.drupal.org issue thread. So I think we're ready to commit (assuming tests pass). It would be good to add a test but not straightforward to port from 7.x so I think we can go ahead and commit.

aohrvetpv’s picture

aohrvetpv commented

Going to hold off on new 7.x.2.x alpha release due to this bug which was just reported: #3018799: Consecutive character count validation fails when system generates the password

  • AohRveTPV committed 176dd7e on 8.x-3.x 
    Issue #3018480 by AohRveTPV: Forward port SA-CONTRIB-2018-077
aohrvetpv’s picture

aohrvetpv commented
Status: Reviewed & tested by the community » Fixed

We could add a test later for 8.x-3.x as a separate issue.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented

Hi @AohRveTPV,
I would have suggested to open a different issue to allow ppl reference this one as d7 level patch and the other one for d8 version of password policy, but as it's too late now, I'd like to leave here this message for further usage/evaluations:

The issue contains forward port of SA-CONTRIB-2018-077 for Password Policy module 7.x-2.x AND 8.x-3.x versions which are currently not stable and because of this are not covered by Drupal Security Team.
The patches are tested and included in the latest dev versions of the above mentioned branches but not yet released!

Thanks again!
Bests,
Balazs.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
commented

Hi @AohRveTPV

Can you create a new alpha release containing the security fix? The reported bug you mentioned was closed.

Regards,
Sjoerd

aohrvetpv’s picture

aohrvetpv commented

Made new 7.x-2.x alpha release. nerdstein does releases for 8.x-3.x, so leaving it to him to make a new alpha release for that branch.

Status: Fixed » Closed (fixed)

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

andras_szilagyi’s picture

andras_szilagyi commented

In the patch, why not use drupal_strlen() ?