Author: Sam McCall Date: 2021-04-15T14:51:23+02:00 New Revision: ecf93a716c9ecf2e38898547df90323e239a623c
URL: https://github.com/llvm/llvm-project/commit/ecf93a716c9ecf2e38898547df90323e239a623c DIFF: https://github.com/llvm/llvm-project/commit/ecf93a716c9ecf2e38898547df90323e239a623c.diff LOG: [clangd] Only allow remote index to be enabled from user config. Differential Revision: https://reviews.llvm.org/D100542 Added: Modified: clang-tools-extra/clangd/ConfigCompile.cpp clang-tools-extra/clangd/ConfigFragment.h clang-tools-extra/clangd/ConfigProvider.cpp clang-tools-extra/clangd/ConfigProvider.h clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp index a5745727dca78..31beb6ac10cb2 100644 --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -101,6 +101,7 @@ struct FragmentCompiler { llvm::SourceMgr *SourceMgr; // Normalized Fragment::SourceInfo::Directory. std::string FragmentDirectory; + bool Trusted = false; llvm::Optional<llvm::Regex> compileRegex(const Located<std::string> &Text, @@ -183,6 +184,7 @@ struct FragmentCompiler { } void compile(Fragment &&F) { + Trusted = F.Source.Trusted; if (!F.Source.Directory.empty()) { FragmentDirectory = llvm::sys::path::convert_to_slash(F.Source.Directory); if (FragmentDirectory.back() != '/') @@ -320,6 +322,13 @@ struct FragmentCompiler { void compile(Fragment::IndexBlock::ExternalBlock &&External, llvm::SMRange BlockRange) { + if (External.Server && !Trusted) { + diag(Error, + "Remote index may not be specified by untrusted configuration. " + "Copy this into user config to use it.", + External.Server->Range); + return; + } #ifndef CLANGD_ENABLE_REMOTE if (External.Server) { elog("Clangd isn't compiled with remote index support, ignoring Server: " @@ -510,8 +519,8 @@ CompiledFragment Fragment::compile(DiagnosticCallback D) && { trace::Span Tracer("ConfigCompile"); SPAN_ATTACH(Tracer, "ConfigFile", ConfigFile); auto Result = std::make_shared<CompiledFragmentImpl>(); - vlog("Config fragment: compiling {0}:{1} -> {2}", ConfigFile, LineCol.first, - Result.get()); + vlog("Config fragment: compiling {0}:{1} -> {2} (trusted={3})", ConfigFile, + LineCol.first, Result.get(), Source.Trusted); FragmentCompiler{*Result, D, Source.Manager.get()}.compile(std::move(*this)); // Return as cheaply-copyable wrapper. diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h index 6b58b88dfd490..8e4110e6dba13 100644 --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -94,6 +94,9 @@ struct Fragment { /// Absolute path to directory the fragment is associated with. Relative /// paths mentioned in the fragment are resolved against this. std::string Directory; + /// Whether this fragment is allowed to make critical security/privacy + /// decisions. + bool Trusted = false; }; SourceInfo Source; diff --git a/clang-tools-extra/clangd/ConfigProvider.cpp b/clang-tools-extra/clangd/ConfigProvider.cpp index 3b705d8f4a82d..2b8e406b60d5e 100644 --- a/clang-tools-extra/clangd/ConfigProvider.cpp +++ b/clang-tools-extra/clangd/ConfigProvider.cpp @@ -36,7 +36,7 @@ class FileConfigCache : public FileCache { : FileCache(Path), Directory(Directory) {} void get(const ThreadsafeFS &TFS, DiagnosticCallback DC, - std::chrono::steady_clock::time_point FreshTime, + std::chrono::steady_clock::time_point FreshTime, bool Trusted, std::vector<CompiledFragment> &Out) const { read( TFS, FreshTime, @@ -45,6 +45,7 @@ class FileConfigCache : public FileCache { if (Data) for (auto &Fragment : Fragment::parseYAML(*Data, path(), DC)) { Fragment.Source.Directory = Directory; + Fragment.Source.Trusted = Trusted; CachedValue.push_back(std::move(Fragment).compile(DC)); } }, @@ -54,35 +55,38 @@ class FileConfigCache : public FileCache { std::unique_ptr<Provider> Provider::fromYAMLFile(llvm::StringRef AbsPath, llvm::StringRef Directory, - const ThreadsafeFS &FS) { + const ThreadsafeFS &FS, + bool Trusted) { class AbsFileProvider : public Provider { mutable FileConfigCache Cache; // threadsafe const ThreadsafeFS &FS; + bool Trusted; std::vector<CompiledFragment> getFragments(const Params &P, DiagnosticCallback DC) const override { std::vector<CompiledFragment> Result; - Cache.get(FS, DC, P.FreshTime, Result); + Cache.get(FS, DC, P.FreshTime, Trusted, Result); return Result; }; public: AbsFileProvider(llvm::StringRef Path, llvm::StringRef Directory, - const ThreadsafeFS &FS) - : Cache(Path, Directory), FS(FS) { + const ThreadsafeFS &FS, bool Trusted) + : Cache(Path, Directory), FS(FS), Trusted(Trusted) { assert(llvm::sys::path::is_absolute(Path)); } }; - return std::make_unique<AbsFileProvider>(AbsPath, Directory, FS); + return std::make_unique<AbsFileProvider>(AbsPath, Directory, FS, Trusted); } std::unique_ptr<Provider> Provider::fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath, - const ThreadsafeFS &FS) { + const ThreadsafeFS &FS, bool Trusted) { class RelFileProvider : public Provider { std::string RelPath; const ThreadsafeFS &FS; + bool Trusted; mutable std::mutex Mu; // Keys are the (posix-style) ancestor directory, not the config within it. @@ -124,18 +128,19 @@ Provider::fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath, // This will take a (per-file) lock for each file that actually exists. std::vector<CompiledFragment> Result; for (FileConfigCache *Cache : llvm::reverse(Caches)) - Cache->get(FS, DC, P.FreshTime, Result); + Cache->get(FS, DC, P.FreshTime, Trusted, Result); return Result; }; public: - RelFileProvider(llvm::StringRef RelPath, const ThreadsafeFS &FS) - : RelPath(RelPath), FS(FS) { + RelFileProvider(llvm::StringRef RelPath, const ThreadsafeFS &FS, + bool Trusted) + : RelPath(RelPath), FS(FS), Trusted(Trusted) { assert(llvm::sys::path::is_relative(RelPath)); } }; - return std::make_unique<RelFileProvider>(RelPath, FS); + return std::make_unique<RelFileProvider>(RelPath, FS, Trusted); } std::unique_ptr<Provider> diff --git a/clang-tools-extra/clangd/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h index 25d9450f28a72..428438b67f14d 100644 --- a/clang-tools-extra/clangd/ConfigProvider.h +++ b/clang-tools-extra/clangd/ConfigProvider.h @@ -69,7 +69,8 @@ class Provider { /// Directory will be used to resolve relative paths in the fragments. static std::unique_ptr<Provider> fromYAMLFile(llvm::StringRef AbsPath, llvm::StringRef Directory, - const ThreadsafeFS &); + const ThreadsafeFS &, + bool Trusted = false); // Reads fragments from YAML files found relative to ancestors of Params.Path. // // All fragments that exist are returned, starting from distant ancestors. @@ -78,7 +79,8 @@ class Provider { // // If Params does not specify a path, no fragments are returned. static std::unique_ptr<Provider> - fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath, const ThreadsafeFS &); + fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath, const ThreadsafeFS &, + bool Trusted = false); /// A provider that includes fragments from all the supplied providers. /// Order is preserved; later providers take precedence over earlier ones. diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index 77e8ff1539e9e..8fe14231bfee1 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -855,8 +855,8 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var if (llvm::sys::path::user_config_directory(UserConfig)) { llvm::sys::path::append(UserConfig, "clangd", "config.yaml"); vlog("User config file is {0}", UserConfig); - ProviderStack.push_back( - config::Provider::fromYAMLFile(UserConfig, /*Directory=*/"", TFS)); + ProviderStack.push_back(config::Provider::fromYAMLFile( + UserConfig, /*Directory=*/"", TFS, /*Trusted=*/true)); } else { elog("Couldn't determine user config file, not loading"); } diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp index 23347e2324feb..f999e0ec2c357 100644 --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -318,7 +318,21 @@ TEST_F(ConfigCompileTests, TidyBadChecks) { DiagKind(llvm::SourceMgr::DK_Warning)))); } +TEST_F(ConfigCompileTests, ExternalServerNeedsTrusted) { + Fragment::IndexBlock::ExternalBlock External; + External.Server.emplace("xxx"); + Frag.Index.External = std::move(External); + compileAndApply(); + EXPECT_THAT( + Diags.Diagnostics, + ElementsAre(DiagMessage( + "Remote index may not be specified by untrusted configuration. " + "Copy this into user config to use it."))); + EXPECT_FALSE(Conf.Index.External.hasValue()); +} + TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) { + Frag.Source.Trusted = true; Fragment::IndexBlock::ExternalBlock External; External.File.emplace(""); External.Server.emplace(""); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
