元記事
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済みです。
また最新のドキュメントにも赤く注意書きとして、「ignoreメソッドにユーザーが制御するリクエストを渡さないで下さい」とわかりやすく書いてあります。
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)に上がってました。
トップコメントが
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.
参考文献
- Unique Rule SQL Injection Warning - The Laravel Blog
- [5.8] Strip slashes for unique rule helper by themsaid · Pull Request #27940 · laravel/framework
- [Improvement] Database Validation Rules (Exists/Unique/ etc.) · Issue #1572 · laravel/ideas
- Unique Rule SQL Injection Warning : laravel
- addslashes() Versus mysql_real_escape_string(), by Chris Shiflett
- 【php】addslashesとmysql_real_escape_stringって何が違うの at softelメモ