Laravel 5.8.5でfixされた Unique Rule SQL Injection Warning について調べてみた

元記事

blog.laravel.com

Unique Rule SQL Injection Warning とは?

Laravelで提供しているバリデーションルールにuniqueルールというのがあります。

これはデータベースのテーブルの指定したカラムの値がユニークであるか（一意であるか）をチェックする事が可能です。例えば以下のように実装することで、リクエストで送られてきらemailという値が、usersテーブルでユニークであるか?を簡単に判断する事ができます。

 $request->validate([
     // usersテーブルのlogin_nameカラムで一意チェックを行う
     'email' => 'unique:users'
 ]);

今回報告された「Unique Rule SQL Injection Warning」の内容を読んでざっくり要約すると、uniqueルールで指定したIDのチェックを除外したい場合に使うignoreメソッドの引数に悪意のあるSQL文が渡されるとSQLインジェクションが起きてしまうという内容でした。

The unique rule's "exclude ID" feature is intended to only accept system-generated IDs, such as auto-incrementing IDs or UUIDs generated by your application, which is the only way the documentation demonstrates using the rule

具体的にはignoreメソッドの引数で渡される値はアプリケーション側で生成されるauto-increment属性のIDとか、もしくはUUIDを想定して作られておりドキュメントの例にもそのように書いてあります。

However, if users depart from the documented usage of the feature and allows user controlled data to specify the "exclude ID" value or column, a maliciously crafted request could lead to an SQL injection attack.

しかしながらドキュメントに記載されている用途から外れてignoreメソッドの引数に悪意を持って作成されたSQL文が渡された場合に、SQLインジェクションの攻撃に繋がる可能性があるそうです。

この脆弱性についてはLaravel 5.8.5でFix済みです。

laravel-news.com

また最新のドキュメントにも赤く注意書きとして、「ignoreメソッドにユーザーが制御するリクエストを渡さないで下さい」とわかりやすく書いてあります。

f:id:kikuchi1201:20190322020913p:plain

ref:https://laravel.com/docs/5.8/validation#rule-unique

実際にPRでの変更箇所を見ると、ignoreメソッドに引数で渡されてきたidに対してstr_replaceメソッドを使って,を空文字に置換する処理が追加されてます。

remove commas from values · laravel/framework@da4d4a4 · GitHub

という事なので、あまりないとは思いますが、Laravelでuniqueルールでignoreメソッドを使っている場合に、引数にそのままSQL文を流すみたいな処理を記述する事はやめましょう。

ちなみにというか、こういうの見ると毎回「reddit民ではどんな反応してるんだ?」と思って見に行くのですが、案の定サブレ(subreddit)に上がってました。

www.reddit.com

トップコメントが

I don't like it that they pushed a "silent" fix (no PR, no discussion) in >https://github.com/laravel/framework/commit/da4d4a468eee174bd619b4a04aab57e419d10ff4

と書いてあるように、上記の対応はPRで特に議論されずに、"silent"にfixされた事に不満が上がっているみたいですね。

Seems they realized this and created https://github.com/laravel/framework/pull/27940 ...

またコメントを見ると、もう既にstr_replaceではなく、addslashes関数を使っての処理に置き換わっています。

ただそれに対して

Well, that uses addslashes which is a very dangerous way of escaping data for queries...

と、addslashesをDBクエリーのエスケープに使うのは非常に危険だ..とコメントが上がってます。

[5.8] Strip slashes for unique rule helper by themsaid · Pull Request #27940 · laravel/framework

addslashes関数自体はPR内で議論されているように悪因のあるSQL文を完璧にエスケープができないというか、危険なのでは?という疑問も当然出ていますが、PRのコメントをちゃんと読むと、addslashesメソッドはエスケープする用途で入れているのではなく、あくまでもバリデーションルールでstr_getcsvを呼び出しに、スラッシュを追加するために、addslashes関数を追加したとの事でした。

taylorotwell 2 days ago We aren't adding slashes for the DB query. We are adding slashes for the str_getcsv call in the validator. This has nothing to do with trying to escape data for a DB query.