Skip to content

Commit 0ada5d2

Browse files
ssddanbrownssddanbrown
committed
Login: Added extra timing defenses for failed login attempts
- Adds a dummy hash attempt to balance the time of unknown user login attempt with known user login attempt to help prevent timing being used to indicate existing accounts. - Adds some random variance to failed login attempts to help prevent timing based information discovery. Thanks to Tanner Marks for their responsible disclosure of this.
1 parent 4e406c4 commit 0ada5d2

2 files changed

Lines changed: 53 additions & 1 deletion

File tree

‎app/Access/LoginService.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use BookStack\Theming\ThemeEvents;
1414
use BookStack\Users\Models\User;
1515
use Exception;
16+
use Illuminate\Support\Facades\Hash;
1617

1718
class LoginService
1819
{
@@ -171,10 +172,22 @@ public function attempt(array $credentials, string $method, bool $remember = fal
171172
} catch (LoginAttemptInvalidUserException $e) {
172173
// Catch and return false for non-login accounts
173174
// so it looks like a normal invalid login.
174-
return false;
175+
$result = false;
175176
}
176177
}
177178

179+
// Perform a dummy hash check to balance out the time of a login with an existing known user
180+
// with that of a user not in the system (which we don't perform a hash check for in the above).
181+
if (!$result && auth()->getLastAttempted() === null) {
182+
Hash::check($credentials['password'], '$2y$04$A.H9icXH4/lxLd9DHuaYqO/GVBd0OKetxyY0txmNfTAlPLVnTBx3y');
183+
}
184+
185+
// Add some noise to request times on failed login attempts
186+
if (!$result) {
187+
$sleepMs = random_int(0, 250);
188+
usleep($sleepMs * 1000);
189+
}
190+
178191
return $result;
179192
}
180193

‎tests/Auth/AuthTest.php‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace Tests\Auth;
44

55
use BookStack\Access\Mfa\MfaSession;
6+
use Illuminate\Support\Facades\Cache;
67
use Illuminate\Support\Facades\Hash;
78
use Illuminate\Testing\TestResponse;
89
use Tests\TestCase;
@@ -172,6 +173,44 @@ public function test_login_specifically_disabled_for_guest_account()
172173
$resp->assertSee('These credentials do not match our records.');
173174
}
174175

176+
public function test_failed_login_attempt_has_noise_added_and_have_similar_times_between_known_and_unknown_users()
177+
{
178+
$this->markTestSkipped('Time consuming test');
179+
180+
$user = $this->users->editor();
181+
$user->password = bcrypt('password');
182+
$user->save();
183+
// Warmup
184+
$this->post('/login', ['email' => $user->email, 'password' => 'passwordtesting']);
185+
186+
// For known user attempts
187+
$durations = [];
188+
for ($i = 0; $i < 25; $i++) {
189+
$knownStart = microtime(true);
190+
$this->post('/login', ['email' => $user->email, 'password' => 'passwordtesting']);
191+
$durations[] = (microtime(true) - $knownStart) * 1000;
192+
Cache::clear(); // Clear the cache to avoid hitting rate limits
193+
}
194+
$range = max($durations) - min($durations);
195+
$this->assertGreaterThan(125, $range);
196+
$knownAvg = array_sum($durations) / count($durations);
197+
198+
// For unknown user attempts
199+
$durations = [];
200+
for ($i = 0; $i < 25; $i++) {
201+
$unknownStart = microtime(true);
202+
$this->post('/login', ['email' => 'unknown@example.com', 'password' => 'passwordtesting']);
203+
$durations[] = (microtime(true) - $unknownStart) * 1000;
204+
Cache::clear(); // Clear the cache to avoid hitting rate limits
205+
}
206+
$range = max($durations) - min($durations);
207+
$this->assertGreaterThan(125, $range);
208+
$unknownAvg = array_sum($durations) / count($durations);
209+
210+
$knownDiff = abs($knownAvg - $unknownAvg);
211+
$this->assertLessThan(25, $knownDiff);
212+
}
213+
175214
/**
176215
* Perform a login.
177216
*/

0 commit comments

Comments
 (0)