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

リダイレクトを含むユーザの照会がエラーになる #15048

Open
1 task
anatawa12 opened this issue Nov 23, 2024 · 15 comments · May be fixed by #15149
Open
1 task

リダイレクトを含むユーザの照会がエラーになる #15048

anatawa12 opened this issue Nov 23, 2024 · 15 comments · May be fixed by #15149
Labels
🐛Bug Unexpected behavior 🔥high priority

Comments

@anatawa12
Copy link
Member

💡 Summary

https://mi.7mi.site/@[email protected] のようなURLで照会すると (つまりリダイレクトを含む照会をすると) 2024.10.1のサーバーでは問題なく照会できるのに 2024.11.0 のサーバーだと 500 internal server error を返します。

複数のサーバーで確認しました。

🥰 Expected Behavior

リダイレクトをちゃんと追跡して照会できる

🤬 Actual Behavior

500 internal server errorが返ってくる

📝 Steps to Reproduce

  1. 適当な 2024.11.0 以降の misskey または他の (ほかサーバーの投稿を activity pub としてリクエストした際に本家のサーバーにリダイレクトする) activity pub 実装 のサーバーで、そのサーバー以外のユーザのページを開いてその URL をコピーする
  2. 2024.11.0 のサーバーで1で取得した URL で照会を行う

💻 Frontend Environment

* Model and OS of the device(s):
* Browser:
* Server URL:
* Misskey:

🛰 Backend Environment (for server admin)

* Installation Method or Hosting Service: docker + k8s
* Misskey: 2024.11.0-kinel.1
* Node: ??
* PostgreSQL: ?? 
* Redis: ?? 
* OS and Architecture: Linux ??

Do you want to address this bug yourself?

  • Yes, I will patch the bug myself and send a pull request
@anatawa12 anatawa12 added the 🐛Bug Unexpected behavior label Nov 23, 2024
@anatawa12 anatawa12 added this to the v2024.11.1? milestone Nov 23, 2024
@eternal-flame-AD
Copy link
Contributor

eternal-flame-AD commented Nov 26, 2024

The fetched host is already guaranteed to be from the authoritative source (assertActivityMatchesUrls) here:

取得されたホストは、すでに信頼できるソース (assertActivityMatchesUrls) からのものであることが保証されています:

const object = (this.user
? await this.apRequestService.signedGet(value, this.user) as IObject
: await this.httpRequestService.getActivityJson(value)) as IObject;

I do not think we have to check again without considering redirects (as the returned object will have the correct attribution and this original host has no control over it) the most mi.7mi.site can do is redirect you to a different site, but the returned user will have to match that different domain to pass the above check which will be clearly identifiable by the user:

リダイレクトを考慮せずに再度確認する必要はないと思います (返されるオブジェクトには正しい属性があり、元のホスト(mi.7mi.site)は制御できない) できることはせいぜい別のホストにリダイレクトすることですが、返されるホストはmisskey.ioではないのははっきりと表示されます。

if (this.utilityService.punyHost(object.id) !== this.utilityService.punyHost(value)) {
throw new Error(`invalid AP object ${value}: id ${object.id} has different host`);
}

However it is impossible to be sure without an appropriate advisory clearly identifying the reasoning of multiple seemingly-redundant checks like these. I hope the advisory can be published so instance owners know the how to appropriately deal with these patches (which are already public), as opposed to being tempted to revert them potentially exposing them to known threats.

ただし、このような複数のチェックの理由を明確に示す適切なアドバイスがなければ、確信を得ることはできません。インスタンス所有者がこれらのパッチ (すでに公開されています) を適切に処理する方法を知ることができるように、アドバイスが公開されることを願っています。

@eternal-flame-AD
Copy link
Contributor

リダイレクトを許可することによるリスクはユーザーが確認していないことですが(?)、それはバックエンドのエラーではなく、フロントエンドの警告のほうが良いと思います。

@samunohito
Copy link
Member

具体的な対応案としては以下のいずれかでしょうか…

①リダイレクトを検知したらユーザにエラーと回避策を通知する(いまは×しか出ないので)
②リダイレクトを検知したらユーザに確認ダイアログを提示して、承認が得られたら結果を画面に出す
③チェックやめる(上記でコメント頂いている通り、signedGetあるし…)
④リダイレクトされて帰ってきた情報と、その情報をもとに再照会した情報と突合して相違なければ結果を画面に出す

いずれにせよ、方針を決めて早く実装する必要があると思います

@samunohito
Copy link
Member

(個人的には③か④を推したい)

@kakkokari-gtyih
Copy link
Contributor

kakkokari-gtyih commented Dec 18, 2024

悪意あるサーバーに問い合わせてリダイレクト無くその場で偽の情報を返された場合は為す術が無いのでやるなら①だと思う(多分)

@anatawa12
Copy link
Member Author

リダイレクトの結果URLでjson内uri/urlと照合するのではだめなんです?

@kakkokari-gtyih
Copy link
Contributor

照会する際に使用するURLだけでは、それが第三者のサーバーのコンテンツ(リダイレクトを含む照会)なのか、問い合わせているサーバーのローカルコンテンツなのかの見分けがつかない

@anatawa12
Copy link
Member Author

照会する際に使用するURLだけではわかりませんが、リダイレクトされたときにはHTTP 30x + LocationヘッダでリダイレクトされたかどうかはResponse.urlとリクエストするときのurlを比較すればわかりますし、リダイレクト先のサーバーが別サーバーの偽情報を返されたかどうかもResponse.urlとbodyのurl/uriを比較すればわかると思っています

@kakkokari-gtyih
Copy link
Contributor

kakkokari-gtyih commented Dec 18, 2024

GHSA-m2gq-69fp-6hv4 を見たほうがいい(現時点で詳細情報は関係者のみに公開)

@anatawa12
Copy link
Member Author

anatawa12 commented Dec 18, 2024

見直したけど、照会のときにそうしないといけないのは私は納得がいってない。Activity Pubの各種解決時はまぁわかるんだが。

@anatawa12
Copy link
Member Author

ここのチェックをap/showの初回のresolveでスキップする仕組み(オプション)があればいい気がしている

if (this.utilityService.punyHost(object.id) !== this.utilityService.punyHost(value)) {
throw new Error(`invalid AP object ${value}: id ${object.id} has different host`);
}

const object = await resolver.resolve(uri) as any;

@anatawa12
Copy link
Member Author

anatawa12 commented Dec 18, 2024

そもそもだけど、 #14371 など、照会の最初のリダイレクト解決のための仕組みが、 activity pub の通常の解決の仕組みに入ってるのがなんかおかしい気もしてきた。ある程度似たような目的を持っているというのもわかるのでだめだとは言い切れないけど。


関係ないけど signToActivityPubGet が無効だと #14371 が動かないバグがあるきがします

@kakkokari-gtyih
Copy link
Contributor

照会のリダイレクト解決と通常のAPリクエスト時のリダイレクト解決って別のものかしら
どちらもActivityPubオブジェクトを取りに行く(それが手動かどうかだけの違い)ので GHSA-m2gq-69fp-6hv4 に該当するという認識だけど違うかしら

@kakkokari-gtyih
Copy link
Contributor

何も出ないと何が起こったかまったくわからないので暫定対応として #15147 を入れるのは良いとおもわれる

@anatawa12
Copy link
Member Author

GHSA-m2gq-69fp-6hv4 は関連オブジェクトの取得時のvalidation不足によって発生してるって認識してる。

#15147 に関しては他のエラーについてもあるしいれるべきだと思うけど、このissueを私の言ったように治すのをほぼ同時にするなら _responseInvalidIdHostNotMatch のエラーメッセージは不正なオブジェクトを照会してる旨のほうが正しそう

@kakkokari-gtyih kakkokari-gtyih removed this from the v2024.12.0 milestone Dec 18, 2024
@anatawa12 anatawa12 linked a pull request Dec 18, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛Bug Unexpected behavior 🔥high priority
Projects
Development

Successfully merging a pull request may close this issue.

4 participants