From 29620df85e72a199afb5e0d9d2c8b3f2e73c5b34 Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@floxdev.com>
Date: Thu, 15 Apr 2021 10:40:55 -0400
Subject: [PATCH] Passwords: check in constant time

The default password comparison logic does not use
constant time validation. Switching to constant time
offers a meager improvement by removing a timing
oracle.

A prepatory step in moving to Argon2id password storage, since we'll need this change anyway after
for validating existing passwords.

Co-authored-by: Graham Christensen <graham@grahamc.com>
---
 flake.nix                     | 14 ++++++++++++++
 src/lib/Hydra.pm              |  3 +--
 src/lib/Hydra/Schema/Users.pm |  9 +++++++++
 t/Schema/Users.t              | 29 +++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 2 deletions(-)
 create mode 100644 t/Schema/Users.t

diff --git a/flake.nix b/flake.nix
index 6444af6c..8fac28dd 100644
--- a/flake.nix
+++ b/flake.nix
@@ -229,6 +229,19 @@
               license = with final.stdenv.lib.licenses; [ artistic1 ];
             };
           };
+
+          StringCompareConstantTime = final.buildPerlPackage {
+            pname = "String-Compare-ConstantTime";
+            version = "0.321";
+            src = final.fetchurl {
+              url = "mirror://cpan/authors/id/F/FR/FRACTAL/String-Compare-ConstantTime-0.321.tar.gz";
+              sha256 = "0b26ba2b121d8004425d4485d1d46f59001c83763aa26624dff6220d7735d7f7";
+            };
+            meta = {
+              description = "Timing side-channel protected string compare";
+              license = with final.lib.licenses; [ artistic1 gpl1Plus ];
+            };
+          };
         };
 
         hydra = with final; let
@@ -279,6 +292,7 @@
                 SQLSplitStatement
                 SetScalar
                 Starman
+                StringCompareConstantTime
                 SysHostnameLong
                 TermSizeAny
                 TestMore
diff --git a/src/lib/Hydra.pm b/src/lib/Hydra.pm
index a90235b8..0ff86f11 100644
--- a/src/lib/Hydra.pm
+++ b/src/lib/Hydra.pm
@@ -34,8 +34,7 @@ __PACKAGE__->config(
             credential => {
                 class => "Password",
                 password_field => "password",
-                password_type => "hashed",
-                password_hash_type => "SHA-1",
+                password_type => "self_check",
             },
             store => {
                 class => "DBIx::Class",
diff --git a/src/lib/Hydra/Schema/Users.pm b/src/lib/Hydra/Schema/Users.pm
index 7789b42c..ca650d9e 100644
--- a/src/lib/Hydra/Schema/Users.pm
+++ b/src/lib/Hydra/Schema/Users.pm
@@ -195,6 +195,9 @@ __PACKAGE__->many_to_many("projects", "projectmembers", "project");
 # Created by DBIx::Class::Schema::Loader v0.07049 @ 2020-02-06 12:22:36
 # DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:4/WZ95asbnGmK+nEHb4sLQ
 
+use Digest::SHA1 qw(sha1_hex);
+use String::Compare::ConstantTime;
+
 my %hint = (
     columns => [
         "fullname",
@@ -210,4 +213,10 @@ sub json_hint {
     return \%hint;
 }
 
+sub check_password {
+    my ($self, $password) = @_;
+
+    return String::Compare::ConstantTime::equals($self->password, sha1_hex($password));
+}
+
 1;
diff --git a/t/Schema/Users.t b/t/Schema/Users.t
new file mode 100644
index 00000000..edfb9002
--- /dev/null
+++ b/t/Schema/Users.t
@@ -0,0 +1,29 @@
+use strict;
+use Setup;
+
+my %ctx = test_init();
+
+require Hydra::Schema;
+require Hydra::Model::DB;
+
+use Test2::V0;
+
+my $db = Hydra::Model::DB->new;
+hydra_setup($db);
+
+# Catalyst's default password checking is not constant time. To improve
+# the security of the system, we replaced the check password routine.
+# Verify comparing correct and incorrect passwords work.
+
+# Starting the user with a sha1 password
+my $user = $db->resultset('Users')->create({
+    "username" => "alice",
+    "emailaddress" => 'alice@nixos.org',
+    "password" => "8843d7f92416211de9ebb963ff4ce28125932878" # SHA1 of "foobar"
+});
+isnt($user, undef, "My user was created.");
+
+ok(!$user->check_password("barbaz"), "Checking the password, barbaz, is not right");
+ok($user->check_password("foobar"), "Checking the password, foobar, is right");
+
+done_testing;