Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSPを導入する #9848

Open
Ry0taK opened this issue Feb 9, 2023 · 3 comments · May be fixed by #9863
Open

CSPを導入する #9848

Ry0taK opened this issue Feb 9, 2023 · 3 comments · May be fixed by #9863
Labels
✨Feature This adds/improves/enhances a feature 🔒Security Security related issue/PR

Comments

@Ry0taK
Copy link
Contributor

Ry0taK commented Feb 9, 2023

Summary

最低でもscript-srcbase-uriを全ページで指定し、XSSが存在した場合の軽減策として機能するようにしたい。

Description

CSPを適切に設定することでXSSが存在した場合にある程度影響を軽減できる。style-srcimg-src等は想定していない箇所が壊れる可能性があるので、一旦script-srcbase-uriを指定し、最低限無いよりマシな状況にしたい。
CSPについてはこちらを参照: https://developer.mozilla.org/ja/docs/Web/HTTP/CSP

(良さそうであれば実装してみるので教えてください。)

script-srcに関して

以下の理由により、JavaScriptを全部外部のファイルに分けてscript-src 'self'のみを使う形にするのが一番良さそう

nonceを用いたCSP

nonce形式を用いる場合、リクエストごとにランダムなnonceを生成してクライアントに返す必要がある。
また、そのnonceをそれぞれのscriptタグに付与する必要が出てくるが、@fastify/view/pugには動的にnonceを付与する機能が無いため、pugでテンプレートを描画した後に一度サーバー側でDOMをパースしてそれぞれのscriptタグにnonceを付与する必要が出てくると思われる。
(他にいい実装方法を思いつく方は教えてください。)

ハッシュを用いたCSP

scriptタグ内のコンテンツのハッシュを計算し、それを予めContent-Security-Policyヘッダで指定する必要がある。
この方法だとビルド時 or 起動時に一度だけ計算すればそれ以降はContent-Security-Policyヘッダに付与しておけば良くなる。
ただし、どこか(多分pug)でJavaScriptの内容がminifyされているため、minify後のコンテンツをどうにかして取ってハッシュを計算する必要がある。

script-src 'self'

script-src 'self'を用いる場合、ページ内に埋め込まれたスクリプト (= inline script)を全て外部のファイルに分離する必要がある。(別ファイルにすることによるパフォーマンスの低下は未計測です。)
確認できた限り以下を外部のファイルへ切り分ける必要がありそう。

script.
var VERSION = "#{version}";
var CLIENT_ENTRY = "#{clientEntry.file}";
script
include ../boot.js

(pugはJavaScriptの動的な生成をサポートしていなさそうなので、このインラインスクリプトをどうするか考える必要がある)

script
include ../bios.js

script
include ../cli.js

script.
const msg = document.getElementById('msg');
const successText = `\nSuccess Flush! <a href="/">Back to Misskey</a>\n成功しました。<a href="/">Misskeyを開き直してください。</a>`;

<script src="https://cdn.jsdelivr.net/npm/[email protected]/bundles/redoc.standalone.js" integrity="sha256-WJbngBWN9vp6vkEuzeoSj5tE5saW9Hfj6/SinkzhL2s=" crossorigin="anonymous"></script>

最後のもののみintegrity属性を設定しているので、ここだけ例外的にハッシュを使用する形にしても良さそう

base-uriに関して

確認した限りbaseタグは使用していないため、base-uriは'self'決め打ちで良さそう。

@Ry0taK Ry0taK added the ✨Feature This adds/improves/enhances a feature label Feb 9, 2023
@acid-chicken acid-chicken added the 🔒Security Security related issue/PR label Feb 9, 2023
@acid-chicken
Copy link
Member

ありがとうございます。

とりあえずコンテキストの共有ですが、現在の server/web/views あたりの構造(どのようにバックエンドが HTML を生成してフロントエンドのアセットを配信するか)は aoi 時代からのものからあまり大胆に手を加えていないまま今日に至るので、大胆な変更もやぶさかでなく、見直しの自由度は高いです。

Related to #16, #5709, #8888

例えば

最後のもののみintegrity属性を設定しているので、ここだけ例外的にハッシュを使用する形にしても良さそう

は、ひとまず OpenAPI のドキュメントが見られる状態になることを目的としてシンプルに組み上げていたり(#4351)しているだけで、そもそも特段 CDN で切り出している理由はない認識です。

@Ry0taK
Copy link
Contributor Author

Ry0taK commented Feb 10, 2023

とりあえずコンテキストの共有ですが、現在の server/web/views あたりの構造(どのようにバックエンドが HTML を生成してフロントエンドのアセットを配信するか)は aoi 時代からのものからあまり大胆に手を加えていないまま今日に至るので、大胆な変更もやぶさかでなく、見直しの自由度は高いです。

様々な方法を検討してみたのですが、以下のような理由で大幅な変更は行わずにJavaScriptファイルを外部に分ける形での対応が良さそうでした。

nonceを用いたCSP

@fastify/helmet を用いることによりdecoratorを使って動的なnonceの付与ができそうではあるが、この手法を使った場合毎回HTMLレスポンスが動的に変化してしまうため、キャッシュなどの観点から考えると望ましくない

ハッシュを用いたCSP

テンプレートエンジンを変更し、動的にインラインスクリプトのハッシュ値を計算できると嬉しいという点はありつつ、@fastify/viewがサポートしているテンプレートエンジンの中に当該の機能を持つものが無さそう (あるにはあるが、結局外部のファイルへ分割してそのファイルのハッシュ値を計算しているため、これを行う事によるメリットがあまり無い)

例えば

最後のもののみintegrity属性を設定しているので、ここだけ例外的にハッシュを使用する形にしても良さそう

は、ひとまず OpenAPI のドキュメントが見られる状態になることを目的としてシンプルに組み上げていたり(#4351)しているだけで、そもそも特段 CDN で切り出している理由はない認識です。

こちらのドキュメントに関してなのですが、どうやら 3a7182b で参照していたエンドポイントが削除されているようで、どこからも参照されていない状態になっているようでした。
そのためこのファイルに関しては特別な対応を行わず、別途削除してしまって良さそうな雰囲気があります。

また、検討を進める中で懸念事項として、Misskeyをカスタマイズして外部のJavaScriptを挿入している場合にCSPの追加が破壊的変更となりうるという点に気が付きました。
例えばmisskey.ioにおいては、CloudflareのWeb Analyticsの利用やCloudflare AppsによるJavaScriptの挿入を行っているため、script-src 'self'を追加するとこれらのJavaScriptが動作しなくなります。
そのため、CSPの導入にあたっては設定ファイルで柔軟に変更できるようにしつつ、リリースノートにその旨を明記しておく必要がありそうです。

@eternal-flame-AD
Copy link
Contributor

eternal-flame-AD commented Nov 19, 2024

Googleのアナライザーからのヒント:

Relavant warnings for #9863 on https://csp-evaluator.withgoogle.com/ :

⚠ 'self' can be problematic if you host JSONP, AngularJS or user uploaded files. - Misskey hosts user uploaded files.
⚠ 'unsafe-eval' allows the execution of code injected into DOM APIs such as eval().

先週に自分のインスタンスに最小権限の CSP を実装しました。「unsafe-eval」 を使用せず、初期化が完了した後のインジェクションを防ぐためにすべてのインライン スクリプト/SVG の SRI を初期化時に計算します。

これは Self-XSS ( rel: #14839 ) に対する保護も提供します。これにより、コンソールを開いて情報を漏らすことさえできなくなります。他のドメインにfetch() は許可されません。プロキシされた画像 URL でデータを漏らすには、画像を挿入してデータをエンコードする必要があります。

ただし、これはアップストリームには厳しすぎるかもしれません。これと #9863 の中間が良さそう (インライン スクリプトを事前にハッシュして「unsafe-eval」を削除します)。

I implemented a least-privilege CSP last week on my instance that do not use 'unsafe-eval' (which significantly hampers the effect of CSP) and precomputes during initialization all inline script/SVG SRIs to prevent any injection after initialization is complete.

This also provides good protection against Self-XSS ( rel: #14839 ) . With this you can't even open the console and leak information out: no fetch() will be allowed. You have to insert an image and encode your data to exfiltrate in a proxied image URL.

This might be too strict for upstream though, I think a middleground between this and #9863 is a good idea (maybe pre hash the inline scripts and remove 'unsafe-eval' but don't be too worried about strict CSS safety.


Related commits:

Initial implementation: https://forge.yumechi.jp/yume/yumechi-no-kuni/commits/branch/master/search?q=csp&all=
Configuration: https://forge.yumechi.jp/yume/yumechi-no-kuni/pulls/15

✅ Tested Working: WebUI (Admin+User use), "cli", Bull Dashboard, code highlighting, embed
🚫 Currently Not Working:

  • Some hardcoded images in about-misskey need to be proxied to be allowed.
  • /api-doc need to have the CDN dependency removed

Analysis (no true positive warnings!):

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨Feature This adds/improves/enhances a feature 🔒Security Security related issue/PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants