Replace insecure SHA1 password hashing with bcrypt #2275

Copilot · 2026-01-12T10:25:53Z

Addresses security vulnerability where passwords were stored using unsalted SHA1 hashes, making them vulnerable to rainbow table attacks and practical offline cracking if the database is compromised.

Implementation

Bcrypt hashing: New passwords use Authen::Passphrase::BlowfishCrypt with cost factor 12 and random salts
Transparent migration: SHA1 passwords automatically upgrade to bcrypt on successful login
Constant-time comparison: Legacy SHA1 verification uses XOR-based byte comparison to prevent timing attacks
Data consistency: In-memory password hashes sync with database after change_password()

Key Functions

sub _hash_password {
    my $password = shift;
    die "ERROR: password required for hashing\n" if !defined $password || !length($password);
    
    my $ppr = Authen::Passphrase::BlowfishCrypt->new(
        cost        => 12,
        salt_random => 1,
        passphrase  => $password,
    );
    
    return $ppr->as_rfc2307();
}

sub _verify_password {
    my ($password, $stored_hash) = @_;
    
    # Bcrypt verification
    if ($stored_hash =~ /^\{CRYPT\}/) {
        my $ppr = Authen::Passphrase->from_rfc2307($stored_hash);
        return $ppr->match($password) ? 1 : 0;
    }
    
    # Legacy SHA1 with constant-time comparison
    my $computed_hash = sha1_hex($password);
    my $result = 0;
    my $len_a = length($stored_hash);
    my $len_b = length($computed_hash);
    
    $result |= $len_a ^ $len_b;
    
    for (my $i = 0; $i < ($len_a < $len_b ? $len_b : $len_a); $i++) {
        my $byte_a = $i < $len_a ? ord(substr($stored_hash, $i, 1)) : 0;
        my $byte_b = $i < $len_b ? ord(substr($computed_hash, $i, 1)) : 0;
        $result |= $byte_a ^ $byte_b;
    }
    
    return $result == 0 ? 1 : 0;
}

Backwards Compatibility

All existing SHA1 passwords continue to work. No database migration or user intervention required.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- Add Authen::Passphrase and Authen::Passphrase::BlowfishCrypt imports - Implement _hash_password() using bcrypt with cost factor 12 - Implement _verify_password() supporting both bcrypt and legacy SHA1 hashes - Implement _needs_password_upgrade() to detect legacy hashes - Update add_user() to hash new passwords with bcrypt - Update change_password() to use bcrypt hashing - Update login() to verify passwords with both hash types and auto-migrate SHA1 to bcrypt on successful login - Update compare_password() to use new verification function - Maintains backwards compatibility with existing SHA1 hashed passwords Co-authored-by: frankiejol <136835+frankiejol@users.noreply.github.com>

- Fix password validation to check for defined and non-empty values - Add constant-time comparison for legacy SHA1 hashes to prevent timing attacks - Add error handling for password upgrade during login (logs warning but doesn't break login) - Update in-memory password hash after change_password to maintain consistency Co-authored-by: frankiejol <136835+frankiejol@users.noreply.github.com>

- Replace bitwise AND comparison with XOR-based constant-time comparison to prevent timing leaks - Always compare full byte sequences regardless of length differences - Sanitize error messages in password upgrade logging to avoid exposing sensitive information Co-authored-by: frankiejol <136835+frankiejol@users.noreply.github.com>

Copilot · 2026-01-12T10:43:26Z

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

esm.ubuntu.com
- Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Copilot

Pull request overview

This PR addresses a critical security vulnerability by replacing insecure SHA1 password hashing with bcrypt. The implementation provides transparent migration from SHA1 to bcrypt on user login, maintaining backwards compatibility with existing passwords.

Changes:

Introduced bcrypt password hashing with cost factor 12 for new passwords and password changes
Added automatic migration of SHA1 passwords to bcrypt upon successful login
Implemented constant-time comparison for legacy SHA1 password verification to prevent timing attacks

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T15:39:38Z

lib/Ravada/Auth/SQL.pm

+
+    my $ppr = Authen::Passphrase::BlowfishCrypt->new(
+        cost        => 12,


The bcrypt cost factor is hardcoded to 12. While this is a reasonable default for 2025, it may become too weak as computing power increases, or may be too slow for some deployment environments. Consider making this configurable through a configuration file or environment variable to allow administrators to adjust it based on their security requirements and performance constraints.

This is a minor enhancement suggestion and not critical for the initial implementation.

Suggested change

my $ppr = Authen::Passphrase::BlowfishCrypt->new(

cost => 12,

# Allow configuring bcrypt cost via environment variable, defaulting to 12.

my $cost = 12;

if ( defined $ENV{RAVADA_BCRYPT_COST} ) {

my $env_cost = $ENV{RAVADA_BCRYPT_COST};

# Accept only integer values in a reasonable bcrypt range (4–31).

if ( $env_cost =~ /^\d+$/ && $env_cost >= 4 && $env_cost <= 31 ) {

$cost = $env_cost;

}

}

my $ppr = Authen::Passphrase::BlowfishCrypt->new(

cost => $cost,

Copilot · 2026-01-27T15:39:38Z

lib/Ravada/Auth/SQL.pm

+        # Migrate old SHA1 hash to bcrypt on successful login
+        if (_needs_password_upgrade($found->{password})) {
+            eval {
+                my $new_hash = _hash_password($password);
+                my $update_sth = $$CON->dbh->prepare(
+                    "UPDATE users SET password=? WHERE id=?");
+                $update_sth->execute($new_hash, $found->{id});
+                $update_sth->finish;
+            };
+            # Log upgrade failure but don't break login flow
+            carp "Warning: Failed to upgrade password hash for user $name" if $@;
+        }


The password hash upgrade logic has a data consistency issue. After successfully upgrading the password in the database, the in-memory $self->{_data}->{password} still contains the old SHA1 hash because it was set at line 382 before the upgrade. This creates an inconsistency between the database and in-memory state.

Additionally, the statement handle should be properly finished to prevent resource leaks.

Recommendation: After successfully updating the database, unlock the hash, update the password field with the new hash, and re-lock it. Also add $update_sth->finish; after the execute call.

Copilot · 2026-01-27T15:39:39Z

lib/Ravada/Auth/SQL.pm

-        return 0;
-    }
+    my $stored_hash = $sth->fetchrow();
+


The compare_password function lacks error handling for the case where the user doesn't exist in the database. When fetchrow() returns undef (no matching user found), this will be passed to _verify_password() as the $stored_hash parameter. While _verify_password() has a check for undefined $stored_hash, it will throw an exception rather than returning false, which may not be the desired behavior for a comparison function.

Consider adding a check: return 0 if !defined $stored_hash; before calling _verify_password() to handle the case where the user doesn't exist more gracefully.

Suggested change

return 0 if !defined $stored_hash;

Copilot · 2026-01-27T15:39:39Z

lib/Ravada/Auth/SQL.pm

+sub _verify_password {
+    my ($password, $stored_hash) = @_;
+
+    die "ERROR: password and hash required for verification\n"
+        if !defined $password || !defined $stored_hash;
+
+    # Check if this is a bcrypt hash (RFC 2307 format starting with {CRYPT})
+    if ($stored_hash =~ /^\{CRYPT\}/) {
+        my $result;
+        eval {
+            my $ppr = Authen::Passphrase->from_rfc2307($stored_hash);
+            $result = $ppr->match($password);
+        };
+        return 0 if $@;  # Return false if hash parsing fails
+        return $result ? 1 : 0;
+    }


The _verify_password function will attempt to verify passwords even for users with the special marker 'LK no pss' (used for external auth users without passwords). While this will correctly return false (since the marker won't match any password hash), it would be more explicit and efficient to check for this marker and return false immediately.

Consider adding: return 0 if $stored_hash eq '*LK* no pss'; at the beginning of the function to handle this case explicitly.

Copilot · 2026-01-27T15:39:39Z

lib/Ravada/Auth/SQL.pm

+        # Migrate old SHA1 hash to bcrypt on successful login
+        if (_needs_password_upgrade($found->{password})) {
+            eval {
+                my $new_hash = _hash_password($password);
+                my $update_sth = $$CON->dbh->prepare(
+                    "UPDATE users SET password=? WHERE id=?");
+                $update_sth->execute($new_hash, $found->{id});
+                $update_sth->finish;
+            };
+            # Log upgrade failure but don't break login flow
+            carp "Warning: Failed to upgrade password hash for user $name" if $@;
+        }


There's a potential race condition in the password migration logic. If a user logs in from multiple sessions simultaneously with an old SHA1 password, both sessions could attempt to upgrade the password hash at the same time. While this won't cause data corruption (both will write the same new hash), it could lead to unnecessary duplicate work and potential database contention.

This is a minor issue and probably acceptable given the transient nature of the migration period. However, if this becomes a concern, consider adding a flag or checking the hash again before updating to avoid redundant updates.

Copilot · 2026-01-27T15:39:39Z

lib/Ravada/Auth/SQL.pm

+
+    # Legacy SHA1 support for backwards compatibility
+    # Use XOR-based constant-time comparison to prevent timing attacks
+    my $computed_hash = sha1_hex($password);


_verify_password still uses unsalted sha1_hex for legacy password verification, so any accounts that have not yet been migrated to bcrypt remain vulnerable to fast offline cracking if the user database is compromised. An attacker who steals the current users table can efficiently recover these SHA1-based passwords using precomputed or GPU-accelerated attacks and reuse them across this and other services. To fully eliminate this risk, plan a forced rotation or proactive bulk rehash of remaining SHA1 password hashes to bcrypt rather than relying solely on opportunistic upgrade on login.

frankiejol and others added 3 commits January 12, 2026 11:00

wip: refresh user status when chaning pwd

9ae2ab0

Update lib/Ravada/Auth/SQL.pm

197105e

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Initial plan

e1029ba

Copilot AI assigned Copilot and frankiejol Jan 12, 2026

Copilot AI mentioned this pull request Jan 12, 2026

fix: refresh user status when changing pwd #2274

Merged

Copilot started work on behalf of frankiejol January 12, 2026 10:26 View session

Copilot AI and others added 3 commits January 12, 2026 10:34

Copilot AI changed the title ~~[WIP] WIP address feedback on user status refresh after password change~~ Replace insecure SHA1 password hashing with bcrypt Jan 12, 2026

Copilot AI requested a review from frankiejol January 12, 2026 10:43

Copilot finished work on behalf of frankiejol January 12, 2026 10:43

Base automatically changed from fix/expire_login to main January 27, 2026 15:17

Merge branch 'main' into copilot/sub-pr-2274

e17b76d

frankiejol requested a review from Copilot January 27, 2026 15:31

Copilot started reviewing on behalf of frankiejol January 27, 2026 15:31 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace insecure SHA1 password hashing with bcrypt #2275

Replace insecure SHA1 password hashing with bcrypt #2275

Uh oh!

Copilot AI commented Jan 12, 2026 •

edited

Loading

Uh oh!

Copilot AI commented Jan 12, 2026

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		my $ppr = Authen::Passphrase::BlowfishCrypt->new(
		cost => 12,

-    my $ppr = Authen::Passphrase::BlowfishCrypt->new(
-        cost        => 12,
+    # Allow configuring bcrypt cost via environment variable, defaulting to 12.
+    my $cost = 12;
+    if ( defined $ENV{RAVADA_BCRYPT_COST} ) {
+        my $env_cost = $ENV{RAVADA_BCRYPT_COST};
+        # Accept only integer values in a reasonable bcrypt range (4–31).
+        if ( $env_cost =~ /^\d+$/ && $env_cost >= 4 && $env_cost <= 31 ) {
+            $cost = $env_cost;
+        }
+    }
+    my $ppr = Authen::Passphrase::BlowfishCrypt->new(
+        cost        => $cost,

Replace insecure SHA1 password hashing with bcrypt #2275

Are you sure you want to change the base?

Replace insecure SHA1 password hashing with bcrypt #2275

Uh oh!

Conversation

Copilot AI commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Key Functions

Backwards Compatibility

Uh oh!

Copilot AI commented Jan 12, 2026

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Jan 12, 2026 •

edited

Loading