概要
ところでこのツイートを見てほしい。このソースコードをどう思う？
世界最悪のログイン処理コード。
— はっしー@海外プログラマ🇳🇿元社畜 (@hassy_nz) 2018年8月10日
実際のサービスで可動していたものだとか……https://t.co/C2bG93ZCkj pic.twitter.com/EfVNAEslrn
すごく……セキュリティーホールです……
一応は動いていますが、あまりに問題がありすぎるため、Twitterでも話題になっていました。
問題点は片手に入り切らないぐらいある気がしますが、一つづつ解説していきます。
※元記事のタイトルに記載されていますが、このコードはイントラネット内で動作していたものです。
問題点リスト
1. クライアント上のJavaScriptで書かれている
他の問題点を全部ぶっ飛ばすぐらいの重大な不具合です。
クライアントと言うのはこの場合、サイトにWebブラウザ等でアクセスするユーザーのことを指します。
こういったJavaScriptはHTMLやCSSと一緒にサーバーから送られてきますので、当然ユーザーに読まれます。
すると、わざわざサーバーをハッキングせずともログイン処理の全容がバレバレということになります。
2. Cookieに書き込む認証情報がザル過ぎる
1.により、568行目で「(jQueryにより)Cookieに『loggedin』キーに『yes』値を保存している」ことがバレます。
つまり、「手動でCookieに同じ値をセットすればパスワードを調べずともログインしたことにできる」ことがバレます。
それこそWebブラウザのInspectorで観測すれば一発でバレるクソ要素ですが、1.が無ければ発見が遅れたことでしょう。
……ちなみに真っ当なログイン処理の場合、書き込む情報をランダムなログインIDにしたりJSON Web Token(JWT)にしたりします。
3. 任意のSQLを実行できてしまう
1.によって露見してしまった、超重大な脆弱性です。
544～546行から「apiService.sqlメソッドで任意のSQLを実行できる」ことが分かります。
また、メソッド名から察するに、accoutsはSQLの実行結果を「連想配列の配列」で受け取れるのでしょう。
つまり、JavaScriptのデバッグコンソールからデータベースに対して任意のSQLを送れます。
※「任意」と言いましたがその範囲は想像以上に広く、単に「usersテーブルから全部のデータを取り出せる」他、有名なRDBのコマンドを送って応答を見ることでどのRDBを使用しているかまで察せられます。すると、他のテーブルの情報も根こそぎ取られるだけでなく、ユーザー情報を弄って完全に支配を奪ってしまうことも可能かもしれません。ｱｶﾝ……
4. パスワードを平文で保存している
平文とは、暗号化されていない文のことです。つまり、データベースにパスワードの文字列がそのまま保存されています。
すると、データベースがハッキングされた際、ユーザーが入力したパスワードが全部漏れてしまうことになります。
一般的なWebサイトだと、そうならないようにパスワードをハッシュ関数で暗号化することにより、仮にデータベースをハッキングされてもパスワードがバレないようにしますが、この場合はそうじゃないということですね。
さてここで1.と3.を思い出してください。これらを合わせると、「apiService.sql("SELECT username, password FROM users");」とすれば全ユーザーのユーザー名とパスワードが取得できます。
5. なぜかユーザー情報を全部取得している
4.までならまだ、「SELECT count(username) FROM users WHERE username=${username} AND password=${password}」とでもすればユーザーの存在有無は判定できるでしょう。しかし今回の場合、毎回全ユーザーの全情報を取得しています。
ユーザー名とパスワードが一致したデータだけ取り出せばいいので全ユーザーを取得する必要はないですし、ユーザー名とパスワードしか見ていないので全情報を取得する必要もないです。
おまけに、1.により上記処理がクライアント(Webブラウザ)で実行されますので、ユーザーに無駄なメモリを使わせることにも繋がります。ゾッとしますね……。
その他のおかしな点
- SQLを直接文字列を入力して実行→今回のケースではSQLインジェクションすらできないほどクソだったのでさほど影響はないですが、一般的に、「SQLに任意の文字列を入力できる」はバグの温床です
-
if ("true" == "true")やif (authenticated === true)も相当トチ狂っていますが、重大な脆弱性ってほどでもないかと…… - どこからどこまでもクソコードな癖に、「===」演算子を使うことで暗黙の型変換をキッチリ回避しているのが謎です
まとめ
問題点1.が無くても簡単にハッキングできるログイン処理でしたが、問題点1.が全部持っていった感があります。
また、問題点5.の処理が「あまりに脳筋なためにSQLインジェクションできない」といった意味不明な結果になったのは笑いましたｗ
なお、元のソースコードをマシになるように手直ししていくと、たぶん原形を留めなくなるので省略します。