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

Disable loading of workspace configs by default and make it configurable #9545

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

the-dipsy
Copy link

Makes loading of workspace-local config.toml and languages.toml files optional and disables it by default to protect against vulnerability to arbitrary code execution when running helix in an untrusted directory. Should fix #9514 and alleviate #2697.

@archseer
Copy link
Member

archseer commented Feb 6, 2024

This is a quick fix but not the one I'm intending.

  • The editor should hold an allow list file that contains a list of allowed directories followed by a hash of the file.
  • We cross reference this list when trying to load a local config. If a config is detected but not on the list a status message should appear
  • :allow would append the file + it's hash to the allowlist

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we will want this behavior once we have workspace trust. Instead I think we will want the opposite: the ability to say that you never want to be prompted about workspace trust and never use workspace config. (So the default would be the opposite: that you do merge workspace configs but only when you trust the workspace.)

helix-term/src/config.rs Outdated Show resolved Hide resolved
helix-term/src/config.rs Outdated Show resolved Hide resolved
});
let global = merge_language_config(default_lang_config(), crate::lang_config_file())?;

let config = match global.get("workspace-config").and_then(|v| v.as_bool()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not have separate configurations for workspace-config for general config and language config, it's just confusing. Let's only add it to Config and use that value to determine whether we should load workspace configs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you implement that with the current code structure? Loading the entire config a second time seems wasteful but the current control flow doesn't seem to make it trivial to pass a parameter loaded from the config.toml to the code that loads the languages.toml.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only option is passing down a boolean parameter. IIRC we always load normal config before language config so it should be possible to read this only from config.toml

@@ -1,6 +1,8 @@
# Language support configuration.
# See the languages documentation: https://docs.helix-editor.com/master/languages.html

workspace-config = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, workspace-config should not be in languages.toml.

@the-dipsy
Copy link
Author

@the-mikedavis I'd be fine with your workspace trust concept once it is implemented and if the prompt is absolutely clear about the risks. I'd really like the ability to activate language servers and loading of workspace configs separately though.

@pascalkuthe
Copy link
Member

I'd really like the ability to activate language servers and loading of workspace configs separately though.

that ssesm like security theater to me. VSCode doesn't do that either. If you trust the LSP you trust the config. The other way around you can disable LSP in your workspace config.

@the-dipsy
Copy link
Author

@archseer There is one workspace-config = true for config.toml and one for languages.toml. The current code structure makes it way easier to implement this way and I think it makes sense too 🤷

@the-dipsy
Copy link
Author

@pascalkuthe It seems entirely reasonable to me to have language servers statically analyze code, provide snippets etc. without opening up the gates for ACE 😳 I'm not a heavy user of language servers but this is what GPT-4 says about it:

User: How common is unprotected execution of code from currently edited code repositories in language servers used in IDEs?
GPT-4: Unprotected execution of code from currently edited repositories in language servers is not common and is generally considered a security risk. Language servers typically provide features like code analysis, linting, autocomplete, and refactoring. They parse the code and understand its structure, but they do not execute the actual code being edited. When execution is necessary, for example, during debugging sessions, it is done in a controlled environment, often with explicit user consent and understanding of the risks.

I'd assume that a lot of people who switch to a small-code-base editor that doesn't rely on plugins, advertises to be usable over SSH, and is implemented in a security-driven language expect it to follow a security-conscious approach and configurability.

@the-dipsy
Copy link
Author

the-dipsy commented Feb 6, 2024

Can other users of VSCode confirm that enabling language servers is coupled to loading project local configurations and executing code from them? I've never really used the thing but that'd seem like a no-go to me.

@pascalkuthe
Copy link
Member

gpt is not a reliable source for anything and as usual provides a wordsoup without substance.

The majority of languageserver will execute arbitrary code which is why vscode disables them by default. For example rust-analyzer will execute all "build.rs" files and procedural macros (yes without any sandboxing). They often even have their own config files which they read from the project from which they can and do execute commands.

@the-dipsy
Copy link
Author

the-dipsy commented Feb 6, 2024

@pascalkuthe Certainly no proof and unreliable but often decent at reiterating and summarizing information. I know that rust does this but it therefore is also pretty much the first thing that comes up when putting "language server" and "arbitrary code execution" into a search engine. I don't have a reliable source that says most LSPs don't do this. Do you have a source for the opposite? Jedi apparently is one explicit counter-example. In any case, the fact that there are plenty of ways to use the language server protocol without arbitrary code execution warrants an option to use them that way IMO.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not keen to accept this as-is: we usually do not accept "bandaid" fixes that work around missing features, especially when we would have to remove them or make breaking changes to them later as we would with workspace trust. IMO this is not a very serious threat and seems mostly theoretical so it would be an overreaction to hot-fix it that leaves us with annoying defaults, a lot like git's safe.directory response to cve-2022-24765 (reiterating what I was saying here).

But I think this PR could be a really good base for the changes we need for workspace trust and could be merged before that is introduced with a small change: let's switch the workspace_config: bool option out for an enum

#[derive(Default)]
pub enum LoadWorkspaceConfig {
	Always,
    #[default]
    Never,
}

which you would write in config as "always" or "never". Workspace trust would add a Trust variant that should become the default. That way you can decide what behavior you want: always or never load workspace configs, and the meaning of these options doesn't change at all once workspace trust is added.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2024
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 6, 2024
@archseer
Copy link
Member

archseer commented Feb 6, 2024

I pushed my set of changes based on some comments above here: 5ed223f

@the-dipsy
Copy link
Author

@archseer Oh, shoot. I refactored a bunch already. Is my version okay for you?

@kirawi kirawi removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 8, 2024
@kirawi kirawi added S-waiting-on-review Status: Awaiting review from a maintainer. R-breaking-change This PR is a breaking change for some behavior labels Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements R-breaking-change This PR is a breaking change for some behavior S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop silently executing arbitrary code from the current directory by default
5 participants