fix(sécurité): verrouillage de compte détournable, et Vary manquant sur le CORS
isLockedOut comptait `username = $2 OR ip = $3` dans un seul total. Cinq échecs avec le nom d'un admin, depuis n'importe quelle adresse, verrouillaient donc ce compte un quart d'heure — à répéter indéfiniment, sans coût et sans connaître le mot de passe. Il suffisait de connaître le nom d'utilisateur pour mettre l'admin dehors en permanence. Les deux compteurs deviennent distincts et volontairement asymétriques. L'IP reste stricte à 5 : c'est elle qui freine la force brute, et s'auto-verrouiller n'a aucun intérêt pour un attaquant. Le compteur par username subsiste contre une attaque répartie, mais à 20 — il faut brûler 4 IP avant de commencer à gêner le titulaire du compte. Vérifié contre PostgreSQL : l'admin légitime a bien par_ip=0 quand un tiers martèle depuis ailleurs. Ajoute par ailleurs `Vary: Origin` aux réponses CORS. L'en-tête Allow-Origin dépend de l'origine demandée : sans Vary, un cache intermédiaire peut servir à une origine la réponse mise en cache pour une autre — ce qui revient à autoriser une origine qui ne l'est pas, ou à faire refuser une origine légitime. Posé aussi quand l'origine est refusée, pour la même raison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
bcd533ae84
commit
a21bd55a90
4 changed files with 66 additions and 15 deletions
|
|
@ -4,7 +4,20 @@ import jwt from "jsonwebtoken";
|
|||
export const ADMIN_COOKIE_NAME = "admin_session";
|
||||
const SESSION_TTL_S = 12 * 3600; // 12h
|
||||
const LOCKOUT_WINDOW_MIN = 15;
|
||||
const LOCKOUT_MAX_ATTEMPTS = 5;
|
||||
// Deux compteurs distincts, volontairement asymetriques.
|
||||
//
|
||||
// L'ancienne requete comptait `username = $2 OR ip = $3` dans un seul total :
|
||||
// cinq echecs avec le nom d'un admin, depuis n'importe quelle adresse,
|
||||
// verrouillaient ce compte un quart d'heure. N'importe qui connaissant le nom
|
||||
// d'utilisateur pouvait donc mettre l'admin dehors, indefiniment et sans cout.
|
||||
//
|
||||
// L'IP reste stricte : c'est elle qui freine une attaque par force brute, et
|
||||
// s'auto-verrouiller n'a aucun interet pour un attaquant. Le compteur par
|
||||
// username subsiste contre une attaque repartie sur plusieurs adresses, mais
|
||||
// avec un seuil bien plus haut : un attaquant devra bruler 4 IP avant de
|
||||
// commencer a genner le titulaire du compte.
|
||||
const LOCKOUT_MAX_PER_IP = 5;
|
||||
const LOCKOUT_MAX_PER_USERNAME = 20;
|
||||
// Hash bcrypt valide mais sans correspondance, utilisé pour égaliser le temps
|
||||
// de réponse quand le username n'existe pas (évite l'énumération de comptes).
|
||||
const DUMMY_HASH = "$2b$12$CwTycUXWue0Thq9StjUM0uJ8vKR1dlT0LYzGsv8ZE8nFI8q9Z5T9.";
|
||||
|
|
@ -45,14 +58,17 @@ export function verifyAdminSession(token) {
|
|||
|
||||
export async function isLockedOut(pool, username, ip) {
|
||||
const r = await pool.query(
|
||||
`SELECT count(*)::int AS n
|
||||
`SELECT
|
||||
count(*) FILTER (WHERE ip = $3)::int AS par_ip,
|
||||
count(*) FILTER (WHERE username = $2)::int AS par_username
|
||||
FROM admin_login_attempts
|
||||
WHERE success = false
|
||||
AND created_at > now() - make_interval(mins => $1)
|
||||
AND (username = $2 OR ip = $3)`,
|
||||
[LOCKOUT_WINDOW_MIN, username, ip]
|
||||
);
|
||||
return r.rows[0].n >= LOCKOUT_MAX_ATTEMPTS;
|
||||
const { par_ip, par_username } = r.rows[0];
|
||||
return par_ip >= LOCKOUT_MAX_PER_IP || par_username >= LOCKOUT_MAX_PER_USERNAME;
|
||||
}
|
||||
|
||||
export async function recordLoginAttempt(pool, username, ip, success) {
|
||||
|
|
|
|||
|
|
@ -73,6 +73,14 @@ export async function buildServer(opts = {}) {
|
|||
const origin = request.headers.origin;
|
||||
const allowedOrigins = corsOrigins;
|
||||
|
||||
// `Vary: Origin` est indispensable des lors que l'en-tete de reponse DEPEND
|
||||
// de l'origine demandee : sans lui, un cache intermediaire (CDN, proxy)
|
||||
// peut servir a une origine la reponse mise en cache pour une autre, ce qui
|
||||
// revient a autoriser une origine qui ne l'est pas. Pose systematiquement,
|
||||
// y compris quand l'origine est refusee -- sinon le refus lui-meme se
|
||||
// retrouve mis en cache pour une origine legitime.
|
||||
reply.header('Vary', 'Origin');
|
||||
|
||||
if (allowedOrigins.includes(origin)) {
|
||||
reply.header('Access-Control-Allow-Origin', origin);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -115,18 +115,32 @@ describe("verifyPassword", () => {
|
|||
});
|
||||
|
||||
describe("isLockedOut", () => {
|
||||
it("verrouille à partir de 5 tentatives échouées", async () => {
|
||||
const under = makeFakePool([{ match: "admin_login_attempts", result: rows({ n: 4 }) }]);
|
||||
const at = makeFakePool([{ match: "admin_login_attempts", result: rows({ n: 5 }) }]);
|
||||
expect(await isLockedOut(under, "nico", "1.2.3.4")).toBe(false);
|
||||
expect(await isLockedOut(at, "nico", "1.2.3.4")).toBe(true);
|
||||
const compteurs = (par_ip, par_username) =>
|
||||
makeFakePool([{ match: "admin_login_attempts", result: rows({ par_ip, par_username }) }]);
|
||||
|
||||
it("verrouille à partir de 5 échecs venant de la même IP", async () => {
|
||||
expect(await isLockedOut(compteurs(4, 4), "nico", "1.2.3.4")).toBe(false);
|
||||
expect(await isLockedOut(compteurs(5, 5), "nico", "1.2.3.4")).toBe(true);
|
||||
});
|
||||
|
||||
it("compte les tentatives par username OU par IP, sur 15 minutes", async () => {
|
||||
const pool = makeFakePool([{ match: "admin_login_attempts", result: rows({ n: 0 }) }]);
|
||||
it("un tiers ne peut pas verrouiller un compte en échouant depuis ailleurs", async () => {
|
||||
// Le coeur du correctif : les deux compteurs etaient additionnes dans un
|
||||
// seul total. Cinq echecs avec le nom d'un admin, depuis n'importe quelle
|
||||
// adresse, le mettaient dehors un quart d'heure — a repeter indefiniment.
|
||||
// L'admin legitime, lui, n'a aucun echec a son IP : il doit passer.
|
||||
expect(await isLockedOut(compteurs(0, 19), "nico", "1.2.3.4")).toBe(false);
|
||||
});
|
||||
|
||||
it("un seuil par username subsiste contre une attaque répartie", async () => {
|
||||
expect(await isLockedOut(compteurs(0, 20), "nico", "1.2.3.4")).toBe(true);
|
||||
});
|
||||
|
||||
it("compte sur une fenêtre de 15 minutes", async () => {
|
||||
const pool = compteurs(0, 0);
|
||||
await isLockedOut(pool, "nico", "1.2.3.4");
|
||||
const call = pool.find("admin_login_attempts");
|
||||
expect(call.sql).toMatch(/username = \$2 OR ip = \$3/);
|
||||
expect(call.sql).toMatch(/FILTER \(WHERE ip = \$3\)/);
|
||||
expect(call.sql).toMatch(/FILTER \(WHERE username = \$2\)/);
|
||||
expect(call.values).toEqual([15, "nico", "1.2.3.4"]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -73,6 +73,19 @@ describe("CORS", () => {
|
|||
expect(res.headers["access-control-allow-origin"]).toBeUndefined();
|
||||
});
|
||||
|
||||
it("annonce Vary: Origin, y compris quand l'origine est refusée", async () => {
|
||||
// L'en-tete Allow-Origin DEPEND de l'origine demandee. Sans Vary, un cache
|
||||
// intermediaire peut servir a une origine la reponse mise en cache pour une
|
||||
// autre — ce qui revient a autoriser une origine qui ne l'est pas, ou a
|
||||
// faire refuser une origine legitime.
|
||||
for (const origin of ["https://metalfrom.eu", "https://evil.example", undefined]) {
|
||||
const res = await build().inject({
|
||||
method: "GET", url: "/api/health", headers: origin ? { origin } : {},
|
||||
});
|
||||
expect(res.headers["vary"]).toContain("Origin");
|
||||
}
|
||||
});
|
||||
|
||||
it("ne fait pas de match par préfixe sur l'origine", async () => {
|
||||
const res = await build().inject({
|
||||
method: "GET", url: "/api/health", headers: { origin: "https://metalfrom.eu.evil.example" },
|
||||
|
|
@ -337,7 +350,7 @@ describe("/admin/auth/login", () => {
|
|||
|
||||
it("répond 429 quand le compte est verrouillé, sans vérifier le mot de passe", async () => {
|
||||
const res = await build([
|
||||
{ match: "admin_login_attempts", result: rows({ n: 99 }) },
|
||||
{ match: "FROM admin_login_attempts", result: rows({ par_ip: 99, par_username: 0 }) },
|
||||
{ match: "FROM admin_users", result: rows({ password_hash: "x" }) },
|
||||
]).inject({
|
||||
method: "POST", url: "/admin/auth/login",
|
||||
|
|
@ -352,7 +365,7 @@ describe("/admin/auth/login", () => {
|
|||
// Hash à coût 4 : le DUMMY_HASH de production est à coût 12 (~330 ms).
|
||||
const hash = bcrypt.hashSync("autre-chose", 4);
|
||||
const res = await build([
|
||||
{ match: "SELECT count(*)::int AS n", result: rows({ n: 0 }) },
|
||||
{ match: "FROM admin_login_attempts", result: rows({ par_ip: 0, par_username: 0 }) },
|
||||
{ match: "FROM admin_users", result: rows({ password_hash: hash }) },
|
||||
{ match: "INSERT INTO admin_login_attempts", result: rows() },
|
||||
]).inject({
|
||||
|
|
@ -368,7 +381,7 @@ describe("/admin/auth/login", () => {
|
|||
const bcrypt = (await import("bcryptjs")).default;
|
||||
const hash = bcrypt.hashSync("bon", 4);
|
||||
const res = await build([
|
||||
{ match: "SELECT count(*)::int AS n", result: rows({ n: 0 }) },
|
||||
{ match: "FROM admin_login_attempts", result: rows({ par_ip: 0, par_username: 0 }) },
|
||||
{ match: "FROM admin_users", result: rows({ password_hash: hash }) },
|
||||
{ match: "INSERT INTO admin_login_attempts", result: rows() },
|
||||
{ match: "UPDATE admin_users", result: rows() },
|
||||
|
|
@ -389,7 +402,7 @@ describe("/admin/auth/login", () => {
|
|||
const bcrypt = (await import("bcryptjs")).default;
|
||||
const hash = bcrypt.hashSync("x", 4);
|
||||
await build([
|
||||
{ match: "SELECT count(*)::int AS n", result: rows({ n: 0 }) },
|
||||
{ match: "FROM admin_login_attempts", result: rows({ par_ip: 0, par_username: 0 }) },
|
||||
{ match: "FROM admin_users", result: rows({ password_hash: hash }) },
|
||||
{ match: "INSERT INTO admin_login_attempts", result: rows() },
|
||||
]).inject({
|
||||
|
|
|
|||
Loading…
Reference in a new issue