「世界最悪のログイン処理コード」ではない可能性は無いのか？

先日「世界最悪のログイン処理コード」が話題になった。

世界最悪のログイン処理コード。実際のサービスで可動していたものだとか……https://t.co/C2bG93ZCkj pic.twitter.com/EfVNAEslrn

— はっしー@海外プログラマ🇳🇿元社畜 (@hassy_nz) 2018年8月10日

ただ、開発者がその脆弱性を理解した上で、ちゃんと対処を行っており問題ない状態である可能性はないかを考えてみた。

一番の問題

今回のコードで一番問題になっているのはやはりここだろう。

var accounts = apiService.sql(
  "SELECT * FROM users"
);

生のSQLをサーバーに送って実行した結果をクライアント側で取得できるようにしていることで、第三者があらゆる情報を取得できてしまう脆弱性と、クライアント側にて大量のメモリ使用を強制させて負荷を与えるという問題をたった三行で想像させてしまう恐ろしいコードだ。

でも、ちょっと考え方を変えるとそうではない可能性もあるということが分かる。

SQL文をフィルタリングしている

もしサーバー側で送られてきたSQLをフィルタリングして、許可していないSQLを弾いている場合はデータベースのあらゆる情報をとれてしまうということはない。

そもそもSQLをそのまま実行しているわけではない

SELECT * FROM usersというSQL文を引数として渡しているが、これを必ずしもサーバー側でそのまま実行しているとは限らない。そもそもapiService.sqlというメソッドが引数だけを使って処理をしているとは限らない。

色々総合すると下記の様な感じになっている可能性もある。

JavaScript側

class ApiService {
  sql(query) {
    const response = $.ajax({
      url: "/api/sql",
      type: "get",
      async: false,
      data: {
        username: $("#username").val(),
        password: $("#password").val(),
        sql: query
      }
    });
    return response.result;
  }
}

サーバー側

$methods = [
  "SELECT * FROM users" => "getLoginUser",
  :
];
$this->{$methods[$request["sql"]]}($request);

getLoginUserの中でSELECT id, nickname FROM users WHERE username = '$username' AND password = '$password'みたいなのを実行して結果を返す感じ。

こんな感じであればセキュリティ的にもメモリ的にも全く問題はない。

ただしもちろんAPIのURLを別にすればいいだけでSQL文でフィルタリングする必要性も全く無いし、誰がどう見てもSQLを直接動かしているようにしか見えないので実装的にはとんでもない話ではあると思う。

生パスワードを使ってる問題

次に問題となるのは入力されたパスワードをそのまま使っている、つまりデータベースにも生のパスワードが保存されているのではないか疑惑。これに関しては先程のようにサーバー側で単なるSQLを直接実行しているわけではないという前提であれば疑惑を回避できる。

ただ、account.password === passwordという処理があるせいで、やはり生パスワードを使っていないとおかしいのでは、という話になってしまう。

ただここは一応$("#password").val()は必ずしも#passwordが入力欄とは限らないので、例えば#password_inputに入力されたものを随時サーバーに送って、ハッシュ化したものを#passwordというhiddenに入れておく、という可能性があり、それであれば特にその部分の問題はなくなる。

もちろんこのコードにするためにわざわざ通信して変換しているという処理は理解し難い。

まとめ

一応「世界最悪のログイン処理コード」であることを回避することができるような考え方は不可能ではなさそうだが、そのかわり今度は人としての優しさを失ったコードになってしまいそう。