Go: Why is DotDotCheck modeled as a complete path-injection barrier?

オープン
#22,214 コメント 3 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

評価

難易度
4/5
見積もり時間
3〜5日
初心者へのやさしさ
50/100
issue の種類
バグ
明瞭さ
おおむね明確
活発さ
静か
技術スタック
go
領域
security

調査の方向性

TaintedPathCustomizations.qll の DotDotCheck (106-122 行目) から始め、次に TaintedPath.go の 31 行目にある GOOD ケースと、Go のパスインジェクションクエリのテストを調べます。false ブランチが SanitizerGuardAsSanitizer を通じて isBarrier になる仕組みを確認し、意図された動作を filepath.Base のガイダンスと比較します。完了とは、モデルと影響を受けるテストの期待値が、サポートされているパストラバーサル対策を一貫して反映していることを意味します。

索引モデルが issue の本文から書いたものです。

説明

question

Description of the issue

The Go go/path-injection query treats !strings.Contains(path, "..") as a complete path-injection sanitizer. This means that when Contains returns false, taint is fully blocked - no additional sanitizer is required.

I'm wondering why this is modeled as a complete barrier, because checking for the absence of ".." does not prevent absolute-path attacks. For example, /etc/passwd does not contain "..", passes the guard, and can read files outside any intended directory.

The query help seems to agree - it says this approach is "only suitable if the input is expected to be a single file name", and also warns that user-controlled paths "may be absolute paths". But DotDotCheck doesn't distinguish between single-component inputs and multi-component paths.

The existing test case at TaintedPath.go line 31 marks this pattern as GOOD with the comment "This can only read inside the provided safe path", but tainted_path = "/etc/passwd" would still pass the check and escape any safe path.

Affected sanitizer

DotDotCheck in TaintedPathCustomizations.qll (lines 106-122) matches strings.Contains(p, "..") and declares the false branch as a complete sanitizer guard. Through SanitizerGuardAsSanitizer, this becomes a full isBarrier node.

Steps to reproduce

func handler(w http.ResponseWriter, r *http.Request) {
    p := r.URL.Query().Get("file")
    if !strings.Contains(p, "..") {
        data, _ := ioutil.ReadFile(p)  // 0 alerts with DotDotCheck enabled
        w.Write(data)
    }
}

Question

Given that !strings.Contains(p, "..") only blocks one specific attack vector and not absolute paths, would it be more appropriate to model it as a non-barrier (or at least not a complete one), similar to how filepath.Base is documented as "not a sanitizer for path traversal" in mime.multipart.model.yml?

Environment

  • CodeQL CLI 2.25.6
  • CodeQL repository commit: f6f45d1536312f53eed079868e344a5906bf3d72
  • Go 1.22.12 on Linux/amd64
主要言語
CodeQL
スター
10.1k
フォーク
2.1k
平均マージ
2日 10時間
マージ済み PR(30日)
134

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

github/codeql のほかの issue

github/codeql の issue をすべて見る

似ている issue

Security の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。