どうも、社会人歴2年目のオチラルです。
初ブログ記事です。
この記事は、フューチャー Advent Calendar 2021の17日目の記事で、昨日は@hichikaさんのGoで多段のファイル変換処理をしてみたでした。
##はじめに
タイトルの通り、GORMでSQLインジェクションが起こりうるコードを書いてしまいました。
幸いテスト期間中に発覚したので大事にはなりませんでしたが、原因究明中に公式ドキュメントがいけてないなと思ったので同じミスを他の人が侵さないように世のため人のため、~~OSSコントリビュート実績解除チャンスだとウキウキで、~~公式ドキュメントにコントリビュートした話をします。
##GORMとは
GORMは公式ドキュメントによると、
デベロッパーフレンドリーを目指した、Go言語のORMライブラリです。
とあります。
読んで字の如く、Go言語で書かれたORMライブラリです。
ORMとは、Object Relational Mappingの略で、Objectはオブジェクト指向のオブジェクトで、RelationalはRDB（リレーショナルデータベース）のRで、オブジェクトをRDBにマッピング（データ変換）することを言います。
今回、ORMはそこまで重要ではなく、ただGo言語用のDB操作ライブラリがGORMなんだと言う認識で良いです。筆者の認識がそこで止まっている
##SQLインジェクションとは
IPA(情報処理推進機構)でも、安全なウェブサイトの作り方第1章の一番最初に説明されているほど、メジャーな脆弱性です。セキュリティに疎い開発者でも最低限知っておきたい脆弱性の1つです。
仕組みは単純で、例えば次のSQL文があり、ユーザーからの入力をinput1、input2だとします。
select * from users where id = 'input1' and password = 'input2'
ユーザーからの入力をSQLにそのまま埋め込んでしまうと、例えばinput2が、1' or '1'='1と入力された場合の次のSQLになる危険性があります。1
select * from users where id = 'input1' and password = '1' or '1'='1'
// select * from users where (id = 'input1' and password = '1') or '1'='1'
// select * from users where false or true
// select * from users where true
// 本当は1件もレコードが見つからない（ユーザが見つかりません）になってほしいのに、全レコードが取得される
これは常に真となり、ユーザーテーブルの中身がすべて攻撃者に見えてしまう/認証を不正に突破してしまう危険性があります。
##SQLインジェクション対策
SQLインジェクション対策も単純で、ユーザーからの入力をエスケープすれば良いです。（他にも対策すべき点はありますが、割愛します）
上記の例だったら、
select * from users where id = 'input1' and password = '1\' or \'1\'=\'1'
と、あくまで、入力を一つの文字列として扱えば良いです。
そして、SQL操作ライブラリはSQLインジェクションの危険性を十分承知してるので、一般的にプレースホルダ（事前に文字を埋め込む）を使い、ユーザーからの入力をバインド出来るように（変数を埋め込む）してます。次のようなSQLを準備して、後からバインドします。
select * from users where id = ? and password = ?
上記のSQL文の内、？がプレースホルダです。変数をバインドする方法はSQL操作ライブラリによって違いますが、基本的にバインドする際に必要な型変換、エスケープ処理を自動的に行ってくれます。
そのため、開発者はユーザーからの入力をSQLにバインドする際、SQL操作ライブラリの用意してくれたプレースホルダを使うことが推奨されます。（ないとは思いますが、どうしても手動でバインドしたい場合は自分でエスケープ処理をする。）
##GORMのプレースホルダとSQLインジェクションを起こした操作
公式ドキュメントによると、GORMでは、以下のように、各操作を行う引数に、？のプレースホルダのある文字列を入れ、そのすぐ後にバインドする変数を書くことで適切にインジェクション対策されたSQLを書けるようです。
また、構造体を用いることも可能です。
あえて、公式の例題にエスケープしなければいけないinputを入れて記載します。
db.Find(&user, "name = ?", "1' or '1'='1")
// SELECT * FROM users WHERE name = '1\' or \'1\'=\'1'
db.Raw("SELECT name, age FROM users WHERE name = ? Order by age", "1' or '1'='1")
// SELECT name, age FROM users WHERE name = '1\' or \'1\'=\'1' order by age
db.Table("users").Select("name,age").Where("name = ?", "1' or '1'='1").Order("age")
// SELECT name, age FROM users WHERE name = '1\' or \'1\'=\'1' order by age
// Struct
// AgeはInt型になってるので、インジェクションする値を入れられない
db.Where(&User{Name: "1' or '1'='1", Age: 20}).First(&user)
// SELECT * FROM users WHERE name = '1\' or \'1\'=\'1' AND age = 20 ORDER BY id LIMIT 1;
あえて、プレースホルダを使わないで、インジェクションが起きるようにgormを書くと、以下のようになります。
userInput := "1' or '1'='1"
db.Find(&user, "name = '"+ userInput + "'")
// SELECT * FROM users WHERE name = '1' or '1'='1'
sql := "SELECT name, age FROM users WHERE name = '" + userInput + "' Order by age"
db.Raw(sql)
// SELECT name, age FROM users WHERE name = '1' or '1'='1' order by age
上記のコードを見比べると、プレースホルダを使う方がスッキリしているし、公式にもあえてインジェクションが起きる書き方を紹介してないのでほとんどの場合インジェクションが起きるコードを書くことはないと思います。
ユーザーから受け取った入力というのは、抽出の条件になることが多いので、where、joinなどを使うときは素直にプレースホルダを使えば良いです。
gormでは、一部受け取った文字列をエスケープせず、そのままSQLに埋め込む処理がありますが、それはselect、order、group byなどDBカラム名を入れるような処理ばかりで、一般的にユーザーからの入力を入れるような処理ではありません。
ただし、1つだけ例外があり、ユーザーから入力され抽出に使われる可能性があるにも関わらずエスケープされない操作があります。
それが、公式ドキュメントの以下の箇所で
プライマリキーでオブジェクトを取得する
主キーが数値の場合は、 Inline Conditions を使用することで、主キーでオブジェクトを取得できます。
実際のコードとして、以下の通りになります。
db.First(&user, 10)
// SELECT * FROM users WHERE id = 10;
db.First(&user, "10")
// SELECT * FROM users WHERE id = 10;
db.Find(&users, []int{1,2,3})
// SELECT * FROM users WHERE id IN (1,2,3);
こちらは、first、findメソッドなどの引数に数字または、数字に変換出来る文字列を入れると良い感じに、where id = 引数を数字に変換したものにしてくれるよという機能です。
もし、引数がいい感じに数字に変換出来ない文字列の場合はそのままwhere 入力された引数となります。
db.Find(&table, ID)
と書かれていた場合、引数IDの条件分岐として、
- IDが数字 OR 数字に変換出来る文字列
- SELECT * FROM table WHERE table.id = ID
- IDが数字に変換出来ない文字列
- SELECT * FROM table WHERE ID
となる少し分かりにくい仕様があります。（文字列はエスケープされない）
気になった方はgormはOSSなので、該当箇所のコードを確認出来ます。
上記の仕様のため、IDを文字列で扱っているとき、数字に変換出来るのかを確認しないで、
db.Find(&table, ID)した場合、IDに悪意のある文字列が入力された場合、エスケープされず、whereにそのまま入るのでインジェクションが発生します。
今回はこちらのメソッドでインジェクションが発生するコードを書いてしまっていました。
##書いたコード
私のチームでは、基本的にAPIの動作として、受け取ったIDなどのパラメータを用い、SQL文でゴニョゴニョして、いい感じの返り値を返すような構造になっていました。（大体そう）
そして、受け取ったIDがDBに存在するかどうかのバリデーションとして、
db.Find(&table, id)
を走らせ、レコードが存在するかを確認する操作をいくつかのAPIで実行していました。
APIはopenAPIを用いており、goのコードに来た時点で、IDはint型であることが保証されていました。
しかし、今回SQLインジェクションが発生しうるコードになってしまったAPIでは、単一のIDではなく、複数のIDが欲しいAPIになっていました。
そのため、一旦IDを文字列で受け取り、受け取ったIDをスライスし、スライスしたIDで上記のバリデーションを走らせました。整数に変換することを忘れ。。（そもそも文字列のID一覧を正規表現などでバリデーションするべきであった）
そのため、前節で説明したように、SQLインジェクションが発生しうるコードになってしまっていました。
簡単なコードとしては、以下のようになっていました。
// パラメータでIDsを ','区切りで渡していたため、スライスする
// 本来ここで、修正方法①IDsが有効な形をしているかバリデーションしなければいけなかった
IDs := strings.Split(parameter.hogeIDs, ",")
for _, id := range IDs {
// ここでインジェクション発生
// 修正方法②idが文字列になっているため、ここで数字に変換するべきだった（変換不可だったらエラーハンドリング）
db.Find(&table, id)
}
##公式ドキュメントの記載
公式ドキュメントでは、あえて前節では省きましたが、一応、プライマリキーでオブジェクトを取得するの箇所では、
文字列を扱う場合、SQLインジェクションを避けるために十分な注意が必要です。詳細については、 Security セクションを参照してください。
とありました。
しかし、詳細を書いてあるはずの、Securityセクションでは、ユーザーの入力は引数としてのみ使用する必要があります。とだけ記載されていました。（今回の件は引数に入れていたがインジェクションが発生した。）
今回問題のFirstメソッドに関しては、
// will be escaped
db.First(&user, "name = ?", userInput)
// SQL injection
db.First(&user, fmt.Sprintf("name = %v", userInput))
と単一の引数で、プライマリキーでオブジェクトを取得する際の例ではなく、普通のプレースホルダでwhere文を使う例のみ記載されていました。
自分はgormを使う前にドキュメントを軽く読み込む際、securityセクションも読んでいましたが、プレースホルダを使えばよいという記憶しか残っておらずプライマリキーでオブジェクトを取得する際は、パラメータの型をしっかり確認（バリデーション）しようということは頭に残りませんでした。(現在は自分が出したPRがマージされたため記載がある）
そのため、文句の一言を言いたく世のため人のため、ドキュメントのコントリビュートボタンをクリックし、ドキュメント拡充をしました。
##OSSコントリビュート
https://github.com/go-gorm/gorm.io/pull/482
↑が実際に出したコントリビュートPRです。
すでに、マージされており、日本語ドキュメント含め各国ドキュメントに自分の足した文面が追加されています。マージ直後は、各国ドキュメントに僕が追記した文書が英語のまま載っていたのですが、記事公開までに早速日本語訳が付いていました。（マージされて2日ぐらい、日本語訳が最速、記事公開時は日本語訳のみ）
大元に足したのは下記の箇所です。security.mdに件のメソッドを使うとき、inputは型チェックしてエラー返そうねという記載にしました。文法が正しいかは知らないです。
When retrieving objects with number primary key by user’s input, you should check the type of variable.
userInputID := "1=1;drop table users;"
// safe, return error
id,err := strconv.Atoi(userInputID)
if err != nil {
return error
}
db.First(&user, id)
// SQL injection
db.First(&user, userInputID)
// SELECT * FROM users WHERE 1=1;drop table users;
初めてのOSSコントリビュートでしたので、とても慎重に他の人のPR、コミット履歴を穴が空くほど見ながらPRを出しました。
簡単な全体的な流れとしては、
- 公式ドキュメントのcontributeページ、GitHubのreadme.mdを読みルールの確認をする
- 公式ドキュメントのどこにどんな記載を書けば良さそうか考える
- forkしてcloneしたドキュメントのローカルで修正＆コミット＆プッシュ＆PRを出す
- 適切なPRコメントを書き、レビュー/マージを待つ
でした。
1つずつ簡単に振り返り/解説します。
####1.公式ドキュメントのcontributeページ、GitHubのreadme.mdを読みルールの確認をする
ほとんど、OSSはcontributeルールがドキュメント、githubのreadme.mdにあります。
gormの公式ドキュメントは、github管理されており、英語のドキュメントのみgithubにPRを出す形でドキュメント拡充、翻訳はcrowdinというサービスで管理しており、別サイトで翻訳をする形でした。
今回は日本語の翻訳の問題ではなく、英語の大元がいけてなかったので、英語のPRを出すことにしました。
ちなみに、contributeルールは、英語はgithub、翻訳は別サイトで行うということのみでした。
####2. 公式ドキュメントのどこにどんな記載を書けば良さそうか考える
地味に悩みました。query.mdに書くか、security.mdに書くか、そもそも文章で別節で書くか、ただ例文SQLにしれっと追記するかなど。
他の人のドキュメント拡充PRを見たところ、大きな文章を追加してるPRがほぼほぼなく、何行も文章書いてマージされるだろうかというのも疑問でした。
最終的に、security.mdに既存の構成を壊さないように例多めで1節追記しました。（例だったらマージしやすいだろうという予想）
####3. forkしてcloneしたドキュメントのローカルで修正＆コミット＆プッシュ＆PRを出す
これは、OSSコントリビュートのルールですね。forkしてのPRは初めてで新鮮でした。
地味に迷ったのが、ブランチ名です。特にルール決めがなく、他の人のPRを眺めたところ、
- masterのまま
- fix-〇〇みたいな名前のブランチ
- patch-1、patch-2
という名前のブランチになっており、何でもありということがよく分かりました。
ただ、patch-1、patch-2のブランチが個人的にしっくりと来たため勝手に採用して頂きました。（実はドキュメントの誤字も見つけており、PR2回出す予定であったため、数字付けするのが良いなと思った）
このとき、他の人のPR、コミットメッセージなどを見たのですが、結構翻訳PRは出すなというルールを読まないでgorm開発者の「jinzhu」さんに突っ込まれてる人が多いこと、別にどっちでもいいじゃんな文法、誤字修正PRが多いことを知り、OSSコントリビュートPRってもっと気軽に出していいんだなーと実感しました。
コミットメッセージと、PRタイトルは、security.mdを更新したので、他の人を真似て、単純に「update security.md」にしました。
余談ですが、gormのドキュメントはhexoで出来ており
$ npm install
$ npm install hexo -g
$ hexo serve
git cloneしたディレクトリで、上記のコマンドを用い、手元で動作確認出来ます。（自分も今回の変更がどう見えるのか確認のためserveしました。）
また、フューチャーの技術ブログである、future tech blogもhexoで出来ており、
oss公開されているので、ブログだったり公式ドキュメント作りたい！という方はgithubにコードがあるので、見てみると楽しいです。
####4. 適切なPRコメントを書き、レビュー/マージを待つ
ほとんどのgitプロジェクトは、PRを出す際に、デフォルトでPRに必要なチェックリストテンプレートがあるかと思います。今回は、「このPRは翻訳PRじゃない」というチェックを付けて、どういったPRなのかの解説を書くだけでした。
他の方のPRと比べて少しだけ文面多めだったので、何故PRを出したのか出来る限り丁寧に説明しました。
拙い英語ですが、「通じればええやろ」の精神で、既存の文章などを参考に書きました。
アドベントカレンダーまでにマージされないだろうな、別の記事書こうかなと思っていたところ、即日でマージされてとても嬉しかったです。
##まとめ
今回の件で晴れて、OSSコントリビュートする、公式ドキュメントを拡充すると言った夢を叶えることが出来ました。いつか、ドキュメント以外のOSSに開発コントリビュート、誰も気付いていないようなissueを立てるなどのコントリビュートをしたいと思います。
今までの人生、先人たちが残してくれた神ライブラリ、神ドキュメント、神ブログ記事、神qiita記事などで救われたことが多々あるので、これからは自分が誰かを救う、救いの神コンテンツを創出する側になっていけるよう精進していきたいなと思います。
1人でも「ちょうどgormでSQLインジェクションが起きていたので助かりました」、「インジェクションの勉強になった」「公式ドキュメント拡充って結構簡単なんだな」、「自分も開発中に学んだことをアウトプットしよう」と思った方がいれば幸いです。
ちなみに、今すぐ公式ドキュメントコントリビュートしたい！っていう方は、2021/12/14現在、僕のPRがマージされたこともあり、gorm.ioの日本語ドキュメントの翻訳率が100%（1月前ぐらい）から93%になっていたので、翻訳コントリビュートチャンスです！
13日にPRマージされて14日に記事書いたのですが、記事公開までに僕の英語ドキュメント拡充した分がすでに日本語に翻訳されていました。日本語エンジニアコミュニティはやはりすごい。
明日のアドベントカレンダーは@nadareさんの記事です。
-
1=1;SQL文; --(後ろのSQLをコメントアウト）みたいなインジェクションコードもあります。インジェクション対策テストしたい場合は、1=1のところに様々なパターンのインジェクション出来うるコードを書き、SQL文にsleepを埋め込めばテスト出来ます。 ↩