ANDPADボードチームの原田(tomtwinkle)です。
前回の記事で結構反響いただけたようで注意喚起記事としては名目は果たせたかなと思います。
個人的には過去にもQiitaの自分のブログにもこんな記事
Gormとの破局、そしてFacebook/entとの出会い - Qiita
を書いてるようにGorm自体にはそこまで思い入れはなく、新規にプロダクトを作る際の選択肢で検討しているなら出来ればやめた方が良いと言いたい気持ちが強いのですが……既に採用されているプロダクトなら乗り換えは大変ですよね。
以下はそんな既にGormを利用している皆さんのための内容になります。
これは何？
GormのQuery Conditions関数を用いた実装を行った際に脆弱性につながる実装があった場合Pull RequestのReviewで自動的に指摘しようというやつです。
前回Gormの脆弱性対策に関して「各自注意して」みたいな感じであっさり気味に書いてしまいましたが、実装者がGormに習熟した人ばかりでもないので各自注意していてもうっかり実装してしまう事があります。
もちろんPull RequestのReviewで弾けば良いのですが既知の問題をいちいち人の手で指摘していたらレビュアーの負荷がどんどん増大していってしまいます。 パターンマッチングで解決できる程度の既知の問題に対するPull Request Reviewは自動化するに越したことはありません。
ざっくりとした流れとしては以下の通りです。
- Pull Request --hook--> Github Actions
- returntocorp/semgrep でソースコードをgrepしてパターンマッチングした内容があればメッセージを出す
- reviewdog/reviewdog でSemgrepのメッセージを拾い、Pull Requestにコメントを付ける
Semgrepの有料プランを使用する場合はCI Actionの機能も組み込まれているのでreviewdogは不要です。 ちなみに今回はあくまで前回の続き物として書いていますが、もちろんパターンマッチングで検知できるものならGormの脆弱性以外でも利用可能です。
Semgrep ruleのサンプル
以下はサンプルなので各自自分のプロダクトに合わせて修正してください。
今回チェックする対象は app/hogehoge/database ディレクトリ直下の *.go ファイルだとします。
Gormで危険なConditionの使用方法は
db.Find(&users, id)
のように Find() などの特定の関数名で引数が2つのパターンです。
今回使用するSemgrepは名前の通り単なるgrepなので Find() や Delete() 等の関数で引数が2つであれば全部引っ掛かってしまいます。
それをなるべく避けるためにGolangの慣習にならって引数1つ目が ctx (context.Context)である場合を省くように設定をしていきます。
出来上がったものがこちらです。
.semgrep 配下に rules.yml のようなファイルを作成し以下を記載します。
rules: - id: gorm-conditions-sqli paths: include: - app/hogehoge/database/*.go pattern-either: - pattern-regex: (\s|\.)First\(\s*[^(ctx)][&a-zA-Z0-9]+\s*\,\s*(&|)\w+\s*\) - pattern-regex: (\s|\.)Last\(\s*[^(ctx)][&a-zA-Z0-9]+\s*\,\s*(&|)\w+\s*\) - pattern-regex: (\s|\.)Find\(\s*[^(ctx)][&a-zA-Z0-9]+\s*\,\s*(&|)\w+\s*\) - pattern-regex: (\s|\.)Take\(\s*[^(ctx)][&a-zA-Z0-9]+\s*\,\s*(&|)\w+\s*\) - pattern-regex: (\s|\.)Delete\(\s*[^(ctx)][&a-zA-Z0-9]+\s*\,\s*(&|)\w+\s*\) message: Gorm Query Conditionの第2引数にparameterを指定するのはSQLインジェクションが発生する可能性があり危険です。 https://gorm.io/docs/security.html severity: WARNING languages: - go
以下のようにDockerを利用してlocalで動作確認が出来ます。
docker run \ -v $(pwd):/workdir \ --workdir /workdir \ returntocorp/semgrep \ --severity WARNING --json \ -f /workdir/.semgrep /workdir \ | jq -r '.results[] | "\(.path):\(.start.line):\(.start.col): \(.extra.message)"' \ | sed 's#^/workdir/##'
Github ActionsのWorkflow
.github/workflows 配下にGithub ActionsのWorkflowファイルを作成します。
そのままコピペでも動くと思います。
name: semgrep-with-reviewdog on: [pull_request] jobs: reviewdog: name: reviewdog runs-on: ubuntu-latest steps: - uses: reviewdog/action-setup@v1 with: reviewdog_version: latest - uses: actions/checkout@v2 with: fetch-depth: 0 - name: Run semgrep env: REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | docker run \ -v $(pwd):/workdir \ --workdir /workdir \ returntocorp/semgrep \ --severity WARNING --json -f /workdir/.semgrep /workdir \ | jq -r '.results[] | "\(.path):\(.start.line):\(.start.col): \(.extra.message)"' \ | sed 's#^/workdir/##' \ | reviewdog \ -efm="%f:%l:%c: %m" \ -diff="git diff FETCH_HEAD" \ -level=warning \ -reporter=github-pr-review
脆弱なコードを書いてGithub Actionsを動かしてみる
実際に脆弱なコード書いて動かしてみるとPull RequestのDiffに以下のようにSemgrepのメッセージが表示されるようになります。
最後に
群雄割拠のベンチャーに於いてはプロダクト開発のスピードこそがプロダクト成長の最大の鍵であるのですが、それでプロダクト品質を落としてユーザーが離れてしまっては本末転倒です。 プロダクトが大きくなるにつれてエンジニアリングの難しさは指数関数的に上がっていきがちですがそれをなるべく減らすように努力するのが真の意味でのエンジニアリングだと思っています。
こういった小さな手間をテックファーストで改善していくことで、「スピードと品質」を両方担保していくプロダクトの開発を続けていけるようプロフェッショナルでありつづけエンジニア全体の価値を高めていきたい所です。
現在ANDPADではそのようなメンバーを積極的に募集しています。
「取り敢えず話だけでも聞いてみたい！」みたいな人向けのカジュアル面談の制度もありますので是非話だけでも聞きに来てみてください。
