phpBB Lab

J’ai repris le ZIP hucial_antibot fichier par fichier et j’ai vérifié la logique serveur, les entrées utilisateur, les signatures, le tri, le PoW, la base SQL, l’ACP et le JavaScript.

Résultat de l’audit

Pas de SQL injection évidente, pas de XSS évident, pas de RCE, pas de backdoor et pas de fuite de secret.
En revanche, j’ai trouvé 3 points que je corrigerais avant de considérer l’extension comme réellement durcie.

Gravité - Problème - Fichier
Haute - Challenge marqué solved réutilisable pendant sa durée de vie - captcha/antibot.php
Moyenne - Garbage collector pouvant provoquer une grosse suppression SQL en une seule requête - captcha/antibot.php
Moyenne - Création de challenges et comptage des abus insuffisamment bornés sous forte charge - captcha/antibot.php
Faible - Migration 1.1.x ne durcit pas le schéma/indexation - migrations/*.php

1/ Le challenge solved peut être rejoué

Les lignes concernés 240 - 244

if (!empty($this->challenge['solved']))
{
    $this->solved = true;
    return false;
}

Le problème est que lorsqu'un challenge est validé :

SET solved = 1,
    solved_at = ...

mais il n'est pas supprimé ni invalidé cryptographiquement après utilisation. Le challenge reste donc valide jusqu'à son expiration, actuellement, soit 20 minutes :

'expires_at' => $now + 1200,

C'est problématique car un utilisateur ou un bot qui a réussi le CAPTCHA peut potentiellement conserver et les réutiliser pendant la durée de vie du challenge.

hucial_ab_id
hucial_ab_sig

C'est particulièrement intéressant pour les usages :

• login ;

• posting ;

• report ;

• autres actions utilisant le même type de CAPTCHA.

Le comportement est cohérent avec une partie du fonctionnement historique de phpBB : le CAPTCHA de phpBB conserve lui aussi un état solved. Mais dans ton extension, tu as explicitement présenté le challenge comme un mécanisme à usage unique /replay-controlled. Le code actuel ne garantit pas réellement cette propriété.

Il faut distinguer :

• CAPTCHA résolu pour permettre l'affichage du formulaire suivant

• autorisation CAPTCHA consommée lors de l'action protégée

La solution propre est d'ajouter un état d'utilisation et de refuser un challenge déjà consommé.

2/ Garbage collector dangereux sous forte charge

Lignes 65 - 68

if (mt_rand(1, 100) === 1)
{
    $this->garbage_collect(0);
}

puis 374–381

$sql = 'DELETE FROM ' . $this->table . '
    WHERE expires_at < ' . (int) $now . $where_type;
$this->db->sql_query($sql);

Le problème
Le principe du nettoyage aléatoire est acceptable. Mais la suppression est sans limite

DELETE ... WHERE expires_at < NOW

Si ton forum subit une attaque importante et accumule plusieurs centaines de milliers de challenges expirés, un heureux tirage à 1 % peut déclencher une énorme suppression SQL.
Cela peut provoquer :

• verrouillage ;

• forte charge CPU/IO ;

• temps d'exécution important ;

• ralentissement du forum ;

• timeout selon le SGBD.

La solution serait de faire des suppressions par lots, 500 à 1 000 lignes maximum par passage. Ou, selon le SGBD supporté, utiliser directement une suppression limitée lorsque c'est correctement abstrait par phpBB.

$sql = 'SELECT challenge_id
    FROM ' . $this->table . '
    WHERE expires_at < ' . (int) $now;

$result = $this->db->sql_query_limit($sql, 500);

$ids = array();

while ($row = $this->db->sql_fetchrow($result))
{
    $ids[] = $row['challenge_id'];
}

$this->db->sql_freeresult($result);

if ($ids)
{
    $sql = 'DELETE FROM ' . $this->table . '
        WHERE ' . $this->db->sql_in_set('challenge_id', $ids);

$this->db->sql_query($sql);
}

3/ La table peut grossir fortement pendant une attaque

Le point intéressant est ici :

$challenge = array(
    'challenge_id' => bin2hex(random_bytes(16)),
    ...
);

puis

INSERT INTO hucial_antibot_challenges

Le challenge dure 20 minutes, donc un attaquant peut multiplier les sessions et provoquer :
session 1 → challenge
session 2 → challenge
session 3 → challenge
...
session N → challenge

Le challenge est bien lié à la session [incode]'session_id' => (string) $this->user->session_id,[/incode] ce qui est correct du point de vue sécurité. Mais cela signifie aussi qu'un bot qui fabrique énormément de sessions peut faire grossir la table.
Le nettoyage à 1 % n'est pas une protection contre cette situation ; c'est seulement du garbage collection.

4/ Les index sont bons mais peuvent être améliorés

Actuellement

'session_type' => array('INDEX', array('session_id', 'confirm_type')),
'expires'      => array('INDEX', 'expires_at'),
'ip_created'   => array('INDEX', array('ip_hash', 'created_at')),

Mais les requêtes importantes sont

WHERE session_id = ?
AND confirm_type = ?
AND expires_at >= ?
AND solved = 0
ORDER BY created_at DESC

et

WHERE ip_hash = ?
AND created_at >= ?

Le deuxième est très bien couvert. Le premier pourrait bénéficier d'un index composite supplémentaire :

'session_lookup' => array(
    'INDEX',
    array('session_id', 'confirm_type', 'solved', 'expires_at', 'created_at')
),

Il faut éviter de multiplier inutilement les index sur une table qui reçoit beaucoup d'INSERT.

Merci d’avoir pris le temps de regarder l’extension en détail. J’ai repris tes remarques une par une.

Il y a des choses que je retiens, notamment le nettoyage SQL par lots et le fait de mieux borner la création des challenges. Là-dessus, oui, ce sont de bonnes pistes et je vais les intégrer au durcissement.

Par contre, sur certains points, je pense que tu vas un peu trop vite entre « j’ai trouvé un comportement qui pourrait être amélioré » et « j’ai démontré une vulnérabilité ».

Sur le replay du challenge

C’est surtout là que je ne suis pas d’accord avec la conclusion telle qu’elle est formulée.

Tu pars de ceci :

if (!empty($this->challenge[‘solved’]))
{
$this->solved = true;
return false;
}

et tu en déduis qu’un challenge résolu peut être rejoué pendant 20 minutes.

Mais ce bout de code, pris tout seul, ne démontre pas ça.

Dans phpBB, un CAPTCHA résolu n’est justement pas forcément supprimé immédiatement au moment où sa réponse devient correcte. Il faut tenir compte du cycle complet du formulaire.

Par exemple, dans le processus d’inscription de phpBB, le CAPTCHA est validé, l’utilisateur est créé, puis phpBB appelle explicitement :

$captcha->reset();

avec d’ailleurs le commentaire :

// Okay, captcha, your job is done.

Donc la vraie question n’est pas simplement de savoir si [c]solved = 1[/c] reste quelques instants en base.

La vraie question est :

après l’exécution normale de l’action protégée et le passage par [c]reset()[/c], est-ce que le même challenge peut réellement être présenté une deuxième fois et accepté pour effectuer une nouvelle action ?

C’est ça qu’il faudrait démontrer.

Dans ton audit, tu montres l’état [c]solved[/c], mais tu ne montres pas le scénario complet qui permettrait :

• []de résoudre le challenge ;
  []de terminer une première action protégée ;
  []de conserver [c]hucial_ab_id[/c] et [c]hucial_ab_sig[/c] ;
  []de lancer une deuxième action ;

• et de faire accepter à nouveau le même challenge.

Si ce scénario est reproductible, alors oui, il y a quelque chose à corriger.

Mais pour classer directement ça en gravité haute, il faut démontrer le replay réel, pas seulement constater qu’un état [c]solved[/c] existe.

Et il faut aussi suivre [c]reset()[/c] dans l’extension elle-même et dans chacun des contextes concernés. Inscription, connexion, publication et signalement ne doivent pas être mis dans le même sac sans suivre leur cycle complet.

Ça n’empêche absolument pas d’aller plus loin dans le durcissement.

Je peux très bien ajouter un état de consommation définitive, ou faire en sorte que le challenge soit invalidé de manière atomique lorsqu’il a réellement terminé son rôle.

Mais dans ce cas, je parlerais d’un durcissement anti-replay supplémentaire tant qu’un replay exploitable n’a pas été reproduit.

Pour le garbage collector

Là, je te rejoins davantage.

Une suppression de toutes les entrées expirées en une seule requête peut effectivement devenir coûteuse si la table a énormément grossi.

Passer sur quelque chose comme :

SELECT challenge_id
…
LIMIT 500

puis supprimer uniquement ces identifiants est plus prévisible.

Ça borne le travail effectué lors d’un passage du GC et évite qu’une seule requête ait soudainement beaucoup de lignes à supprimer.

Donc oui, je vais probablement modifier ce point.

Par contre, je parlerais plutôt ici de robustesse sous forte charge que d’une faille de sécurité en elle-même.

Pour la croissance de la table

Même chose : le raisonnement est valable, mais il faut distinguer le risque théorique de l’impact réellement mesuré.

Un système qui stocke des challenges côté serveur peut évidemment recevoir beaucoup de créations si quelqu’un génère énormément de sessions.

Le GC ne doit effectivement pas être la seule réponse à ça.

Je trouve donc pertinent de prévoir notamment :

• []un nombre maximal de challenges actifs pour une session et un type donné ;
  []la réutilisation d’un challenge encore actif au lieu de créer systématiquement une nouvelle ligne ;
  []un bornage supplémentaire des créations sur une fenêtre de temps ;
  []un nettoyage progressif des anciennes entrées.

Ça, c’est du durcissement utile.

Par contre, dire simplement :

session 1 = challenge
session 2 = challenge
session N = challenge

ne suffit pas encore à mesurer la capacité réelle d’un attaquant à saturer la table.

Il faudrait également mesurer le coût de création des sessions, le débit atteignable, la croissance réelle de la table et l’impact sur MariaDB.

Pour l’index composite proposé

C’est le point sur lequel je serais le plus prudent.

Tu proposes :

(session_id, confirm_type, solved, expires_at, created_at)

pour une requête qui contient notamment :

expires_at >= ?
ORDER BY created_at DESC

Le problème, c’est qu’avec un index B-tree MySQL/MariaDB, une fois qu’on arrive sur une condition de plage comme :

expires_at >= ?

mettre ensuite [c]created_at[/c] dans l’index ne signifie pas automatiquement que le moteur pourra utiliser cette colonne pour éviter le tri.

Autrement dit, ajouter les cinq colonnes dans le même index ne garantit pas l’optimisation attendue.

Il faut aussi tenir compte du fait que [c]solved[/c] a très peu de valeurs possibles et que chaque index supplémentaire doit être maintenu à chaque [c]INSERT[/c].

Et justement, ici, on parle d’une table où les écritures sont importantes.

Donc avant d’ajouter un gros index supplémentaire, je préfère faire ce qu’on fait normalement dans ce cas-là :

EXPLAIN

sur la requête réelle, avec un volume de données représentatif, puis regarder le plan choisi par MariaDB.

Si le plan montre qu’un nouvel index est utile, on l’ajoute.

Sinon, on évite d’alourdir chaque insertion pour rien.

Concernant les migrations

Là aussi, je ne vois pas vraiment en quoi le fait qu’une migration 1.1.x « ne durcisse pas le schéma » constitue un problème en soi.

Une migration doit faire évoluer le schéma lorsqu’une évolution du code le nécessite.

Si j’ajoute par exemple un état [c]consumed[/c], alors oui, il faudra une migration.

Si les mesures démontrent qu’un nouvel index est utile, il faudra également une migration.

Mais une migration n’a pas à ajouter des colonnes ou des index simplement pour être considérée comme plus « durcie ».

Au final

Je retiens donc plusieurs choses de ton audit.

Le GC par lots est une bonne idée.

Mieux borner la création des challenges également.

Un mécanisme explicite de consommation définitive peut apporter une protection supplémentaire intéressante.

Et je vais faire des tests de charge plus poussés sur la table.

Là où je suis plus réservé, c’est sur la qualification de certains constats.

Il y a une différence entre :

• []une vulnérabilité reproduite ;
  []une possibilité théorique ;
  []une amélioration de robustesse ;
  []une optimisation de base de données.

Pour moi, un audit sérieux doit justement garder cette distinction.

En particulier, annoncer une vulnérabilité de gravité haute sur un replay suppose de montrer le replay complet dans le fonctionnement réel de phpBB, jusqu’à une deuxième action acceptée avec le même challenge.

Sinon, on a identifié une surface à durcir, ce qui est déjà utile, mais ce n’est pas encore la démonstration de la vulnérabilité annoncée.

Donc merci pour les pistes. Il y en a que je vais reprendre.

Mais je vais aussi les vérifier une par une dans le cycle complet de phpBB avant de modifier le code, parce qu’entre un morceau de code qui paraît problématique lorsqu’on le lit isolément et son comportement réel une fois replacé dans tout le flux, il peut y avoir une différence assez importante.

Merci pour ta réponse détaillée.

Après avoir repris le raisonnement au regard de tes explications, je suis d’accord avec plusieurs de tes remarques, notamment sur la nécessité de distinguer clairement une vulnérabilité démontrée d’un risque théorique, d’un point de robustesse ou d’une optimisation.

Sur le replay, je reconnais que ma première formulation était trop catégorique. Le simple fait que solved = 1 reste présent en base ne suffit effectivement pas à démontrer qu’un challenge peut être réutilisé pour effectuer une seconde action. Le point déterminant est bien le cycle complet, et notamment l’appel à reset() après l’action protégée.

Dans ce contexte, je requalifierais donc ce point en durcissement anti-replay potentiel, et non en vulnérabilité haute avérée. L’ajout d’un état consumed pourrait néanmoins constituer une défense en profondeur intéressante, mais ce n’est pas la même chose que corriger une faille démontrée.

Pour le garbage collector, je maintiens en revanche que le nettoyage par lots est une bonne amélioration. Mais je suis d’accord pour le qualifier de problème de robustesse sous forte charge plutôt que de vulnérabilité de sécurité directe.

Même chose pour la croissance de la table : le scénario d'accumulation est théoriquement possible, mais il faudrait effectivement mesurer le débit réel de création des challenges, le coût de création des sessions, l’impact sur MariaDB et la croissance de la table avant de parler d'un véritable scénario de saturation.

Concernant l’index composite, ta remarque est également pertinente. Ajouter systématiquement les cinq colonnes n’est pas forcément une optimisation. Un EXPLAIN sur la requête réelle avec un volume représentatif est clairement la bonne approche avant de modifier le schéma.

Au final, je pense que ton approche est la bonne : vérifier chaque point dans le flux réel avant de modifier le code, plutôt que de corriger préventivement chaque comportement qui peut sembler suspect lorsqu’il est considéré isolément.

Mon audit initial a donc été un peu trop sévère dans la qualification de certains points, en particulier pour le replay. En revanche, les pistes concernant le GC par lots, le bornage des créations et les tests de charge restent à mon avis pertinentes comme mesures de durcissement.

Merci en tout cas d’avoir repris les différents points de manière aussi précise. C’est justement ce type de vérification dans le fonctionnement complet de phpBB qui permet de faire la différence entre une véritable vulnérabilité et une simple surface d’amélioration.

Avec plaisir fred.