Mikubill / sd-webui-controlnet

WebUI extension for ControlNet
GNU General Public License v3.0
16.41k stars 1.9k forks source link

Add security warning for install from environment variable override URL #2953

Closed huchenlei closed 3 weeks ago

huchenlei commented 3 weeks ago

The os.environ override of dep path is meant to be used by package author in China to serve users who has difficulties accessing github. Original PR: https://github.com/Mikubill/sd-webui-controlnet/pull/2514

However, this poses a security risk to install any package from internet if someone set these environment variables somewhere else. This PR adds an explicit warning message to warn user when URLs from these environs are used.

@Akegarasu Do you think there is a better way to handle this on your side? Current mechanism is somewhat unsafe.

Akegarasu commented 3 weeks ago

Actually, I don't think this is a security issue worth worrying about, perhaps due to the exposure of some node of comfyui today that some people are overly concerned.

Firstly, within sd-webui, there are many commands that directly read and execute environment variables, such as TORCH_COMMAND, or XFORMERS_PACKAGE, among others. This is a method that everyone uses.

Secondly, if a malicious program is already capable of modifying the environment variables, it is highly likely that it does not need to use this method to install a malicious package.

wfjsw commented 3 weeks ago

With access to environment variable, I can get all my way into your code and remove the warning before it is even executed, so it is not much safer than the current state.

huchenlei commented 3 weeks ago

Thanks for the explanation! I guess we can keep it as it is.