セキュリティレビューで見つかった問題の修正 - #4
Merged
Merged
Conversation
公開後にコードを読み直して見つかったもの。動かして確かめたうえで直している。 **TOOL_SCOPES に書き忘れたツールが未認証で開く（fail-open）。** `TOOL_SCOPES.get()` は 「公開してよい」と「表に無い」の両方で None を返していた。write ツールを足して表への 追記を忘れると、誰でも呼べる状態で静かに動き続ける。import 時に登録済みツールと 突き合わせ、食い違えば起動しないようにした（実際に1件抜いて、起動を拒むことを確認）。 **scope 検査が JSON-RPC のバッチを見ていない。** 配列は isinstance(payload, dict) を 素通りして検査されずに流れていた。いまの SDK が配列を 400 で弾くので実害は無かったが、 守っていたのは下流のふるまいであってこちらではない。バッチを通す SDK になった瞬間に 認可が丸ごと迂回される。中の全 tools/call を見て一番厳しい要求を採る。 **download_file が SSRF になっていた。** S3 保存では InvenioRDM が本文に署名済み URL を 入れて返すため、本文が「http…X-Amz-Signature…」なら追う作りだった。同じ形は**中身が URL のファイルを1つ上げる**だけで作れるので、任意の URL を取りに行かせ、応答まで 受け取れた。応答の長さが登録サイズと違うときだけ署名済み URL として読むようにした。 ファイル本体は必ず自分の大きさで返るので、これで本体と区別がつく。設定は増やさない。 **recid やファイル名がパス片として符号化されていない。** quote() の既定は "/" を残すので `../` を含む値が別のエンドポイントに届いた。_seg()（safe=""）に揃えた。通常の recid は 素通り、`../../users/1` は封じられることを確認。 **要求本文が無制限に読まれる。** 認可の判定に本文が全部要るので、1本の POST で メモリを食い潰せた。MCP_MAX_REQUEST_BYTES（添付上限の2倍）を設け、超えたら 413。 **search_records がクエリを URL エンコードしていない。** `&size=10000` を含むクエリが InvenioRDM への要求に別のパラメータとして差し込まれていた（stdio 版は元から正しい）。 **バッチ拒否時の監査がずれる。** 先頭の要素を載せていたので、実際に拒否したツールと 食い違っていた。チャレンジが持つ名前を優先する。 併せて、stdio 版の add_file(source_path=...) が「プロセスの読めるファイルを何でも読む」 ことを SECURITY に明記した。設計どおりではあるが、呼ぶのは言語モデルなので プロンプトインジェクションの的になる。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q1qSnUV2jao6JRp8KPwXCd
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
公開後にコードを読み直したものです。実際に動かして再現・確認したうえで直しています。
直したもの
TOOL_SCOPES書き忘れで未認証公開（fail-open）download_fileの SSRFhttp…X-Amz-Signature…のファイルを1つ上げれば、サーバに任意の URL を取らせ応答を受け取れるrecid/ ファイル名がパス片として符号化されていない../で別のエンドポイントに届くsearch_recordsのクエリ未エンコード&size=10000がパラメータとして差し込めるdownload_fileの SSRF をどう閉じたかS3 保存では InvenioRDM が本文に署名済み URL を入れて返すので、その形を追う必要があります。ただ同じ形は利用者がファイルの中身として作れます。
登録サイズと突き合わせて解きました。ファイル本体は必ず登録された大きさで返るので、応答の長さが登録サイズと一致するなら、中身が署名済み URL の形をしていてもそれは本体であり、追いません。長さが違うときだけ署名済み URL として読みます。ホストの許可リストのような設定を運用者に増やさずに閉じられます。
fail-open を構造で潰す
TOOL_SCOPESと登録済みツールを import 時に突き合わせ、食い違えば 起動しません。実際にdelete_recordの行を抜いた版を作り、起動を拒むことを確認しています。確認したこと
payload_too_largeの deny が残る_seg("abc-123")は素通り、_seg("../../users/1")→..%2F..%2Fusers%2F1_qs(q="x&size=10000", size=3)→?q=x%26size%3D10000&size=3mkdocs build --strict警告ゼロ版について
v0.0.1は打った後なので、変更は[Unreleased]に日英で記録しました。0.x ではマイナー番号が破壊的変更を運ぶ方針なので、これらは互換を壊さない修正として0.0.2に載ります。ただしTOOL_SCOPESの検査は起動を拒むので、表が食い違っている派生版があれば起動しなくなります。それは意図した挙動です。🤖 Generated with Claude Code
https://claude.ai/code/session_01Q1qSnUV2jao6JRp8KPwXCd