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

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

Closed
wants to merge 1 commit into from

Conversation

huchenlei
Copy link
Collaborator

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: #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
Copy link
Contributor

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
Copy link

wfjsw commented Jun 9, 2024

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
Copy link
Collaborator Author

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

@huchenlei huchenlei closed this Jun 9, 2024
@huchenlei huchenlei deleted the security branch June 9, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants