w-okada / voice-changer

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

[ISSUE]: Privacy/Security Concern: Any website/domain is able to make fetch requests to the backend! #1114

Open Venryx opened 9 months ago

Venryx commented 9 months ago

Voice Changer Version

MMVCServerSIO_win_onnxgpu-cuda_v.1.5.3.17b.zip

Operational System

Windows 10

GPU

NVIDIA GeForce RTX 2060 SUPER

Read carefully and check the options

Model Type

RVC

Issue Description

The backend (ie. the python system serving requests to 127.0.0.1:18888/info, 127.0.0.1:18888/update_settings, etc.) currently does no CORS validation (ie. validation of the Referer header) for any requests made to it. (nor identity checks of any other sort)

The relevant code in the server where these requests are being received: https://github.com/w-okada/voice-changer/blob/927bba6467532470a2951462d470a398f3befff4/server/restapi/MMVC_Rest_Fileuploader.py#L20-L34

What this means: Any website on the internet can just make whatever API calls it wants to the running W-Okada backend, including:

As linked above, I created a working codepen demonstrating how random sites can access the backend: https://codepen.io/Venryx/pen/NWJeKaz

If the W-Okada app is running when you open the link above, it will read and display the contents of the first model's params.json file. (I can extend the demo to show various other endpoints being called if needed, but from the ones I've tested so far, all of them work.)

Application Screenshot

Here is an example output from the quick codepen.io demo I put together, where it reads the contents of the first model in model_dir (one of the demo voices that the app starts with):

2024-02-13_15-36-01_node

Logs on console

Not applicable. The problem is not that an error is occuring, but that an error is not occuring, ie. the backend is accepting requests from any website, without any identity verification. (for example, verifying that the Referer header equals 127.0.0.1:18888)

deiteris commented 9 months ago

In general, the current implementation must not be exposed to the internet (only localhost or VPN/local network) since it does not implement any access checks. And yes, model loading is dangerous since the model may be loaded with PyTorch which uses pickle.

Misconfigured CORS is the least of the problems since even if it's fixed (the lines related to CORS can be simply removed since they are just permissive), you can just make the requests directly.

Venryx commented 9 months ago

Ah yes, good point about PyTorch/pickle: (from: https://huggingface.co/docs/hub/security-pickle)

Pickle is a widely used serialization format in ML. Most notably, it is the default format for PyTorch model weights.

There are dangerous arbitrary code execution attacks that can be perpetrated when you load a pickle file.

[...]

REDUCE is what tells the unpickler to execute the function with the provided arguments and *GLOBAL instructions are telling the unpickler to import stuff.

To sum up, pickle is dangerous because:

  • when importing a python module, arbitrary code can be executed
  • you can import builtin functions like eval or exec, which can be used to execute arbitrary code
  • when instantiating an object, the constructor may be called

This is why it is stated in most docs using pickle, do not unpickle data from untrusted sources.

I don't have the expertise to know for sure if the uploaded models are subsequently imported with pickle-functionality enabled, but it seems so, eg.: https://github.com/w-okada/voice-changer/blob/927bba6467532470a2951462d470a398f3befff4/server/voice_changer/RVC/RVCModelSlotGenerator.py#L41-L42

The code above appears to call torch.load on the active model's file, with the definition for that torch.load function being here: https://pytorch.org/docs/stable/generated/torch.load.html

torch.load() unless weights_only parameter is set to True, uses pickle module implicitly, which is known to be insecure. It is possible to construct malicious pickle data which will execute arbitrary code during unpickling. Never load data that could have come from an untrusted source in an unsafe mode, or that could have been tampered with. Only load data you trust.

And the warning above seems to apply here, since weights_only defaults to False, and the call above does not modify that default.

So in summary: If the W-Okada Voice Changer is running, it appears that any website is able to execute arbitrary code on the visitor's host machine.

Steps:

Again, no expert in the model loading pipeline, so perhaps there are some protections in place to try to prevent the chain above (though hard to see what that would be). So until word is received from the core devs on this issue, it's pretty concerning.

At the very least, there is the aforementioned privacy vulnerability in that any website can get a list of voices that someone has installed, and call the various other endpoints; but it's very serious if the chain above does indeed allow websites to run arbitrary code on the wider machine.

@w-okada Please clarify on these concerns (whether pickle's vulnerability is indeed activated here, and what the plans are for addressing the ability of random websites to call the backend), as it makes me hesitant to leave the W-Okada Voice Changer running between usage sessions now.

Venryx commented 9 months ago

Misconfigured CORS is the least of the problems since even if it's fixed (the lines related to CORS can be simply removed since they are just permissive), you can just make the requests directly.

What do you mean by "make the requests directly"?

From what I understand, browsers enforce that requests made from websites' JS always include the Referer header, with it set to the host/domain that the webpage is hosted on. So, properly configured CORS would help limit that attack vector at least (which is the most serious since it's completely silent, and requires no downloads/installations).

That said, one could argue that the protection should go further, eg. some way to prevent rogue Chrome Extensions from calling the backend (since they aren't subject to the same CORS restrictions). Although, stopping Chrome Extensions from accessing the backend may also cut off some legitimate use-cases. (eg. a 3rd-party desktop app from calling the W-Okada backend, as I do with a custom electron app of mine, since I've created a more streamlined interface for voice-conversion there)

EDIT: Also, the concern about models maybe being loaded with PyTorch (as opposed to Onnx), is made more severe by the fact that the website-accessible API lets you change the settings, meaning a malicious website can just force-change the conversion system from Onnx to PyTorch prior to loading the exploit-carrying model.

deiteris commented 9 months ago

What do you mean by "make the requests directly"?

You can simply visit the pages served by the backend without crafting a web page (i.e., by just navigating to http://127.0.0.1/info directly), and this is bad if the server is publicly available. A malicious HTTP client may craft and send HTTP requests to the backend that uploads and executes a malicious model without interacting with the front end.

Again, no expert in the model loading pipeline, so perhaps there are some protections in place to try to prevent the chain above (though hard to see what that would be). So until word is received from the core devs on this issue, it's pretty concerning.

It's possible to enforce the usage of safetensors (I have it implemented in my fork for example), but other providers and popular sources for the voice models should be also supporting and promoting this format, which may be difficult.

From what I understand, browsers enforce that requests made from websites' JS always include the Referer header, with it set to the host/domain that the webpage is hosted on.

It's a bit different. Web servers do not rely on the Referer header since it's up to client what to set in this header. CORS relies on the Access-Control-* headers provided by the server (which domains are allowed, which methods are allowed, etc.). Typically, this is only to restrict cross-origin requests and mitigate XSS attacks. For example, when a user navigates to a malicious page that replicates the origin, but performs additional malicious logic when it makes a request to the actual origin website.

That said, one could argue that the protection should go further, eg. some way to prevent rogue Chrome Extensions from calling the backend (since they aren't subject to the same CORS restrictions).

Well, no, Chrome extensions are actually subject to CORS as well. They should not be able to make requests to the backend directly from their environment (via fetch()). I think even if they do script injection, they still shouldn't be able to do that.

a 3rd-party desktop app from calling the W-Okada backend, as I do with a custom electron app of mine, since I've created a more streamlined interface for voice-conversion there

CORS shouldn't be a problem for this. As long as your app opens and operates within the same domain - it should work without any issues.

Venryx commented 9 months ago

It's a bit different. Web servers do not rely on the Referer header since it's up to client what to set in this header. CORS relies on the Access-Control-* headers provided by the server (which domains are allowed, which methods are allowed, etc.). Typically, this is only to restrict cross-origin requests and mitigate XSS attacks. For example, when a user navigates to a malicious page that replicates the origin, but performs additional malicious logic when it makes a request to the actual origin website.

Okay, but even if the standard purpose of the Referer header is for a different use-case, implementation of that check for all server endpoints would still help though, no? (my using the term "CORS" was maybe incorrect; I just meant trying to use the Referer header as an extra protection-check)

That is, if the w-okada server/backend required that all API requests have the Referer header set to 127.0.0.1:18888, that would prevent random websites from being able to make calls to the server, since the browser does not allow websites' fetch requests (nor those in img tags, script tags, etc. or in direct-requests from newly launched tabs) to just set a custom value for the Referer header.

While it's true that other clients could spoof that Referer flag (eg. 3rd-party desktop applications), at least the most convenient attack vector (simply having a visitor load your website) would be blocked from accessing the w-okada server/backend.

EDIT: Maybe using the origin header for this purpose would be preferred over referer? Not sure if/when they can differ.

Well, no, Chrome extensions are actually subject to CORS as well. They should not be able to make requests to the backend directly from their environment (via fetch()). I think even if they do script injection, they still shouldn't be able to do that.

It looks like this is true for regular extensions, but there's apparently a way for privileged Chrome extensions to modify the Referer header in requests it makes (see the 2nd answer for manifest v3): https://stackoverflow.com/a/31003808

However, since the extension requires a special permission (that of modifying web requests), I suppose it's not that big of a concern. (since it limits the categories of extensions which could try to hijack the backend; most would seem suspicious if they requested that permission)

deiteris commented 9 months ago

EDIT: Maybe using the origin header for this purpose would be preferred over referer? Not sure if/when they can differ.

That's exactly what one of the checks CORS does :) The server reports the allowed origins and the browser enforces the origin checks. By default, all browsers allow requests only within the same origin if CORS header is not present.

The problem is that the current access control header for origin is too permissive, it just allows any origin: https://github.com/w-okada/voice-changer/blob/master/server/restapi/MMVC_Rest.py#L53

But this will fix only one part of the problem. The other part is still in the fact that the backend is just accessible by anyone and at least simple authN/authZ is required on the backend side. If your proposal is to check by the headers, then it won't work because you should not trust the headers reported by the client.

Venryx commented 9 months ago

If your proposal is to check by the headers, then it won't work because you should not trust the headers reported by the client.

The reason I suggested an origin/referer header check is that it's easy to implement; it may only require a few lines being changed (eg. the line you linked where you set the allowed origins). And, a simple origin check will at least prevent the easiest pathway for exploiting the vulnerability (of just getting your target to load a webpage you control).

It doesn't protect from rogue 3rd-party desktop apps, but are there other attack vectors you have in mind beyond this? (ie. ways to call the API with the origin/referer faked to be 127.0.0.1:18888, without having to trick the user into installing something that runs outside their browser?)

More importantly: Is there a downside to adding the basic origin check? While it doesn't cover every pathway, it certainly seems to be an improvement. And I'd rather get an "80% fix" now, than wait potentially a long time for any fix at all. (Which is a concern imo, since it's been 5 days and there has been no fix or even comment from the w-okada dev[s?] so far. I'm assuming this is because they're busy, which is understandable -- but that is why a simple fix is preferred for now, since it's more likely to get implemented quickly.)

deiteris commented 9 months ago

It doesn't protect from rogue 3rd-party desktop apps, but are there other attack vectors you have in mind beyond this?

I cannot think of any other possible attack vectors. If we are talking only about things happening inside the browser environment, then, IMO, only rogue extensions may be an issue.

More importantly: Is there a downside to adding the basic origin check?

No, not really, at least from what I've tried. If you want, you can download a build from my fork that should be addressing this part of the issue and check it yourself: https://github.com/deiteris/voice-changer/actions/runs/7944366293.

I simply removed permissive headers, so the browser falls back to same-origin policy and restricts this kind of manipulations: https://github.com/deiteris/voice-changer/commit/c4e11e6eb208f2af2487cb38cb59f95094620cde image

If I attempt to use fetch() to send requests to the backend from some other page, there will be an error. For example, here I made request while being on google.com: image

Venryx commented 9 months ago

Interesting, I may try that fork. (downloading it for now at least)

That said, since the CORS protection is being applied by the browser itself, I believe you mentioned earlier how the attack can still occur, eg. by changing the fetch request's mode to no-cors (opaque), as mentioned in the screenshotted error-message -- since the malicious requests don't actually need to read any data back, they simply need to send certain commands in the correct sequence.

So the question is whether removing the CORSMiddleware from the app's fast-api (as done in your fork) is sufficient for the server to also outright reject any requests which do not have the correct origin header provided. Based on what you said earlier, I'm guessing this is not the case.

EDIT: Indeed, I've tested (in a small fastapi demo) and the CORSMiddleware (with limited allow_origins) does nothing in terms of preventing a request from being processed by the server. In fact, the fetch request doesn't even need to set no-cors; while the browser prevents the fetch response from being readable by the JavaScript code, the actual request goes through perfectly fine, performing its effects on the server.

So, I suppose the CORS route is indeed not very helpful, since it doesn't even provide protection for the browser-based attack vector (well... it might still be worth doing since it prevents simple reading of the user's voice list and what-not, but very insufficient).

In that case, something like the middleware below might work: (well, for the browser-based attack vector)

allowedReferers = [
    "http://127.0.0.1:18888/", "https://127.0.0.1:18888/",
    "http://localhost:18888/", "https://localhost:18888/"
]

@app.middleware("http")
async def check_referer_header(request: Request, call_next):
    referer = request.headers.get("Referer", None)
    # require that the `referer` header be recognized (unless doing the initial loading of the root page/document)
    if referer not in allowedReferers and request.url.path != "/":
        return Response(content="Invalid Referer header", status_code=403)

    response = await call_next(request)
    return response

I've tested this, and it seems to work in rejecting requests without the correct Referer header. (I chose Referer rather than Origin, since the Origin header is apparently not sent for all requests, whereas the Referer header seems to be sent for all requests from the frontend page)

Note that I haven't tested the middleware above in the w-okada app itself (since its build process is more involved than the small fastapi demo repo I was tinkering with), but presumably it should work there as well. (although it will need some small adjustments, eg. dynamically reading the port that it is supposed to allow, rather than using the hard-coded 18888)

w-okada commented 9 months ago

In the next major update, we're also considering making the application of CORS middleware optional.

Venryx commented 9 months ago

In the next major update, we're also considering making the application of CORS middleware optional.

Sounds good! Though as noted above, even without the cross-origin acceptance (ie. CORS with accept "*"), the exploit is still possible by just making specific fetch requests with no-cors set.

While the browser will stop the webpage from reading the results, the problem still exists because the exploit can be done just by doing a set of "opaque requests" in sequence that upload and load a malicious model file/blob.

So there needs to be something server-side that completely rejects wrong-[referrer/origin] requests (like my example code above), rather than simply preventing reading the results (which is the only protection that browsers can enforce, surprisingly).

Venryx commented 9 months ago

This remains a serious security risk; I'm having trouble understanding why there is not more alarm at the the ability for random websites to run arbitrary code on the host computer, merely if the user has the w-okada app running in the background.

Here is an example "from the wild" of a model uploaded to hugging-face having an exploit that opens a shell on the victim's computer, letting them control it remotely: https://arstechnica.com/security/2024/03/hugging-face-the-github-of-ai-hosted-code-that-backdoored-user-devices/

In loading PyTorch models with transformers, a common approach involves utilizing the torch.load() function, which deserializes the model from a file. [...]

In the context of the repository “baller423/goober2,” it appears that the malicious payload was injected into the PyTorch model file using the __reduce__ method of the pickle module. This method, as demonstrated in the provided reference, enables attackers to insert arbitrary Python code into the deserialization process, potentially leading to malicious behavior when the model is loaded. [...]

The model’s payload grants the attacker a shell on the compromised machine, enabling them to gain full control over victims’ machines through what is commonly referred to as a ‘backdoor,’” JFrog Senior Researcher David Cohen wrote. “This silent infiltration could potentially grant access to critical internal systems and pave the way for large-scale data breaches or even corporate espionage, impacting not just individual users but potentially entire organizations across the globe, all while leaving victims utterly unaware of their compromised state.”

And the w-okada situation is worse than the above, because the user merely has to visit a malicious site -- they don't need to start a download or even click anything.

@w-okada While the referrer check in my post earlier is pretty unsophisticated, it seems like it would at least patch the gaping hole of mere website visits allowing for arbitrary code execution on the host machine. Could you discuss what changes would be needed for validation along those lines to be accepted in a pull request?

deiteris commented 9 months ago

So the question is whether removing the CORSMiddleware from the app's fast-api (as done in your fork) is sufficient for the server to also outright reject any requests which do not have the correct origin header provided. Based on what you said earlier, I'm guessing this is not the case.

Oh! Yeah, indeed it looks like even though the browser prevents from reading such a request, it still reaches the backend and being processed. That's not good in this specific case... Then yeah, an explicit check for origin/referer is required on the backend to prevent it from being executed at least for now.

This should be relatively simple to do (just like in your example) and additional allowed referers can be configurable with CLI argument. I could try adding this.

I also might try to come up with access control implementation. I have an idea in mind but that would require more changes.

Venryx commented 8 months ago

@w-okada This is still a very serious security hole. (potential for arbitrary Python code execution on the host machine, from any website the user visits, with no condition other than that the w-okada app be running at the time)

I understand if you don't have time to implement a fix yourself, but please at least let us know what type of solution you'd be open to having implemented (for example, a "referrer check" along the lines of my example code above), so that we can then work on a pull-request with confidence that the development effort will not be wasted.

deiteris commented 8 months ago

I've made a PR with security hardening that you can check: https://github.com/w-okada/voice-changer/pull/1153

It adds the Origin header check and the server should respond with status code 400 if the origin was not whitelisted.

Venryx commented 8 months ago

I've made a PR with security hardening that you can check: #1153

It adds the Origin header check and the server should respond with status code 400 if the origin was not whitelisted.

Thank you for your work. I plan to try out your branch and see if I can find any holes in it; will report back when that's done.

EDIT: Okay, I've done some testing of it; I've added my comments here to the pull-request thread.