search

w-okada / voice-changer

リアルタイムボイスチェンジャー Realtime Voice Changer
Other
15.28k stars 1.64k forks source link

Harden web server security #1153

Closed deiteris closed 3 months ago

deiteris commented 3 months ago

Addresses https://github.com/w-okada/voice-changer/issues/1114.

github-actions[bot] commented 3 months ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

deiteris commented 3 months ago

I have read the CLA Document and I hereby sign the CLA

Venryx commented 3 months ago

I've been doing some testing on your harden-security branch, and it seems to work well!

Some of the things I tried from another origin (website, and another localhost port), which were successfully blocked: (tested with server in both https and non-https mode, and in both Chrome and Firefox)

The only thing I was able to do, is perform "GET" requests to the server (eg. /info and /onnx, with my observing a log-message of "No pyTorch filepath" on the server each time the /onnx endpoint was fetched), when:

So, the above is the standard/expected behavior. That said, the one minor "negative" of the "GET" requests being allowed, is that they do technically allow websites to observe whether the W-Okada Voice Changer app is running (if it was launched in non-https mode at least):

// if voice-changer is running, this returns an empty string; else, it ends up timing-out/erroring instead
await (await fetch("http://localhost:18888/info", {
    method: "GET", mode: "no-cors",
})).text()

That's not that big a deal though; only real drawback I can see is some embarrassment if someone using a voice-changer has that revealed by opening a link that detects the app being active. While not 100% ideal, I don't see any easy way to solve this last part. (Since browsers, eg. Chrome, just aren't designed to be able to outright block opaque, cross-origin GET requests; while theoretically blocking these GET requests could happen in the server, eg. by blocking if the referrer header is from another domain, it might be difficult to exactly mimic the type of response that is consistent with the app not running at all, and I don't want to hold up a major security improvement over a minor detail like this.)

In summary: This pull-request does appear to offer a major improvement in security from how things used to be. Thank you for your work fixing this, and making the list of allowed-origins configurable (as this retains flexibility for some custom setups). I hope this can be merged soon!

deiteris commented 3 months ago

The only thing I was able to do, is perform "GET" requests to the server (eg. /info and /onnx, with my observing a log-message of "No pyTorch filepath" on the server each time the /onnx endpoint was fetched), when:

  • The server is launched in non-https mode. (https-mode blocks cross-origin GET requests with the [seemingly non-js-visible] error net::ERR_CERT_AUTHORITY_INVALID)
  • The request body is empty. (browser blocks providing a body in GET requests)
  • The response's contents are unable to be read. (browser makes the response "opaque" by replacing it with an empty string)

Were you able to actually read those requests? With the latest commit on that branch, I'm getting error 400 from unknown origins no matter which parameters I pass. This is what is expected: image image

Same with HTTPS: image image

So even reading should not be possible in case you're trying to access from unknown origin.

One more important thing is that the PR also introduces origin check for SIO, so connecting to websocket from unknown origin should be also forbidden.

image

Venryx commented 3 months ago

Were you able to actually read those requests? With the latest commit on that branch, I'm getting error 400 from unknown origins no matter which parameters I pass.

So even reading should not be possible in case you're trying to access from unknown origin.

Correct, I was not able to actually read those requests. (I simply used the "empty string" opaque response to observe that the voice-changer app was active, since if it weren't, the request would just error/timeout instead; which is expected, not so serious, and hard to 100% solve anyway)

Venryx commented 3 months ago

@w-okada 重要: このプル リクエストに関するフィードバックを提供していただけますか?マージを検討する前に解決してほしい問題はありますか?

現状では、このプログラムをバックグラウンドで実行したままにしておくのは信用できません。マシン上で任意の Python コードを実行しているランダムな Web サイトにアクセスしたままになってしまうからです。 (具体的には、w-okada プログラム/バックエンドへの呼び出しを通じて、pickle エクスプロイトによってアクティブ化された悪意のあるコードを含むカスタムの「音声モデル」をアップロードしてアクティブ化することによって、その API は現在どの Web サイト/ドメインからでもアクセスでき、完全に完了します)沈黙）

[the above is the Japanese translation, in case w-okada is time-constrained and only sees the email notification, which may not offer automatic translation to make a read-through convenient; english version below]

Can you please provide feedback on this pull request? Is there something wrong with it that you'd like addressed before considering merging it?

As it stands, I have trouble being trusting of leaving this program running in the background, as it leaves me open to random websites running arbitrary Python code on my machine. (specifically: by uploading and activating a custom "voice model" containing malicious code, activated by means of the pickle exploit, through calls to the w-okada program/backend, whose API can currently be accessed from any website/domain and in complete silence)

w-okada commented 3 months ago

Thanks for your PR. Your proposal is likely to be very important. However, we cannot immediately determine how significant the changes will impact. We will proceed with the merge, but please note there is a possibility that we will roll it back depending on the effects.