Heroku上のPostgreSQLデータをバックアップするスクリプトを以前書いて使っていたのですが、あるときbrakemanを実行すると、以下のheroku pgコマンド実行部分でコマンドインジェクションのおそれがあると警告されました。
# Rubyスクリプトの一部（HEROKUはherokuコマンドのパス）
...
`#{HEROKU} pg:backups capture --app #{app}`
info = `#{HEROKU} pg:backups:info --app #{app}`
...
# brakeman出力より
Confidence: Medium
Category: Command Injection
Check: Execute
Message: Possible command injection
Code: `#{"/usr/local/bin/heroku"} pg:backups:info --app #{app}`
File: heroku-pgbackup-downloader.rb
Line: 26
Confidence: Medium
Category: Command Injection
Check: Execute
Message: Possible command injection
Code: `#{"/usr/local/bin/heroku"} pg:backups public-url #{backup_id} --app #{app}`
File: heroku-pgbackup-downloader.rb
Line: 38
ローカルでのみ実行するバッチ処理なので緊急ではありませんでしたが、今後のためにRubyから外部コマンドを安全に実行する方法を調べました。
外部コマンドを安全に実行する
結論から言うと、外部コマンドを単に実行するだけならKernel.#system、出力を取り出したいならOpen3モジュールを使います。
修正後は以下になりました。
system(HEROKU, "pg:backups:capture", "--app", app)
info, status = Open3.capture2(HEROKU, "pg:backups:info", "--app", app)
if status.exitstatus != 0
"バックアップリスト取得中にエラーが発生しました。終了します。"
end
ポイントは「シェルを起動しない」ように使うことです。そのためには、第1引数にはスペースやメタ文字などを一切含まない安全なコマンド名「のみ」を渡し、オプションと値は第2引数以降で渡します。
Kernel.#systemやOpen3のcapture2やcapture3を正しく使うことで、シェルで解釈されることなく直接外部コマンドを実行するようになります。
なお、修正後の3行目ではstdoutの実行結果が欲しかったのでOpen3の
capture2を使いましたが、stderrも取り出したい場合はcapture3を使います。
安全でない文字列を実行しないこと
重要なのは、どの方法を使う場合であっても、安全でない文字列をコマンドとして実行したりシェルで実行したりしないことです。
たとえばKernel.#systemを使ったとしても、第1引数に安全でない文字列を渡してしまえば何にもなりません。
バッククォート記法で変数を式展開すると、何かのはずみで変数がユーザー入力由来になる可能性があるので避けましょう（最初はそうでなかったとしても）。今回は外部入力は使っておらず、スクリプト内の定数を変数に与えているので、直接的に危険があるわけではありませんが、それでも普段から安全な書き方を心がけるべきですね。
Rubyには外部コマンドを実行する方法が豊富に用意されていますが、外部からのユーザー入力を扱う可能性のある場合は常にこうした点をチェックしましょう。
参考: §7.7 コマンドラインインジェクション -- Rails セキュリティガイド - Railsガイド
