Skip to content

[10/15] Normalize identifiers before keying the auth rate limiter #163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
UnknownJoe796 wants to merge 1 commit into
base: version-5.3
Choose a base branch
from
Open

[10/15] Normalize identifiers before keying the auth rate limiter #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions .../src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/BackupCodeEndpoints.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,19 @@ public class BackupCodeEndpoints(
),
successCode = HttpStatus.OK,
implementation = { input: IdentificationAndPassword ->
cache().constrainAttemptRate(
cacheKey = "backup-code-count-${input.property}-${input.value}"
) {
val subject = input.type
val subject = input.type

val handler = serverRuntime.server.principalTypes.values.find { it.name == subject }
?: throw IllegalArgumentException("No subject $subject recognized")
val handler = serverRuntime.server.principalTypes.values.find { it.name == subject }
?: throw IllegalArgumentException("No subject $subject recognized")

// Normalize BEFORE building the rate-limit key: the key must be derived from the canonical
// identifier so that case/whitespace variants of the same account share one bucket. Keying on
// the raw value would let an attacker dodge the limiter (and its exponential backoff) simply
// by varying case or whitespace.
val normalizedValue = handler.normalizePropertyValue(input.property, input.value)
cache().constrainAttemptRate(
cacheKey = "backup-code-count-${input.property}-${normalizedValue}"
) {
val subjectId = handler.fetchUserIdString(input.property, input.value)
?: throw BadRequestException("Invalid Backup Code")

Expand Down
14 changes: 9 additions & 5 deletions ...c/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/PasswordProofEndpoints.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,15 @@ public class PasswordProofEndpoints(
successCode = HttpStatus.OK,
implementation = { input: IdentificationAndPassword ->
val now = now()
cache().constrainAttemptRate("password-${input.property}-${input.value}") {
val subject = input.type
val handler = serverRuntime.server.principalTypes.values.find { it.name == subject }
?: throw IllegalArgumentException("No subject $subject recognized")
val normalizedValue = handler.normalizePropertyValue(input.property, input.value)
val subject = input.type
val handler = serverRuntime.server.principalTypes.values.find { it.name == subject }
?: throw IllegalArgumentException("No subject $subject recognized")
// Normalize BEFORE building the rate-limit key: the key must be derived from the canonical
// identifier so that case/whitespace variants (e.g. "Bob@x.com" vs "bob@x.com ") of the same
// account share one bucket. Keying on the raw value would let an attacker dodge the limiter
// (and its exponential backoff) simply by varying case or whitespace.
val normalizedValue = handler.normalizePropertyValue(input.property, input.value)
cache().constrainAttemptRate("password-${input.property}-${normalizedValue}") {
val subjectId = handler.fetchUserIdString(input.property, normalizedValue)
?: throw BadRequestException("User ID and code do not match")

Expand Down
14 changes: 9 additions & 5 deletions ...in/kotlin/com/lightningkite/lightningserver/sessions/proofs/TimeBasedOTPProofEndpoints.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,17 @@ public class TimeBasedOTPProofEndpoints(
implementation = { input: IdentificationAndPassword ->
val now = now()
val gracePeriod = now().minus(5.seconds)
val subject = input.type
val handler = serverRuntime.server.principalTypes[subject]
?: throw IllegalArgumentException("No subject $subject recognized")
// Normalize BEFORE building the rate-limit key: the key must be derived from the canonical
// identifier so that case/whitespace variants of the same account share one bucket. Keying on
// the raw value would let an attacker dodge the limiter (and its exponential backoff) simply
// by varying case or whitespace.
val normalizedValue = handler.normalizePropertyValue(input.property, input.value)
cache().constrainAttemptRate(
cacheKey = "totp-count-${input.property}-${input.value}"
cacheKey = "totp-count-${input.property}-${normalizedValue}"
) {
val subject = input.type
val handler = serverRuntime.server.principalTypes[subject]
?: throw IllegalArgumentException("No subject $subject recognized")
val normalizedValue = handler.normalizePropertyValue(input.property, input.value)
val subjectId = handler.fetchUserIdString(input.property, normalizedValue)
?: throw BadRequestException("User ID and code do not match")

Expand Down
69 changes: 69 additions & 0 deletions .../test/kotlin/com/lightningkite/lightningserver/sessions/proofs/BackupCodeEndpointsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,4 +590,73 @@ class BackupCodeEndpointsTest {
}
}
}

/**
* Security regression: the rate-limit key must be built from the NORMALIZED identifier, so that
* case/whitespace variants of one account share a single bucket. If the key were derived from the
* raw value, an attacker could dodge the limiter (and its exponential backoff) by varying case.
*/
@Test
fun `rate limiter shares one bucket across case variants of the same identifier`() = runBlocking {
TestUser.users.clear()
val userId = Uuid.random()
val user = TestUser(userId, "test@example.com")
TestUser.users[userId] = user

object : ServerBuilder() {
val database = setting("database", Database.Settings("ram"))
val cache = setting("cache", Cache.Settings("ram"))

init {
register(TestUser)
}

val backupCodes = path.path("auth").path("backup") include BackupCodeEndpoints(
database = database,
cache = cache,
proofSigner = RuntimeDeferred.Cached { testBasis.signer("proof") },
proofExpiration = 1.hours
)
}.let { server ->
server.test({}) {
server.backupCodes.modelInfo.table().insert(
listOf(
BackupCodeSecret(
code = "validcode",
subjectId = TestUser.idString(userId),
subjectType = TestUser.name
)
)
)

// Five distinct case variants that all normalize to "test@example.com". The default limit
// is 5 attempts; five failing attempts across these variants must fill ONE shared bucket.
val emailVariants = listOf(
"Test@example.com",
"tEst@example.com",
"teSt@example.com",
"tesT@example.com",
"TEST@example.com",
)
for (variant in emailVariants) {
assertFailsWith<BadRequestException> {
server.backupCodes.prove.test(
null, IdentificationAndPassword("TestUser", "email", variant, "wrongcode")
)
}
}

// A sixth attempt with yet another distinct variant must be blocked by the shared limiter.
val blocked = assertFailsWith<BadRequestException> {
server.backupCodes.prove.test(
null, IdentificationAndPassword("TestUser", "email", "TesT@example.com", "wrongcode")
)
}
assertTrue(
blocked.message.contains("Too many attempts"),
"Expected the shared rate limiter to block, but got: ${blocked.message}"
)
}
}
}
}
63 changes: 63 additions & 0 deletions ...st/kotlin/com/lightningkite/lightningserver/sessions/proofs/PasswordProofEndpointsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,4 +433,67 @@ class PasswordProofEndpointsTest {
}
}
}

/**
* Security regression: the rate-limit key must be built from the NORMALIZED identifier, so that
* case/whitespace variants of one account share a single bucket. If the key were derived from the
* raw value, an attacker could dodge the limiter (and its exponential backoff) by varying case.
*/
@Test
fun `rate limiter shares one bucket across case variants of the same identifier`() = runBlocking {
TestUser.users.clear()
val userId = Uuid.random()
val user = TestUser(userId, "test@example.com")
TestUser.users[userId] = user

object : ServerBuilder() {
val database = setting("database", Database.Settings("ram"))
val cache = setting("cache", Cache.Settings("ram"))

init {
register(TestUser)
}

val passwordProof = path.path("auth").path("password") include PasswordProofEndpoints(
database = database,
cache = cache,
proofSigner = RuntimeDeferred.Cached { testBasis.signer("proof") },
proofExpiration = 1.hours
)
}.let { server ->
server.test({}) {
server.passwordProof.establish(TestUser, userId, EstablishPassword("correctPassword"))

// Five distinct case variants that all normalize to "test@example.com". The default limit
// is 5 attempts; five failing attempts across these variants must fill ONE shared bucket.
val emailVariants = listOf(
"Test@example.com",
"tEst@example.com",
"teSt@example.com",
"tesT@example.com",
"TEST@example.com",
)
for (variant in emailVariants) {
assertFailsWith<BadRequestException> {
server.passwordProof.prove.test(
null, IdentificationAndPassword("TestUser", "email", variant, "wrongPassword")
)
}
}

// A sixth attempt with yet another distinct variant must be blocked by the shared limiter.
// Under the old raw-value key this variant would be an untouched bucket and fail only with
// the ordinary "does not match" error.
val blocked = assertFailsWith<BadRequestException> {
server.passwordProof.prove.test(
null, IdentificationAndPassword("TestUser", "email", "TesT@example.com", "wrongPassword")
)
}
assertTrue(
blocked.message.contains("Too many attempts"),
"Expected the shared rate limiter to block, but got: ${blocked.message}"
)
}
}
}
}
77 changes: 77 additions & 0 deletions ...otlin/com/lightningkite/lightningserver/sessions/proofs/TimeBasedOTPProofEndpointsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,4 +526,81 @@ class TimeBasedOTPProofEndpointsTest {
}
}
}

/**
* Security regression: the rate-limit key must be built from the NORMALIZED identifier, so that
* case/whitespace variants of one account share a single bucket. If the key were derived from the
* raw value, an attacker could dodge the limiter (and its exponential backoff) by varying case.
*/
@Test
fun `rate limiter shares one bucket across case variants of the same identifier`() = runBlocking {
TestUser.users.clear()
val userId = Uuid.random()
val user = TestUser(userId, "test@example.com")
TestUser.users[userId] = user

object : ServerBuilder() {
val database = setting("database", Database.Settings("ram"))
val cache = setting("cache", Cache.Settings("ram"))

init {
register(TestUser)
}

val totpEndpoints = path.path("auth").path("totp") include TimeBasedOTPProofEndpoints(
database = database,
cache = cache,
proofSigner = RuntimeDeferred.Cached { testBasis.signer("proof") },
proofExpiration = 1.hours,
config = testConfig
)
}.let { server ->
server.test({}) {
server.totpEndpoints.modelInfo.table().insert(
listOf(
TotpSecret(
subjectId = TestUser.idString(userId),
subjectType = TestUser.name,
secretBase32 = testSecretBase32,
label = "test",
issuer = "TestApp",
period = 30.seconds,
digits = 6,
algorithm = TotpHashAlgorithm.SHA1,
establishedAt = Clock.System.now(),
lastUsedAt = Clock.System.now()
)
)
)

// Five distinct case variants that all normalize to "test@example.com". The default limit
// is 5 attempts; five failing attempts across these variants must fill ONE shared bucket.
val emailVariants = listOf(
"Test@example.com",
"tEst@example.com",
"teSt@example.com",
"tesT@example.com",
"TEST@example.com",
)
for (variant in emailVariants) {
assertFailsWith<BadRequestException> {
server.totpEndpoints.prove.test(
null, IdentificationAndPassword("TestUser", "email", variant, "000000")
)
}
}

// A sixth attempt with yet another distinct variant must be blocked by the shared limiter.
val blocked = assertFailsWith<BadRequestException> {
server.totpEndpoints.prove.test(
null, IdentificationAndPassword("TestUser", "email", "TesT@example.com", "000000")
)
}
assertTrue(
blocked.message.contains("Too many attempts"),
"Expected the shared rate limiter to block, but got: ${blocked.message}"
)
}
}
}
}
Loading