commit dd840a8559fe5456ec537082d5552a81b333aa9b parent 74055e373218743c661feac0eeb60a668204158a Author: JohnMcPMS <johnmcp@microsoft.com> Date: Mon, 6 Jul 2020 21:35:36 -0700 Further source integrity checks (#481) This change requires that the source is coming from an HTTPS location, and that the source package data has been verified for integrity locally before being used. Diffstat:
12 files changed, 55 insertions(+), 14 deletions(-)
diff --git a/src/AppInstallerCLIE2ETests/Constants.cs b/src/AppInstallerCLIE2ETests/Constants.cs @@ -58,6 +58,9 @@ namespace AppInstallerCLIE2ETests public const int ERROR_NO_SOURCES_DEFINED = unchecked((int)0x8A150015); public const int ERROR_MULTIPLE_APPLICATIONS_FOUND = unchecked((int)0x8A150016); public const int ERROR_NO_MANIFEST_FOUND = unchecked((int)0x8A150017); + public const int ERROR_EXTENSION_PUBLIC_FAILED = unchecked((int)0x8A150018); + public const int ERROR_COMMAND_REQUIRES_ADMIN = unchecked((int)0x8A150019); + public const int ERROR_SOURCE_NOT_SECURE = unchecked((int)0x8A15001A); } } } diff --git a/src/AppInstallerCLIE2ETests/SourceCommand.cs b/src/AppInstallerCLIE2ETests/SourceCommand.cs @@ -37,6 +37,11 @@ namespace AppInstallerCLIE2ETests Assert.AreEqual(Constants.ErrorCode.ERROR_NO_RANGES_PROCESSED, result.ExitCode); Assert.True(result.StdOut.Contains("error occurred while executing the command")); + // Add source with an HTTP url should fail + result = TestCommon.RunAICLICommand("source add", "Insecure http://microsoft.com"); + Assert.AreEqual(Constants.ErrorCode.ERROR_SOURCE_NOT_SECURE, result.ExitCode); + Assert.True(result.StdOut.Contains("error occurred while executing the command")); + // List with no args should list all available sources result = TestCommon.RunAICLICommand("source list", ""); Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); diff --git a/src/AppInstallerCLITests/Sources.cpp b/src/AppInstallerCLITests/Sources.cpp @@ -130,7 +130,7 @@ struct TestSourceFactory : public ISourceFactory m_Create(TestSource::Create), m_Add([](SourceDetails&) {}), m_Update([](const SourceDetails&) {}), m_Remove([](const SourceDetails&) {}) {} // ISourceFactory - std::shared_ptr<ISource> Create(const SourceDetails& details) override + std::shared_ptr<ISource> Create(const SourceDetails& details, IProgressCallback&) override { return m_Create(details); } diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp @@ -144,12 +144,12 @@ namespace AppInstaller::Utility return DownloadToStream(url, outfile, progress, computeHash); } + using namespace std::string_view_literals; + constexpr std::string_view s_http_start = "http://"sv; + constexpr std::string_view s_https_start = "https://"sv; + bool IsUrlRemote(std::string_view url) { - using namespace std::string_view_literals; - constexpr std::string_view s_http_start = "http://"sv; - constexpr std::string_view s_https_start = "https://"sv; - // Very simple choice right now: "does it start with http:// or https://"? if (CaseInsensitiveEquals(url.substr(0, s_http_start.length()), s_http_start) || CaseInsensitiveEquals(url.substr(0, s_https_start.length()), s_https_start)) @@ -160,6 +160,17 @@ namespace AppInstaller::Utility return false; } + bool IsUrlSecure(std::string_view url) + { + // Very simple choice right now: "does it start with https://"? + if (CaseInsensitiveEquals(url.substr(0, s_https_start.length()), s_https_start)) + { + return true; + } + + return false; + } + void ApplyMotwIfApplicable(const std::filesystem::path& filePath) { AICLI_LOG(Core, Info, << "Started applying motw to " << filePath); diff --git a/src/AppInstallerCommonCore/Errors.cpp b/src/AppInstallerCommonCore/Errors.cpp @@ -60,6 +60,8 @@ namespace AppInstaller return "No manifest found matching the criteria"; case APPINSTALLER_CLI_ERROR_COMMAND_REQUIRES_ADMIN: return "Command requires administrator privileges to run"; + case APPINSTALLER_CLI_ERROR_SOURCE_NOT_SECURE: + return "The source location is not secure"; default: return "Uknown Error Code"; } diff --git a/src/AppInstallerCommonCore/ExtensionCatalog.cpp b/src/AppInstallerCommonCore/ExtensionCatalog.cpp @@ -30,6 +30,13 @@ namespace AppInstaller::Deployment return m_extension.Package().Id().Version(); } + bool Extension::VerifyContentIntegrity(IProgressCallback& progress) + { + auto operation = m_extension.Package().VerifyContentIntegrityAsync(); + auto removeCancel = progress.SetCancellationFunction([&]() { operation.Cancel(); }); + return operation.get(); + } + ExtensionCatalog::ExtensionCatalog(std::wstring_view extensionName) { m_catalog = AppExt::AppExtensionCatalog::Open(winrt::hstring(extensionName)); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h @@ -35,6 +35,9 @@ namespace AppInstaller::Utility // Determines if the given url is a remote location. bool IsUrlRemote(std::string_view url); + // Determines if the given url is secured. + bool IsUrlSecure(std::string_view url); + // Apply Mark of the web if the target file is on NTFS, otherwise does nothing. void ApplyMotwIfApplicable(const std::filesystem::path& filePath); } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h @@ -35,6 +35,7 @@ #define APPINSTALLER_CLI_ERROR_NO_MANIFEST_FOUND ((HRESULT)0x8A150017) #define APPINSTALLER_CLI_ERROR_EXTENSION_PUBLIC_FAILED ((HRESULT)0x8A150018) #define APPINSTALLER_CLI_ERROR_COMMAND_REQUIRES_ADMIN ((HRESULT)0x8A150019) +#define APPINSTALLER_CLI_ERROR_SOURCE_NOT_SECURE ((HRESULT)0x8A15001A) namespace AppInstaller { diff --git a/src/AppInstallerCommonCore/Public/winget/ExtensionCatalog.h b/src/AppInstallerCommonCore/Public/winget/ExtensionCatalog.h @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once +#include <AppInstallerProgress.h> + #include <winrt/Windows.ApplicationModel.h> #include <winrt/Windows.ApplicationModel.AppExtensions.h> @@ -29,6 +31,9 @@ namespace AppInstaller::Deployment // Get the version of the package. winrt::Windows::ApplicationModel::PackageVersion GetPackageVersion() const; + // Verifies the integrity of the extension. + bool VerifyContentIntegrity(IProgressCallback& progress); + private: winrt::Windows::ApplicationModel::AppExtensions::AppExtension m_extension; }; diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -50,15 +50,15 @@ namespace AppInstaller::Repository::Microsoft // The base class for a package that comes from a preindexed packaged source. struct PreIndexedFactoryBase : public ISourceFactory { - std::shared_ptr<ISource> Create(const SourceDetails& details) override final + std::shared_ptr<ISource> Create(const SourceDetails& details, IProgressCallback& progress) override final { THROW_HR_IF(E_INVALIDARG, details.Type != PreIndexedPackageSourceFactory::Type()); auto lock = Synchronization::CrossProcessReaderWriteLock::LockForRead(CreateNameForCPRWL(details)); - return CreateInternal(details, std::move(lock)); + return CreateInternal(details, std::move(lock), progress); } - virtual std::shared_ptr<ISource> CreateInternal(const SourceDetails& details, Synchronization::CrossProcessReaderWriteLock&& lock) = 0; + virtual std::shared_ptr<ISource> CreateInternal(const SourceDetails& details, Synchronization::CrossProcessReaderWriteLock&& lock, IProgressCallback& progress) = 0; void Add(SourceDetails& details, IProgressCallback& progress) override final { @@ -75,6 +75,8 @@ namespace AppInstaller::Repository::Microsoft std::string packageLocation = GetPackageLocation(details); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NOT_SECURE, Utility::IsUrlRemote(packageLocation) && !Utility::IsUrlSecure(packageLocation)); + AICLI_LOG(Repo, Info, << "Initializing source from: " << details.Name << " => " << packageLocation); Msix::MsixInfo packageInfo(packageLocation); @@ -124,7 +126,7 @@ namespace AppInstaller::Repository::Microsoft return catalog.FindByPackageFamilyAndId(GetPackageFamilyNameFromDetails(details), Deployment::IndexDBId); } - std::shared_ptr<ISource> CreateInternal(const SourceDetails& details, Synchronization::CrossProcessReaderWriteLock&& lock) override + std::shared_ptr<ISource> CreateInternal(const SourceDetails& details, Synchronization::CrossProcessReaderWriteLock&& lock, IProgressCallback& progress) override { auto extension = GetExtensionFromDetails(details); if (!extension) @@ -133,6 +135,8 @@ namespace AppInstaller::Repository::Microsoft THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_MISSING); } + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NEEDS_REMEDIATION), !extension->VerifyContentIntegrity(progress)); + // To work around an issue with accessing the public folder, we are temporarily // constructing the location ourself. This was already the case for the non-packaged // runtime, and we can fix both in the future. The only problem with this is that @@ -234,7 +238,7 @@ namespace AppInstaller::Repository::Microsoft return result; } - std::shared_ptr<ISource> CreateInternal(const SourceDetails& details, Synchronization::CrossProcessReaderWriteLock&& lock) override + std::shared_ptr<ISource> CreateInternal(const SourceDetails& details, Synchronization::CrossProcessReaderWriteLock&& lock, IProgressCallback&) override { std::filesystem::path packageLocation = GetStatePathFromDetails(details); packageLocation /= s_PreIndexedPackageSourceFactory_IndexFileName; diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -364,9 +364,9 @@ namespace AppInstaller::Repository THROW_HR(APPINSTALLER_CLI_ERROR_INVALID_SOURCE_TYPE); } - std::shared_ptr<ISource> CreateSourceFromDetails(const SourceDetails& details) + std::shared_ptr<ISource> CreateSourceFromDetails(const SourceDetails& details, IProgressCallback& progress) { - return GetFactoryForType(details.Type)->Create(details); + return GetFactoryForType(details.Type)->Create(details, progress); } template <typename MemberFunc> @@ -536,7 +536,7 @@ namespace AppInstaller::Repository UpdateSourceFromDetails(*itr, progress); SetMetadata(currentSources); } - return CreateSourceFromDetails(*itr); + return CreateSourceFromDetails(*itr, progress); } } } diff --git a/src/AppInstallerRepositoryCore/SourceFactory.h b/src/AppInstallerRepositoryCore/SourceFactory.h @@ -15,7 +15,7 @@ namespace AppInstaller::Repository virtual ~ISourceFactory() = default; // Creates a source object from the given details. - virtual std::shared_ptr<ISource> Create(const SourceDetails& details) = 0; + virtual std::shared_ptr<ISource> Create(const SourceDetails& details, IProgressCallback& progress) = 0; // Adds the source from the given details, writing back to the details any changes. virtual void Add(SourceDetails& details, IProgressCallback& progress) = 0;