Add zxcvbn for checking password entropy

studentinsights · Mar 6, 2019 · 357b404 · 357b404
1 parent c94153a
commit 357b404
Show file tree

Hide file tree

Showing 13 changed files with 227 additions and 3 deletions.
diff --git a/Gemfile b/Gemfile
@@ -41,8 +41,10 @@ gem 'uglifier', '>= 1.3.0'
 gem 'wicked_pdf'
 gem 'wkhtmltopdf-binary'
 gem 'rubyzip', '~> 1.2.2'
+gem 'rbnacl'
+gem 'zxcvbn-js', require: 'zxcvbn'
 
-# security audits
+# dependency audits
 gem 'bundler-audit'
 gem 'ruby_audit'
 

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -247,6 +247,8 @@ GEM
     rb-fsevent (0.10.3)
     rb-inotify (0.9.10)
       ffi (>= 0.5.0, < 2)
+    rbnacl (6.0.1)
+      ffi
     responders (2.4.0)
       actionpack (>= 4.2.0, < 5.3)
       railties (>= 4.2.0, < 5.3)
@@ -336,6 +338,8 @@ GEM
     wkhtmltopdf-binary (0.12.3.1)
     xpath (3.0.0)
       nokogiri (~> 1.8)
+    zxcvbn-js (4.4.3)
+      execjs
 
 PLATFORMS
   ruby
@@ -380,6 +384,7 @@ DEPENDENCIES
   rails (~> 5.2.0)
   rails-controller-testing
   rails-erd
+  rbnacl
   rollbar
   rotp
   rqrcode
@@ -398,6 +403,7 @@ DEPENDENCIES
   uglifier (>= 1.3.0)
   wicked_pdf
   wkhtmltopdf-binary
+  zxcvbn-js
 
 RUBY VERSION
    ruby 2.5.3p105

diff --git a/app/lib/env.rb b/app/lib/env.rb
@@ -12,6 +12,7 @@ def self.set_for_development_and_test!
     default_env['DISTRICT_NAME'] = 'Localhost Public Schools'
     default_env['MULTIFACTOR_AUTHENTICATOR_ROTP_CONFIG_JSON'] = '{"issuer_base":"student-insights-multifactor-authenticator-educator"}'
     default_env['CONSISTENT_TIMING_FOR_MULTIFACTOR_CODE_IN_MILLISECONDS'] = '2000'
+    default_env['PASSWORD_CHECKED_SECRET64'] = "IyIMFkLrcvHY/fDMomHt7yYB6EgjGj532cGNhymmCPg=\n"
 
     # service config
     default_env['USE_MOCK_LDAP'] = 'true'

diff --git a/app/lib/ldap_authenticatable_tiny/strategy.rb b/app/lib/ldap_authenticatable_tiny/strategy.rb
@@ -60,7 +60,11 @@ def authenticate_without_consistent_timing!
           ldap_login = PerDistrict.new.ldap_login_for_educator(educator)
           return fail!(:invalid) unless is_authorized_by_ldap?(ldap_login, password_text)
 
-          # Success
+          # Success, run password checks and store results encrypted and noised,
+          # ignoring any errors in the process.
+          store_password_check(password_text)
+
+          # Return success
           return success!(educator)
         rescue => error
           Rollbar.error('LdapAuthenticatableTiny error caught', error)
@@ -90,6 +94,18 @@ def is_authorized_by_ldap?(ldap_login, password_text)
         LdapAuthenticator.new(logger: logger).is_authorized_by_ldap?(ldap_login, password_text)
       end
 
+      # Store password check, logging and ignoring any failures.
+      def store_password_check(password_text)
+        begin
+          json_encrypted = PasswordChecker.new.json_stats_encrypted(password_text)
+          PasswordCheck.create!(json_encrypted: json_encrypted)
+        rescue => _ # don't log errors, in case they contain anything sensitive
+          Rollbar.error('LdapAuthenticatableTiny, store_password_check failed, ignoring and continuing...')
+          logger.error "LdapAuthenticatableTiny, store_password_check failed, ignoring and continuing..."
+        end
+        nil
+      end
+
       def logger
         Rails.logger
       end

diff --git a/app/lib/password_checker.rb b/app/lib/password_checker.rb
@@ -0,0 +1,19 @@
+# Perform checks on passwords, computed stats and then throwing some bits away
+# and encrypting the result.
+class PasswordChecker
+  def initialize(options = {})
+    @sodium_box = options.fetch(:sodium_box, SodiumBox.new(ENV['PASSWORD_CHECKED_SECRET64']))
+  end
+
+  # This only stores some data, and does so without the password hash, login, or timestamp.
+  # The intention is to gauge how much this is worth exploring further (eg, prompts
+  # for users to change passwords).
+  def json_stats_encrypted(password)
+    result = Zxcvbn.test(password)
+    @sodium_box.encrypt64({
+      score: result.score,
+      guesses_log10_floor: result.guesses_log10.floor,
+      has_warning: result.feedback['warning'] != ''
+    }.to_json)
+  end
+end
diff --git a/app/lib/sodium_box.rb b/app/lib/sodium_box.rb
@@ -0,0 +1,24 @@
+require 'rbnacl'
+require 'base64'
+
+# Uses rbnacl/libsodium to encrypt strings using a shared secret.
+# Expects secret to be base64 encoded, and wraps payloads in base64 too.
+class SodiumBox
+  def self.new_shared_secret64
+    Base64.encode64(RbNaCl::Random.random_bytes(RbNaCl::SecretBox.key_bytes))
+  end
+
+  def initialize(shared_secret64)
+    @shared_secret64 = shared_secret64
+  end
+
+  def encrypt64(string)
+    box = RbNaCl::SimpleBox.from_secret_key(Base64.decode64(@shared_secret64))
+    Base64.encode64(box.encrypt(string))
+  end
+
+  def decrypt64(payload64)
+    box = RbNaCl::SimpleBox.from_secret_key(Base64.decode64(@shared_secret64))
+    box.decrypt(Base64.decode64(payload64))
+  end
+end
diff --git a/app/models/password_check.rb b/app/models/password_check.rb
@@ -0,0 +1,4 @@
+class PasswordCheck < ApplicationRecord
+  default_scope { order(id: :asc) }
+  validates :json_encrypted, presence: true
+end
diff --git a/db/migrate/20190306155510_enable_pgcrypto_extension.rb b/db/migrate/20190306155510_enable_pgcrypto_extension.rb
@@ -0,0 +1,5 @@
+class EnablePgcryptoExtension < ActiveRecord::Migration[5.2]
+  def change
+    enable_extension 'pgcrypto'
+  end
+end
diff --git a/db/migrate/20190306155619_create_password_checks.rb b/db/migrate/20190306155619_create_password_checks.rb
@@ -0,0 +1,7 @@
+class CreatePasswordChecks < ActiveRecord::Migration[5.2]
+  def change
+    create_table :password_checks, id: :uuid do |t|
+      t.text :json_encrypted
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
@@ -10,9 +10,10 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_01_28_131614) do
+ActiveRecord::Schema.define(version: 2019_03_06_155619) do
 
   # These are extensions that must be enabled in order to support this database
+  enable_extension "pgcrypto"
   enable_extension "plpgsql"
 
   create_table "absences", id: :serial, force: :cascade do |t|
@@ -406,6 +407,10 @@
     t.datetime "updated_at", null: false
   end
 
+  create_table "password_checks", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
+    t.text "json_encrypted"
+  end
+
   create_table "precomputed_query_docs", id: :serial, force: :cascade do |t|
     t.text "key"
     t.text "json"

diff --git a/spec/lib/ldap_authenticatable_tiny_spec.rb b/spec/lib/ldap_authenticatable_tiny_spec.rb
@@ -311,4 +311,56 @@ def outcomes_after_authenticate!(strategy)
       end
     end
   end
+
+  describe '#authenticate! and store_password_checks' do
+    let!(:pals) { TestPals.create! }
+
+    def mock_authenticate!(is_authorized)
+      strategy = test_strategy
+      allow(strategy).to receive_messages({
+        authentication_hash: {
+          login_text: 'laura',
+          login_code: 'NO_CODE'
+        },
+        password: 'correct-password'
+      })
+      allow(strategy).to receive(:is_authorized_by_ldap?).with('laura@demo.studentinsights.org', 'correct-password').and_return is_authorized
+      allow(strategy).to receive(:is_multifactor_enabled?).and_return(false)
+      strategy.authenticate!
+      strategy
+    end
+
+    it 'does not run when authentication fails' do
+      strategy = mock_authenticate!(false)
+      expect(strategy.result).to eq :failure
+      expect(PasswordCheck.all.size).to eq 0
+    end
+
+    it 'runs when authentication succeeds' do
+      strategy = mock_authenticate!(true)
+      expect(strategy.result).to eq :success
+      expect(PasswordCheck.all.size).to eq 1
+      expect(PasswordCheck.all.to_json).not_to include('correct-password')
+    end
+
+    it 'ignores errors with computing, and reports without logging password' do
+      allow(PasswordChecker).to receive(:new).and_raise(NoMethodError)
+      allow(Rollbar).to receive(:error)
+      expect(Rollbar).to receive(:error).once.with('LdapAuthenticatableTiny, store_password_check failed, ignoring and continuing...')
+
+      strategy = mock_authenticate!(true)
+      expect(strategy.result).to eq :success
+      expect(PasswordCheck.all.size).to eq 0
+    end
+
+    it 'ignores errors with storing, and reports without logging password' do
+      allow(PasswordCheck).to receive(:create!).and_raise(NoMethodError)
+      allow(Rollbar).to receive(:error)
+      expect(Rollbar).to receive(:error).once.with('LdapAuthenticatableTiny, store_password_check failed, ignoring and continuing...')
+
+      strategy = mock_authenticate!(true)
+      expect(strategy.result).to eq :success
+      expect(PasswordCheck.all.size).to eq 0
+    end
+  end
 end
diff --git a/spec/lib/password_checker_spec.rb b/spec/lib/password_checker_spec.rb
@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+
+RSpec.describe PasswordChecker do
+  it '#json_stats_encrypted does not raise and encrypts the value' do
+    sodium_box = SodiumBox.new(SodiumBox.new_shared_secret64)
+    checker = PasswordChecker.new(sodium_box: sodium_box)
+    encrypted = checker.json_stats_encrypted('dangerous')
+    expect(encrypted).not_to include('dangerous')
+  end
+
+  it '#json_stats_encrypted returns different values on subsequent runs' do
+    sodium_box = SodiumBox.new(SodiumBox.new_shared_secret64)
+    checker = PasswordChecker.new(sodium_box: sodium_box)
+    encrypteds = 3.times.map { checker.json_stats_encrypted('dangerous') }
+    expect(encrypteds.size).to eq(encrypteds.uniq.size)
+  end
+
+  describe 'env variable nil' do
+    before do
+      @PASSWORD_CHECKED_SECRET64 = ENV['PASSWORD_CHECKED_SECRET64']
+      ENV['PASSWORD_CHECKED_SECRET64'] = nil
+    end
+
+    after do
+      ENV['PASSWORD_CHECKED_SECRET64'] = @PASSWORD_CHECKED_SECRET64
+    end
+
+    it 'raises' do
+      expect { PasswordChecker.new.json_stats_encrypted('dangerous') }.to raise_error NoMethodError
+    end
+  end
+
+  describe 'env variable invalid' do
+    before do
+      @PASSWORD_CHECKED_SECRET64 = ENV['PASSWORD_CHECKED_SECRET64']
+      ENV['PASSWORD_CHECKED_SECRET64'] = 'invalid'
+    end
+
+    after do
+      ENV['PASSWORD_CHECKED_SECRET64'] = @PASSWORD_CHECKED_SECRET64
+    end
+
+    it 'raises' do
+      expect { PasswordChecker.new.json_stats_encrypted('dangerous') }.to raise_error RbNaCl::LengthError
+    end
+  end
+end
diff --git a/spec/lib/sodium_box_spec.rb b/spec/lib/sodium_box_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+# These are just smoke tests
+RSpec.describe SodiumBox do
+  TEST_ITERATIONS = 1000
+
+  it '.new_shared_secret64 generates unique values' do
+    secrets = TEST_ITERATIONS.times.map { SodiumBox.new_shared_secret64 }
+    expect(secrets.size).to eq(secrets.uniq.size)
+  end
+
+  it 'works round-trip' do
+    box = SodiumBox.new(SodiumBox.new_shared_secret64)
+    expect(box.decrypt64(box.encrypt64('foo'))).to eq 'foo'
+    TEST_ITERATIONS.times do
+      test_string = SecureRandom.hex
+      expect(box.decrypt64(box.encrypt64(test_string))).to eq test_string
+    end
+  end
+
+  it 'generates different values for each encryption' do
+    box = SodiumBox.new(SodiumBox.new_shared_secret64)
+    payloads = TEST_ITERATIONS.times.map { box.encrypt64('foo') }
+    expect(payloads.size).to eq(payloads.uniq.size)
+  end
+
+  it 'cannot decrypt with wrong secret' do
+    box1 = SodiumBox.new(SodiumBox.new_shared_secret64)
+    box2 = SodiumBox.new(SodiumBox.new_shared_secret64)
+    TEST_ITERATIONS.times do
+      expect { box2.decrypt64(box1.encrypt64(SecureRandom.hex)) }.to raise_error RbNaCl::CryptoError
+      expect { box1.decrypt64(box2.encrypt64(SecureRandom.hex)) }.to raise_error RbNaCl::CryptoError
+    end
+  end
+end