やられアプリ BadTodo - 25 TOCTOU競合

前回：やられアプリ BadTodo - 24.3 NULLバイト攻撃（+XSS） - demandosigno

TOCTOU（トックトゥー）Time of check to Time of use.
ある条件をチェック (check) したあと、その結果を行使 (use) するまでに変更が発生することで引き起こされるバグの一種。競合状態の一例。

まったく同一のIDで登録できてしまう。同一のファイル名でアップロードできてしまう。その結果、本来のユーザーでない人に機密情報が洩れる事故などが想定されます。

というわけで、最初はBadTodoの新規登録部分に目を付けて確認してみました。実際下記の記事でもそこが言及されています。
www.itmedia.co.jp

しかしながら「Docker版 BadTodo Ver 2.1.0」では新規登録部分に問題は見つけられませんでした。（確認漏れの点がある気がしますので引き続き探します）
「Ver 1.1.0」の方には問題がありましたので順に記載します。

BadTodo Ver 2.1.0 に対して

新規ユーザー登録時に、同一ユーザー名でほぼ同時に登録作業を行うという方法で確認しました。

ab コマンド (Apache Bench)を使います。Apacheに標準で付いているWEBサーバの性能を計測するためのコマンドです。
GETリクエストの場合。
$ ab -c 20 -n 40 "http://{検査対象アドレス}/todo/adduser.php"
POSTリクエストの場合。
$ ab -c 20 -n 40 -p ab_post_data.txt -T "application/x-www-form-urlencoded" "http://{検査対象アドレス}/todo/adduser.php"

-c : concurrency 同時実行（並列数）
一度に実行する複数のリクエスト数。デフォルトは一度に一つのリクエスト。
-n : requests リクエスト数
実行するリクエスト数。デフォルトは単一のリクエストで、これは通常、偏ったベンチマーク結果に繋がります。
今回の例は20スレッドで40リクエストを送信。
-p : POSTファイル
POST Bodyパラメータを記載したファイル。同時に -T オプションの指定も必要。
今回の例 ab_post_data.txt の中身。 id=toctou2&pwd=toctou&email=toctou2%40example.com&iconfname=656b26f258d99-man1.png
-T : content-type
POST/PUTの際のContent-type headerの指定。今回は application/x-www-form-urlencoded。デフォルトは text/plain。

$ ab -c 20 -n 40 -p ab_post_data.txt -T "application/x-www-form-urlencoded" "http://{検査対象アドレス}/todo/adduser.php"
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
～（後略）～

ベンチマーク結果が出ますが、それ自体は不要なため省略。

データベースを確認すると、増えたのは１件のみで重複はありませんでした。

ソースコード（adduser.php）を見ると

<?php
～（前略）～
function adduser($app, $userid, $pwd, $email, $icon, $super) {
  $errmsg = array();
  try {
    $dbh = dblogin();
    $dbh->beginTransaction();

    $sql = "SELECT COUNT(*) FROM users WHERE userid=?";
    $sth = $dbh->prepare($sql);
    $sth->execute(array($userid));
    $count = $sth->fetchColumn();
    if ($count > 0) {
      $errmsg[] = 'ユーザIDが重複しています';
    }
    $sql = "SELECT COUNT(*) FROM users WHERE email=?";
    $sth = $dbh->prepare($sql);
    $sth->execute(array($email));
    $count = $sth->fetchColumn();
    if ($count > 0) {
      $errmsg[] = 'メールアドレスが重複しています';
    }
    if (! empty($errmsg)) {
      $dbh->rollBack();
      return $errmsg;
    }

    rename("temp/$icon", "icons/$icon");
    @unlink("temp/_64_$icon");   // 縮小画像を削除しておく 2023/1/5
    $sql = "SELECT MAX(id) FROM users";
    $sth = $dbh->query($sql);
    $maxid = $sth->fetchColumn();
    
    $sql = 'INSERT INTO users VALUES(?, ?, ?, ?, ?, ?)';
    $sth = $dbh->prepare($sql);
    $rs = $sth->execute(array($maxid + 1, $userid, $pwd, $email, $icon, $super));
    $dbh->commit();
  } catch (PDOException $e) {
    $app->addlog('クエリに失敗しました: ' . $e->getMessage());
    if (isset($dbh)) {
      $dbh->rollBack();
    }
    return array('只今サイトが大変混雑しています。もうしばらく経ってからアクセスしてください');
  }
  return array();
}

登録時にユーザーIDまたはメールアドレスが既に存在する場合はエラー。そうでなければMAX(id)に +1 して新規登録、となっています。
例えば次のような流れで登録されたとすると双方エラーとならないため、ダブって追加されるはずです。

下記はAdminerから手動で追加してダブらせた例。

そうならないようにデータベースにはトランザクションという仕組みがあります。
MySQL :: MySQL 8.0 リファレンスマニュアル :: 13.3.1 START TRANSACTION、COMMIT および ROLLBACK ステートメント

DBMSに対して複数のSQLを送る際、１つ以上のSQL文をひとかたまりとして扱うよう指示することができます。
「一部だけが実行されることはあってはならない、途中で分割不可能なもの」として取り扱います。
・停電などでトランザクションの処理が中断されたり
・トランザクションの途中で、他の人の処理が割り込んだ場合
自動的にロールバックが行われます。
トランザクションに含まれる複数のSQL文が、DBMSによって不可分なものとして扱われる性質のことをトランザクションの原子性(atomicity)といいます。
（スッキリ分かるSQL入門。第1版第9章より）

今回の場合、
$dbh->beginTransaction(); と
$dbh->commit();
の間に「IDチェック」と「新規追加」のSQLがあり、両方が終わるまで内部でロックされ他のトランザクションから分離されます。仮に自分が読み書きしている行を他人がロックしている場合、その相手のトランザクションが完了するまで自分は待たされます。そして何か問題があれば
$dbh->rollBack();
されます。
ロックがたくさん発生するとDBの動作が遅くなってしまう点には注意が必要です。

検査実行時のクエリを確認すると「START TRANSACTION」から「commit」まで一連の流れとなっています。

1047 Connect  root@172.18.0.4 on todo using TCP/IP
1047 Query    SET NAMES utf8
1047 Query    SELECT COUNT(*) FROM session
1047 Query    SELECT data, expire FROM session WHERE id='17a2ebf0fbbde9da0dd4fcd25aea4483'
1048 Connect  root@172.18.0.4 on todo using TCP/IP
1048 Query    SET NAMES utf8
1048 Query    START TRANSACTION
1048 Query    SELECT COUNT(*) FROM users WHERE userid='toctou'
1048 Query    SELECT COUNT(*) FROM users WHERE email='toctou@example.com'
1048 Query    SELECT MAX(id) FROM users
1048 Query    INSERT INTO users VALUES('5', 'toctou', 'toctou', 'toctou@example.com', '656b26f258d99-man1.png\n', '0')
1048 Query    commit

問題があるような流れの場合は「rollback」されています。（下記はそもそも一つ目が登録されてしまった後のため「重複エラー」の方でひっかかっているはずですが）

1070 Query    START TRANSACTION
1073 Query    START TRANSACTION
1071 Quit
1070 Query    SELECT COUNT(*) FROM users WHERE userid='toctou'
1073 Query    SELECT COUNT(*) FROM users WHERE userid='toctou'
1072 Query    SELECT data, expire FROM session WHERE id='6d2e402d214e47eddd37f106b7bd37b0'
1069 Query    SELECT COUNT(*) FROM users WHERE email='toctou@example.com'
1070 Query    SELECT COUNT(*) FROM users WHERE email='toctou@example.com'
1073 Query    SELECT COUNT(*) FROM users WHERE email='toctou@example.com'
1069 Query    rollback
1073 Query    rollback

その他、トランザクションに関係する設定を確認してみました。
自動コミットモードが有効であるか。

> SELECT @@autocommit;
+--------------+
| @@autocommit |
+--------------+
|            1 |
+--------------+

デフォルトでは、MySQL は自動コミットモードが有効になった状態で動作します。つまり、特にトランザクション内にない場合、各ステートメントは START TRANSACTION および COMMIT で囲まれているかのようにアトミックです。 ROLLBACK を使用して効果を元に戻すことはできませんが、ステートメントの実行中にエラーが発生した場合、ステートメントはロールバックされます。
MySQL :: MySQL 8.0 リファレンスマニュアル :: 13.3.1 START TRANSACTION、COMMIT および ROLLBACK ステートメント

トランザクション分離レベル

> SELECT @@GLOBAL.tx_isolation, @@tx_isolation;
+-----------------------+-----------------+
| @@GLOBAL.tx_isolation | @@tx_isolation  |
+-----------------------+-----------------+
| REPEATABLE-READ       | REPEATABLE-READ |
+-----------------------+-----------------+

REPEATABLE-READ : 「ファントムリード」発生の恐れあり。
ファントムリード：2回の SELECT文の間に、他の人が INSERT文で行を追加すると、2回の SELECTで結果行数が変わってしまう副作用。1回目の検索結果の行数に依存するような処理を行う場合に問題となることがある。

その他。「id カラム、userid カラム」に一意制約(UNIQUE)がありません。排他制御が十分でない場合、id の重複が発生する可能性があります。（実際、前述の「手動で追加してダブらせた例」では id, userid を重複させて登録できています）

MariaDB [todo]> show columns from users; (> DESC users; でもよい)
+--------+--------------+------+-----+---------+-------+
| Field  | Type         | Null | Key | Default | Extra |
+--------+--------------+------+-----+---------+-------+
| id     | int(11)      | NO   |     | NULL    |       |
| userid | varchar(64)  | NO   |     | NULL    |       |
| pwd    | varchar(6)   | NO   |     | NULL    |       |
| email  | varchar(64)  | NO   |     | NULL    |       |
| icon   | varchar(128) | NO   |     | NULL    |       |
| super  | int(11)      | NO   |     | 0       |       |
+--------+--------------+------+-----+---------+-------+
 
一意制約を追加する場合。
今回の場合は「主キー」もないため id の方を主キーとして追加します。
MariaDB [todo]> ALTER TABLE `users` ADD PRIMARY KEY(id);
MariaDB [todo]> ALTER TABLE `users` ADD UNIQUE(userid);
MariaDB [todo]> show columns from users;
+--------+--------------+------+-----+---------+-------+
| Field  | Type         | Null | Key | Default | Extra |
+--------+--------------+------+-----+---------+-------+
| id     | int(11)      | NO   | PRI | NULL    |       |
| userid | varchar(64)  | NO   | UNI | NULL    |       |
| pwd    | varchar(6)   | NO   |     | NULL    |       |
| email  | varchar(64)  | NO   |     | NULL    |       |
| icon   | varchar(128) | NO   |     | NULL    |       |
| super  | int(11)      | NO   |     | 0       |       |
+--------+--------------+------+-----+---------+-------+
 
これによりダブって追加されるとエラーになる。
MariaDB [todo]> INSERT INTO `users` (`id`, `userid`, `pwd`, `email`, `icon`, `super`)
    -> VALUES ('5', 'toctou', 'toctou', 'toctou@example.com', '656b26f258d99-man1.png', '0');
ERROR 1062 (23000): Duplicate entry '5' for key 'PRIMARY'
 
元に戻す。
MariaDB [todo]> ALTER TABLE `users` DROP PRIMARY KEY;
MariaDB [todo]> ALTER TABLE `users` DROP INDEX `userid`;