Password complexity validation with ReDOS vulnerability #31

micapam · 2018-04-05T07:34:58Z

As I mentioned (in #24), the devise-security gem currently is likely (depending on what password_regex is used) to open up a vulnerability to ReDOS attacks.

Fixing this cannot be done without changing the means by which password complexity is set - it's the fact that a single regex is used that's the problem. This unavoidably means a breaking change.

I have implemented a complexity validator using what I believe is the most commonly required POSIX character sets (upper-case letters, lower-case letters, numerals, and punctuation marks/symbols). Client code can specify how many of each they want, or leave out any sets they don't want.

I also added a French translation, which is not great as French is not my native language, but I figured as I knew more French than German or Spanish I may as well add that in too while I was about it :)

coveralls · 2018-04-05T07:48:41Z

Pull Request Test Coverage Report for Build 105

61 of 61 (100.0%) changed or added relevant lines in 5 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.6%) to 90.191%

Files with Coverage Reduction	New Missed Lines	%
test/test_password_expired_controller.rb	1	100.0%
lib/devise-security/rails.rb	2	100.0%
test/test_security_question_controller.rb	3	100.0%

Totals
Change from base Build 101:	0.6%
Covered Lines:	754
Relevant Lines:	836

💛 - Coveralls

olbrich

Thanks for your PR. Here are some initial thoughts.

olbrich · 2018-04-05T13:23:07Z

README.md

@@ -62,7 +62,7 @@ Devise.setup do |config|
  # config.expire_password_after = 3.months

  # Need 1 char of A-Z, a-z and 0-9
-  # config.password_regex = /(?=.*\d)(?=.*[a-z])(?=.*[A-Z])/
+  # config.password_complexity = { digit: 1, lower: 1, upper: 1 }


perhaps a default for punct as well, also it would be good to explain that these values represent counts of the POSIX character classes. For the punct one you would also want to include an example of which characters match that.

In general, I would use configuration keys that are semantically meaningful to users, not developers. The error messages you generate will be displayed to end users and need to be something they are going to be able to comprehend. Your average user won't understand why + or < are not considered to be punctuation.

olbrich · 2018-04-05T14:06:43Z

lib/devise-security/validators/complexity_validator.rb

+  # Options:
+  # - digit: minimum number of digits in the validated string
+  # - lower: minimum number of lower-case letters in the validated string
+  # - punct: minimum number of punctuation characters or symbols in the validated string


the punct POSIX class misses some characters that most people consider symbols.
In general, it might be more flexible to simply use the Character Properties feature of regexes to do the counts.

With this approach the regular expressions you would want to use are:

digit: /\p{Digit}/ lower: /\p{Lower}/ upper: /\p{Upper}/ symbol: /\p{Punct}|\p{S}/

In general, I think these regexes will be more liberal than the posix classes and will probably handle international characters (like ß better).

On a related note, this approach does nothing to protect against sets of repeated characters or sequences.

Agree on the character classes, that makes sense.

Not sure what you mean about 'sets of repeated characters or sequences', though? Each regex is for a single character so I'm not sure how repeated characters or sequences in the input would cause a problem?

Another approach would be simply to iterate over each character in the string in turn, bumping the count for any character set it meets (using unpack('U') or similar) and do away with regexen altogether. Thoughts?

Some recommendations for password complexity suggest that you should avoid passwords with long sets of repeated characters (i.e. aaaaaa) and sequential characters (abcdefg or 1234). This is outside the scope of this specific PR as the original code didn't do that either.

Using unpack('U') is an interesting approach.

Oh I see. When you said 'protect against', I thought you were talking about protecting against ReDOS attacks (which, incidentally, can use repetition, alternation, etc).

Just about to head home for the weekend, but I should be able to update this PR on Monday (Australia time - so that's Sunday eve from your perspective :)

And yes, agreed repeated characters would be useful to protect against (but out of scope for this PR, which is just to use a safer method than asking developers to write a regex). Another common request is avoidance of common dictionary words.

In fact, I don't really think much of the usual complexity rules; I agree with Jeff Atwood that password rules are bullsh**. (And indeed Bill Burr.) I'd much rather use a validator like this one:
https://github.com/cmer/nobspw

However, our clients in their wisdom want the usual kind of complexity rules, based on outdated standards...and they're paying the bills 😂

Agreed. The goal here is to do a good job providing the service, even if the wisdom of this approach is now in question.

micapam · 2018-04-09T02:25:34Z

@olbrich, I think I've addressed your feedback regarding character classes.

Also, as I was adjusting the tests, I found that the setup block in test/test_secure_validatable.rb did absolutely nothing. The reason is that validators accept their rules when the model is first loaded; they don't look up the config again when it's time to validate. (Incidentally, this is also true for the current master branch, too - the setup block does nothing. Try changing the value for password_regex in there - should break the tests, but doesn't.)

I considered making the validator accept a Proc, so that configs could be changed at runtime. However, this felt like deforming production code to make testing easier, which wouldn't be good. So instead, I added a unit test for the validator itself, with some dummy models to test various scenarios. See test/test_complexity_validator.rb.

…er classes and thus fix ReDOS vulnerability

olbrich

new tests should run and pass

olbrich · 2018-04-14T16:54:24Z

test/test_complexity_validator.rb

+end
+
+class EmailValidatorTest < Minitest::Test
+  def with_no_rules_anything_goes


These tests don't execute. They need to start with test_ or use the test 'description' do.. block style (my preference, actually).

olbrich · 2018-04-14T17:20:25Z

test/test_complexity_validator.rb

+  validates :complex_string, 'devise_security/complexity': { lower: 1, upper: 1, digit: 1, symbol: 1 }
+end
+
+class EmailValidatorTest < Minitest::Test


This isn't validating emails, should be named PasswordComplexityValidatorTest

olbrich · 2018-04-14T17:31:30Z

test/test_complexity_validator.rb

+    assert(NoRules.new('aaaa').valid?)
+  end
+
+  def enforces_uppercase


There is another pattern here that may be a bit more explicit.

class PasswordComplexityValidatorTest class Password include ActiveModel::Validations attr_reader :password def initialize(password) @password = password end def setup Password.clear_validators! end def test_enforces_uppercase Password.validates :password, 'devise_security/complexity': { upper: 1 } refute(Password.new('aaaa').valid?) assert(Password.new('Aaaa').valid?) end ...

olbrich · 2018-04-14T17:34:53Z

lib/devise-security/validators/complexity_validator.rb

+  # - upper:  minimum number of upper-case letters in the validated string
+  class ComplexityValidator < ActiveModel::EachValidator
+    PATTERNS = {
+      digit: /\p{Digit}/,


I think it's likely that some people will try to use digits: 2 or symbols: 2 in the configuration, which is silently fail to do the right thing. Consider either making them aliases, or checking the keys to make sure they match the set that can be used.

olbrich · 2018-04-14T17:39:30Z

test/test_secure_validatable.rb

    user = User.create email: 'bob@microsoft.com', password: 'PASSWORD1', password_confirmation: 'PASSWORD1'
    assert_equal(false, user.valid?)
    assert_equal([msg], user.errors.full_messages)
    assert_raises(ActiveRecord::RecordInvalid) { user.save! }
  end

  test 'password must have number' do
-    msg = 'Password must contain big, small letters and digits'
+    msg = 'Password must contain at least one numeral'


replace numeral with digit for consistency.

olbrich · 2018-04-14T17:41:27Z

lib/devise-security/validators/complexity_validator.rb

+  # - lower:  minimum number of lower-case letters in the validated string
+  # - symbol: minimum number of punctuation characters or symbols in the validated string
+  # - upper:  minimum number of upper-case letters in the validated string
+  class ComplexityValidator < ActiveModel::EachValidator


name this PasswordComplexityValidator. You'll also need to rename all the associated tests/files.

olbrich · 2018-04-14T17:42:19Z

👍 I like the approach of testing the validator independently.

micapam · 2018-04-26T04:32:00Z

I think I have addressed all second-round feedback..?

…s-vulnerability

dillonwelch

IMO fine to ignore Replace class var @@password_complexity with a class instance var. but the rest of the CC/Rubocop warnings should be cleaned up.

dillonwelch · 2018-04-26T07:09:42Z

lib/devise-security/models/secure_validatable.rb

@@ -48,7 +49,7 @@ def self.included(base)

          # extra validations
          validates :email, email: email_validation if email_validation # use rails_email_validator or similar
-          validates :password, format: { with: password_regex, message: :password_format }, if: :password_required?
+          validates :password, 'devise_security/password_complexity': password_complexity, if: :password_required?


Can you fix this Rubocop warning while you're in here?

Line length? Sure, done, but now CodeClimate is complaining there are too many LoC! 😂

dillonwelch · 2018-04-26T07:10:06Z

lib/devise-security/validators/password_complexity_validator.rb

@@ -0,0 +1,29 @@
+module DeviseSecurity


This file has a bunch of Rubocop warnings.

dillonwelch · 2018-04-26T07:12:59Z

lib/devise-security/validators/password_complexity_validator.rb

+    }.freeze
+
+    def validate_each(record, attribute, value)
+      (options.keys & PATTERNS.keys).each do |key|


Might be able to pull options.keys & PATTERNS.keys into a separate method to help with complexity.

Yep, that worked.

…ty' of github.com:COzero/devise-security into feature/complexity-validation-without-redos-vulnerability

olbrich reviewed Apr 5, 2018

View reviewed changes

Josh Mostafa added 2 commits April 9, 2018 12:27

Replace password regex config with complexity method based on charact…

e2f7f8f

…er classes and thus fix ReDOS vulnerability

Review feedback and add unit test for validator

7aa5b3c

olbrich requested changes Apr 14, 2018

View reviewed changes

olbrich reviewed Apr 14, 2018

View reviewed changes

olbrich added this to the Next Release milestone Apr 16, 2018

olbrich added the ✨ enhancement label Apr 16, 2018

PR feedback

18fccf7

Merge branch 'master' into feature/complexity-validation-without-redo…

b273b3a

…s-vulnerability

olbrich requested a review from natebird April 26, 2018 05:19

dillonwelch requested changes Apr 26, 2018

View reviewed changes

Josh Mostafa added 2 commits April 27, 2018 10:35

rubocop

80baa7d

Merge branch 'feature/complexity-validation-without-redos-vulnerabili…

0a116f6

…ty' of github.com:COzero/devise-security into feature/complexity-validation-without-redos-vulnerability

dillonwelch approved these changes Apr 27, 2018

View reviewed changes

natebird approved these changes Apr 27, 2018

View reviewed changes

olbrich approved these changes Apr 27, 2018

View reviewed changes

natebird merged commit 66d7031 into devise-security:master Apr 27, 2018

natebird mentioned this pull request Apr 27, 2018

Added Turkish locale #38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Password complexity validation with ReDOS vulnerability #31

Password complexity validation with ReDOS vulnerability #31

micapam commented Apr 5, 2018

coveralls commented Apr 5, 2018 •

edited

olbrich left a comment

olbrich Apr 5, 2018

olbrich Apr 5, 2018

olbrich Apr 5, 2018

micapam Apr 5, 2018

olbrich Apr 6, 2018

olbrich Apr 6, 2018

micapam Apr 6, 2018 •

edited

micapam Apr 6, 2018

micapam Apr 6, 2018

olbrich Apr 6, 2018

micapam commented Apr 9, 2018

olbrich left a comment

olbrich Apr 14, 2018 •

edited

olbrich Apr 14, 2018

olbrich Apr 14, 2018 •

edited

olbrich Apr 14, 2018

olbrich Apr 14, 2018

olbrich Apr 14, 2018

olbrich commented Apr 14, 2018

micapam commented Apr 26, 2018

dillonwelch left a comment

dillonwelch Apr 26, 2018

micapam Apr 27, 2018

dillonwelch Apr 26, 2018

micapam Apr 27, 2018

dillonwelch Apr 26, 2018

micapam Apr 27, 2018

Password complexity validation with ReDOS vulnerability #31

Password complexity validation with ReDOS vulnerability #31

Conversation

micapam commented Apr 5, 2018

coveralls commented Apr 5, 2018 • edited

Pull Request Test Coverage Report for Build 105

💛 - Coveralls

olbrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micapam Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micapam commented Apr 9, 2018

olbrich left a comment

Choose a reason for hiding this comment

olbrich Apr 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olbrich Apr 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olbrich commented Apr 14, 2018

micapam commented Apr 26, 2018

dillonwelch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 5, 2018 •

edited

micapam Apr 6, 2018 •

edited

olbrich Apr 14, 2018 •

edited

olbrich Apr 14, 2018 •

edited