From f88107e9cb873b13a783bb19b848aa287a8ea042 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Tue, 26 Apr 2022 14:50:05 -0700 Subject: [PATCH 01/13] Test --- .../PreIndexedPackageSourceFactory.cpp | 59 ++++++++++++++++++- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index 83eaa1ae5c..62172eb35a 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -16,7 +16,6 @@ namespace AppInstaller::Repository::Microsoft namespace { static constexpr std::string_view s_PreIndexedPackageSourceFactory_PackageFileName = "source.msix"sv; - static constexpr std::string_view s_PreIndexedPackageSourceFactory_AppxManifestFileName = "AppxManifest.xml"sv; static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFileName = "index.db"sv; // TODO: This being hard coded to force using the Public directory name is not ideal. static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv; @@ -338,6 +337,51 @@ namespace AppInstaller::Repository::Microsoft return result; } + struct TempIndexFile + { + TempIndexFile() = default; + + TempIndexFile(Msix::MsixInfo& packageInfo, IProgressCallback& progress) + { + GUID guid; + THROW_IF_FAILED(CoCreateGuid(&guid)); + WCHAR tempFileName[256]; + THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0); + + m_indexFile = Runtime::GetPathTo(Runtime::PathName::Temp); + m_indexFile /= tempFileName; + + m_indexHanlde.reset(CreateFileW(m_indexFile.c_str(), 0, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); + THROW_LAST_ERROR_IF(!m_indexHanlde); + + packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexFile, progress); + } + + ~TempIndexFile() + { + if (!m_indexFile.empty() && std::filesystem::exists(m_indexFile)) + { + if (m_indexHanlde) + { + m_indexHanlde.reset(); + } + + try + { + std::filesystem::remove(m_indexFile); + } + catch (...) + { + AICLI_LOG(Repo, Info, << "Failed to remove temp index file at: " << m_indexFile); + } + } + } + + private: + std::filesystem::path m_indexFile; + wil::unique_handle m_indexHanlde; + }; + struct DesktopContextSourceReference : public ISourceReference { DesktopContextSourceReference(const SourceDetails& details) : m_details(details) @@ -361,7 +405,7 @@ namespace AppInstaller::Repository::Microsoft } std::filesystem::path packageLocation = GetStatePathFromDetails(m_details); - packageLocation /= s_PreIndexedPackageSourceFactory_IndexFileName; + packageLocation /= s_PreIndexedPackageSourceFactory_PackageFileName; if (!std::filesystem::exists(packageLocation)) { @@ -369,6 +413,15 @@ namespace AppInstaller::Repository::Microsoft THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_MISSING); } + Msix::MsixInfo packageInfo(Utility::ConvertToUTF8(packageLocation.c_str())); + TempIndexFile tempIndexFile; + + if (progress.IsCancelled()) + { + AICLI_LOG(Repo, Info, << "Cancelling open upon request"); + return {}; + } + SQLiteIndex index = SQLiteIndex::Open(packageLocation.u8string(), SQLiteIndex::OpenDisposition::Read); // We didn't use to store the source identifier, so we compute it here in case it's @@ -389,7 +442,7 @@ namespace AppInstaller::Repository::Microsoft return std::make_shared(details); } - bool UpdateInternal(const std::string&, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override + bool UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override { // We will extract the manifest and index files directly to this location std::filesystem::path packageState = GetStatePathFromDetails(details); From 0febe83abe0f433c34a22c52d2298290f23f3540 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Thu, 28 Apr 2022 13:50:08 -0700 Subject: [PATCH 02/13] Signature validation --- .../Workflows/MSStoreInstallerHandler.cpp | 1 + src/AppInstallerCommonCore/MsixInfo.cpp | 148 ++++++++++++++++++ .../Public/AppInstallerMsixInfo.h | 3 + src/AppInstallerCommonCore/pch.h | 2 + 4 files changed, 154 insertions(+) diff --git a/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp index 99d11224eb..33f067a209 100644 --- a/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp @@ -88,6 +88,7 @@ namespace AppInstaller::CLI::Workflow if (result.Status() == GetEntitlementStatus::Succeeded) { context.Reporter.Info() << Resource::String::MSStoreInstallGetEntitlementSuccess << std::endl; + AICLI_LOG(CLI, Info, << "Get entitlement succeeded."); } else if (result.Status() == GetEntitlementStatus::NetworkError) { diff --git a/src/AppInstallerCommonCore/MsixInfo.cpp b/src/AppInstallerCommonCore/MsixInfo.cpp index 0f99ae75dc..4761abf7bf 100644 --- a/src/AppInstallerCommonCore/MsixInfo.cpp +++ b/src/AppInstallerCommonCore/MsixInfo.cpp @@ -16,6 +16,10 @@ namespace AppInstaller::Msix { namespace { + // MSIX-specific header placed in the P7X file, before the actual signature + const DWORD P7xFileId = 0x58434b50; + const DWORD P7xFileIdSize = sizeof(P7xFileId); + // Gets the version from the manifest reader. UINT64 GetVersionFromManifestReader(IAppxManifestReader* reader) { @@ -278,6 +282,150 @@ namespace AppInstaller::Msix return { result }; } + bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin) + { + bool result = true; + AICLI_LOG(Core, Info, << "Started trust validation of msix at: " << msixPath); + + try + { + // First verify certificate chain if requested. + if (verifyMicrosoftOrigin) + { + // Retrieve raw signature from msix + MsixInfo msixInfo{ msixPath.u8string() }; + auto signature = msixInfo.GetSignature(); + THROW_HR_IF(E_UNEXPECTED, signature.size() <= P7xFileIdSize); + signature.erase(signature.begin(), signature.begin() + P7xFileIdSize); + + // Get the cert content + wil::unique_any signedMessage; + wil::unique_hcertstore certStore; + CRYPT_DATA_BLOB signatureBlob = { 0 }; + signatureBlob.cbData = signature.size(); + signatureBlob.pbData = signature.data(); + THROW_LAST_ERROR_IF(!CryptQueryObject( + CERT_QUERY_OBJECT_BLOB, + &signatureBlob, + CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED, + CERT_QUERY_FORMAT_FLAG_BINARY, + 0, // Reserved parameter + NULL, // No encoding info needed + NULL, + NULL, + &certStore, + &signedMessage, + NULL)); + + // Get the signer size and information from the signed data message + // The properties of the signer info will be used to uniquely identify the signing certificate in the certificate store + DWORD signerInfoSize = 0; + THROW_LAST_ERROR_IF(!CryptMsgGetParam( + signedMessage.get(), + CMSG_SIGNER_INFO_PARAM, + 0, + NULL, + &signerInfoSize)); + + // Check that the signer info size is within reasonable bounds; under the max length of a string for the issuer field + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_DATA), !(signerInfoSize > 0 && signerInfoSize < STRSAFE_MAX_CCH)); + + std::vector signerInfoBuffer; + signerInfoBuffer.resize(signerInfoSize); + THROW_LAST_ERROR_IF(!CryptMsgGetParam( + signedMessage.get(), + CMSG_SIGNER_INFO_PARAM, + 0, + signerInfoBuffer.data(), + &signerInfoSize)); + + // Get the signing certificate from the certificate store based on the issuer and serial number of the signer info + CMSG_SIGNER_INFO* signerInfo = reinterpret_cast(signerInfoBuffer.data()); + CERT_INFO certInfo; + certInfo.Issuer = signerInfo->Issuer; + certInfo.SerialNumber = signerInfo->SerialNumber; + + wil::unique_cert_context certContext; + certContext.reset(CertGetSubjectCertificateFromStore( + certStore.get(), + X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, + &certInfo)); + THROW_LAST_ERROR_IF(!certContext.get()); + + // Get certificate chain context for validation + CERT_CHAIN_PARA certChainParameters = { 0 }; + certChainParameters.cbSize = sizeof(CERT_CHAIN_PARA); + certChainParameters.RequestedUsage.dwType = USAGE_MATCH_TYPE_AND; + DWORD certChainFlags = CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL; + + wil::unique_cert_chain_context certChainContext; + THROW_LAST_ERROR_IF(!CertGetCertificateChain( + HCCE_LOCAL_MACHINE, + certContext.get(), + NULL, // Use the current system time for CRL validation + certStore.get(), + &certChainParameters, + certChainFlags, + NULL, // Reserved parameter; must be NULL + &certChainContext)); + + // Validate that the certificate chain is rooted in one of the well-known Microsoft root certs + CERT_CHAIN_POLICY_PARA policyParameters = { 0 }; + policyParameters.cbSize = sizeof(CERT_CHAIN_POLICY_PARA); + policyParameters.dwFlags = MICROSOFT_ROOT_CERT_CHAIN_POLICY_CHECK_APPLICATION_ROOT_FLAG; + CERT_CHAIN_POLICY_STATUS policyStatus = { 0 }; + policyStatus.cbSize = sizeof(CERT_CHAIN_POLICY_STATUS); + LPCSTR policyOid = CERT_CHAIN_POLICY_MICROSOFT_ROOT; + BOOL certChainVerifySucceeded = CertVerifyCertificateChainPolicy( + policyOid, + certChainContext.get(), + &policyParameters, + &policyStatus); + + AICLI_LOG(Core, Info, << "Result for certificate chain validation of Microsoft origin: " << policyStatus.dwError); + + result = certChainVerifySucceeded && policyStatus.dwError == ERROR_SUCCESS; + } + + // If certificate chain origin validation is success or not requested, then validate the trust info of the file. + if (result) + { + // Set up the structures needed for the WinVerifyTrust call + WINTRUST_FILE_INFO fileInfo = { 0 }; + fileInfo.cbStruct = sizeof(WINTRUST_FILE_INFO); + fileInfo.pcwszFilePath = msixPath.c_str(); + + WINTRUST_DATA trustData = { 0 }; + trustData.cbStruct = sizeof(WINTRUST_DATA); + trustData.dwUIChoice = WTD_UI_NONE; + trustData.fdwRevocationChecks = WTD_REVOKE_NONE; + trustData.dwUnionChoice = WTD_CHOICE_FILE; + trustData.dwStateAction = WTD_STATEACTION_VERIFY; + trustData.dwProvFlags = WTD_CACHE_ONLY_URL_RETRIEVAL | WTD_REVOCATION_CHECK_NONE; + trustData.pFile = &fileInfo; + + GUID verifyActionId = WINTRUST_ACTION_GENERIC_VERIFY_V2; + + HRESULT verifyTrustResult = (HRESULT)WinVerifyTrust(static_cast(INVALID_HANDLE_VALUE), &verifyActionId, &trustData); + AICLI_LOG(Core, Info, << "Result for trust info validation of the msix: " << verifyTrustResult); + + result = verifyTrustResult == S_OK; + } + } + catch (const wil::ResultException& re) + { + AICLI_LOG(Core, Error, << "Failed during msix trust validation. Error: " << re.GetErrorCode()); + result = false; + } + catch (...) + { + AICLI_LOG(Core, Error, << "Failed during msix trust validation."); + result = false; + } + + return result; + } + MsixInfo::MsixInfo(std::string_view uriStr) { if (Utility::IsUrlRemote(uriStr)) diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h index dd95b7e21e..7ccdd96347 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -42,6 +42,9 @@ namespace AppInstaller::Msix // Gets the package location from the given full name. std::optional GetPackageLocationFromFullName(std::string_view fullName); + // Validate the msix file trust info, also validate the signing cert is from Microsoft origin if verifyMicrosoftOrigin is true; + bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin = false); + // MsixInfo class handles all appx/msix related query. struct MsixInfo { diff --git a/src/AppInstallerCommonCore/pch.h b/src/AppInstallerCommonCore/pch.h index 0ef4c263c1..a4986d3f1e 100644 --- a/src/AppInstallerCommonCore/pch.h +++ b/src/AppInstallerCommonCore/pch.h @@ -13,6 +13,8 @@ #include #include #include +#include +#include #include "TraceLogging.h" From 058a44c33ff00a6c841997f1b453504333b06dac Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Thu, 28 Apr 2022 16:38:41 -0700 Subject: [PATCH 03/13] build --- src/AppInstallerCommonCore/MsixInfo.cpp | 10 +- .../Public/AppInstallerMsixInfo.h | 4 +- .../AppInstallerRepositoryCore.vcxproj | 2 + ...AppInstallerRepositoryCore.vcxproj.filters | 6 ++ .../PreIndexedPackageSourceFactory.cpp | 94 ++++++++----------- .../Microsoft/SQLiteIndex.cpp | 12 +-- .../Microsoft/SQLiteIndex.h | 6 +- .../Microsoft/TempSQLiteIndexFile.cpp | 48 ++++++++++ .../Microsoft/TempSQLiteIndexFile.h | 29 ++++++ 9 files changed, 139 insertions(+), 72 deletions(-) create mode 100644 src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp create mode 100644 src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h diff --git a/src/AppInstallerCommonCore/MsixInfo.cpp b/src/AppInstallerCommonCore/MsixInfo.cpp index 4761abf7bf..8c86fb5684 100644 --- a/src/AppInstallerCommonCore/MsixInfo.cpp +++ b/src/AppInstallerCommonCore/MsixInfo.cpp @@ -520,16 +520,16 @@ namespace AppInstaller::Msix return Utility::ConvertToUTF8(GetPackageFullNameWide()); } - bool MsixInfo::IsNewerThan(const std::filesystem::path& otherManifest) + bool MsixInfo::IsNewerThan(const std::filesystem::path& otherPackage) { THROW_HR_IF(E_NOT_VALID_STATE, m_isBundle); - ComPtr otherStream; - THROW_IF_FAILED(SHCreateStreamOnFileEx(otherManifest.c_str(), - STGM_READ | STGM_SHARE_DENY_WRITE | STGM_FAILIFTHERE, 0, FALSE, nullptr, &otherStream)); + MsixInfo other{ otherPackage.u8string() }; + + THROW_HR_IF(E_INVALIDARG, other.m_isBundle); ComPtr otherReader; - GetManifestReader(otherStream.Get(), &otherReader); + THROW_IF_FAILED(other.m_packageReader->GetManifest(&otherReader)); ComPtr manifestReader; THROW_IF_FAILED(m_packageReader->GetManifest(&manifestReader)); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h index 7ccdd96347..20a703f9d2 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -68,8 +68,8 @@ namespace AppInstaller::Msix std::wstring GetPackageFullNameWide(); std::string GetPackageFullName(); - // Gets a value indicating whether the referenced info is newer than the given manifest. - bool IsNewerThan(const std::filesystem::path& otherManifest); + // Gets a value indicating whether the referenced info is newer than the given package. + bool IsNewerThan(const std::filesystem::path& otherPackage); bool IsNewerThan(const winrt::Windows::ApplicationModel::PackageVersion& otherVersion); diff --git a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj index 0f0df2c4b5..7040bdb73a 100644 --- a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj +++ b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj @@ -269,6 +269,7 @@ + @@ -335,6 +336,7 @@ + diff --git a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters index 78d9b98747..5905fc068a 100644 --- a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters +++ b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters @@ -267,6 +267,9 @@ Public\winget + + Microsoft + @@ -419,6 +422,9 @@ Source Files + + Microsoft + diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index 62172eb35a..a825562fd9 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -4,6 +4,7 @@ #include "Microsoft/PreIndexedPackageSourceFactory.h" #include "Microsoft/SQLiteIndex.h" #include "Microsoft/SQLiteIndexSource.h" +#include "TempSQLiteIndexFile.h" #include #include @@ -337,51 +338,6 @@ namespace AppInstaller::Repository::Microsoft return result; } - struct TempIndexFile - { - TempIndexFile() = default; - - TempIndexFile(Msix::MsixInfo& packageInfo, IProgressCallback& progress) - { - GUID guid; - THROW_IF_FAILED(CoCreateGuid(&guid)); - WCHAR tempFileName[256]; - THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0); - - m_indexFile = Runtime::GetPathTo(Runtime::PathName::Temp); - m_indexFile /= tempFileName; - - m_indexHanlde.reset(CreateFileW(m_indexFile.c_str(), 0, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); - THROW_LAST_ERROR_IF(!m_indexHanlde); - - packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexFile, progress); - } - - ~TempIndexFile() - { - if (!m_indexFile.empty() && std::filesystem::exists(m_indexFile)) - { - if (m_indexHanlde) - { - m_indexHanlde.reset(); - } - - try - { - std::filesystem::remove(m_indexFile); - } - catch (...) - { - AICLI_LOG(Repo, Info, << "Failed to remove temp index file at: " << m_indexFile); - } - } - } - - private: - std::filesystem::path m_indexFile; - wil::unique_handle m_indexHanlde; - }; - struct DesktopContextSourceReference : public ISourceReference { DesktopContextSourceReference(const SourceDetails& details) : m_details(details) @@ -413,8 +369,10 @@ namespace AppInstaller::Repository::Microsoft THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_MISSING); } - Msix::MsixInfo packageInfo(Utility::ConvertToUTF8(packageLocation.c_str())); - TempIndexFile tempIndexFile; + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !Msix::ValidateMsixTrustInfo(packageLocation, WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin))); + + Msix::MsixInfo packageInfo(packageLocation.u8string()); + TempSQLiteIndexFile tempIndexFile{ packageInfo, progress }; if (progress.IsCancelled()) { @@ -422,7 +380,7 @@ namespace AppInstaller::Repository::Microsoft return {}; } - SQLiteIndex index = SQLiteIndex::Open(packageLocation.u8string(), SQLiteIndex::OpenDisposition::Read); + SQLiteIndex index = SQLiteIndex::Open(tempIndexFile.GetIndexFilePath().u8string(), SQLiteIndex::OpenDisposition::Immutable, std::move(tempIndexFile)); // We didn't use to store the source identifier, so we compute it here in case it's // missing from the details. @@ -448,29 +406,51 @@ namespace AppInstaller::Repository::Microsoft std::filesystem::path packageState = GetStatePathFromDetails(details); std::filesystem::create_directories(packageState); - std::filesystem::path manifestPath = packageState / s_PreIndexedPackageSourceFactory_AppxManifestFileName; - std::filesystem::path indexPath = packageState / s_PreIndexedPackageSourceFactory_IndexFileName; + std::filesystem::path packagePath = packageState / s_PreIndexedPackageSourceFactory_PackageFileName; - if (std::filesystem::exists(manifestPath) && std::filesystem::exists(indexPath)) + if (std::filesystem::exists(packagePath)) { - // If we already have a manifest, use it to determine if we need to update or not. - if (!packageInfo.IsNewerThan(manifestPath)) + // If we already have a trusted index package, use it to determine if we need to update or not. + if (Msix::ValidateMsixTrustInfo(packagePath, WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) && + !packageInfo.IsNewerThan(packagePath)) { AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed"); return true; } } + std::filesystem::path tempPackagePath = packagePath.u8string() + ".dlnd.msix"; + AppInstaller::Utility::Download(packageLocation, tempPackagePath, AppInstaller::Utility::DownloadType::Index, progress); + + bool updateSuccess = false; if (progress.IsCancelled()) { AICLI_LOG(Repo, Info, << "Cancelling update upon request"); - return false; + } + else if (Msix::ValidateMsixTrustInfo(tempPackagePath, WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin))) + { + std::filesystem::rename(tempPackagePath, packagePath); + AICLI_LOG(Repo, Info, << "Source update success."); + updateSuccess = true; + } + else + { + AICLI_LOG(Repo, Info, << "Source update failed. Source package failed trust validation."); } - packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, indexPath, progress); - packageInfo.WriteManifestToFile(manifestPath, progress); + if (!updateSuccess) + { + try + { + std::filesystem::remove(tempPackagePath); + } + catch (...) + { + AICLI_LOG(Repo, Info, << "Failed to remove temp index file at: " << tempPackagePath); + } + } - return true; + return updateSuccess; } bool RemoveInternal(const SourceDetails& details, IProgressCallback&) override diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp index 5dd2c0c7b2..a65b6c8ac3 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp @@ -45,15 +45,15 @@ namespace AppInstaller::Repository::Microsoft return result; } - SQLiteIndex SQLiteIndex::Open(const std::string& filePath, OpenDisposition disposition) + SQLiteIndex SQLiteIndex::Open(const std::string& filePath, OpenDisposition disposition, TempSQLiteIndexFile&& tempIndexFileHandle) { AICLI_LOG(Repo, Info, << "Opening SQLite Index for " << GetOpenDispositionString(disposition) << " at '" << filePath << "'"); switch (disposition) { case AppInstaller::Repository::Microsoft::SQLiteIndex::OpenDisposition::Read: - return { filePath, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::None }; + return { filePath, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::None, std::move(tempIndexFileHandle) }; case AppInstaller::Repository::Microsoft::SQLiteIndex::OpenDisposition::ReadWrite: - return { filePath, SQLite::Connection::OpenDisposition::ReadWrite, SQLite::Connection::OpenFlags::None }; + return { filePath, SQLite::Connection::OpenDisposition::ReadWrite, SQLite::Connection::OpenFlags::None, std::move(tempIndexFileHandle) }; case AppInstaller::Repository::Microsoft::SQLiteIndex::OpenDisposition::Immutable: { // Following the algorithm set forth at https://sqlite.org/uri.html [3.1] to convert to a URI path @@ -99,15 +99,15 @@ namespace AppInstaller::Repository::Microsoft target += "?immutable=1"; - return { target, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::Uri }; + return { target, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::Uri, std::move(tempIndexFileHandle) }; } default: THROW_HR(E_UNEXPECTED); } } - SQLiteIndex::SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags) : - m_dbconn(SQLite::Connection::Create(target, disposition, flags)) + SQLiteIndex::SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags, TempSQLiteIndexFile&& tempIndexFileHandle) : + m_dbconn(SQLite::Connection::Create(target, disposition, flags)), m_tempIndexFileHandle(std::move(tempIndexFileHandle)) { m_dbconn.EnableICU(); m_version = Schema::Version::GetSchemaVersion(m_dbconn); diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h index 092abc4153..4fa7ce7da1 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h +++ b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h @@ -5,6 +5,7 @@ #include "Microsoft/Schema/ISQLiteIndex.h" #include "Microsoft/Schema/Version.h" #include "ISource.h" +#include "TempSQLiteIndexFile.h" #include #include #include @@ -57,7 +58,7 @@ namespace AppInstaller::Repository::Microsoft }; // Opens an existing index database. - static SQLiteIndex Open(const std::string& filePath, OpenDisposition disposition); + static SQLiteIndex Open(const std::string& filePath, OpenDisposition disposition, TempSQLiteIndexFile&& tempIndexFileHandle = {}); // Gets the schema version of the index. Schema::Version GetVersion() const { return m_version; } @@ -151,7 +152,7 @@ namespace AppInstaller::Repository::Microsoft std::vector> GetDependentsById(AppInstaller::Manifest::string_t packageId) const; private: // Constructor used to open an existing index. - SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags); + SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags, TempSQLiteIndexFile&& tempIndexFileHandle); // Constructor used to create a new index. SQLiteIndex(const std::string& target, Schema::Version version); @@ -167,5 +168,6 @@ namespace AppInstaller::Repository::Microsoft Schema::Version m_version; std::unique_ptr m_interface; std::unique_ptr m_interfaceLock = std::make_unique(); + TempSQLiteIndexFile m_tempIndexFileHandle; }; } diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp new file mode 100644 index 0000000000..b6ea141f25 --- /dev/null +++ b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp @@ -0,0 +1,48 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "TempSQLiteIndexFile.h" + +namespace AppInstaller::Repository::Microsoft +{ + namespace + { + static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv; + } + + TempSQLiteIndexFile::TempSQLiteIndexFile(AppInstaller::Msix::MsixInfo& packageInfo, AppInstaller::IProgressCallback& progress) + { + GUID guid; + THROW_IF_FAILED(CoCreateGuid(&guid)); + WCHAR tempFileName[256]; + THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0); + + m_indexFile = Runtime::GetPathTo(Runtime::PathName::Temp); + m_indexFile /= tempFileName; + + m_indexHanlde.reset(CreateFileW(m_indexFile.c_str(), GENERIC_READ | GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); + THROW_LAST_ERROR_IF(!m_indexHanlde); + + packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexFile, progress); + } + + TempSQLiteIndexFile::~TempSQLiteIndexFile() + { + if (!m_indexFile.empty() && std::filesystem::exists(m_indexFile)) + { + if (m_indexHanlde) + { + m_indexHanlde.reset(); + } + + try + { + std::filesystem::remove(m_indexFile); + } + catch (...) + { + AICLI_LOG(Repo, Info, << "Failed to remove temp index file at: " << m_indexFile); + } + } + } +} \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h new file mode 100644 index 0000000000..ca94a4a544 --- /dev/null +++ b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h @@ -0,0 +1,29 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once +#include +#include +namespace AppInstaller::Repository::Microsoft +{ + // Holds a temp SQLite index file extracted from index package, exclusive to the process. + struct TempSQLiteIndexFile + { + TempSQLiteIndexFile() = default; + + TempSQLiteIndexFile(AppInstaller::Msix::MsixInfo& packageInfo, AppInstaller::IProgressCallback& progress); + + TempSQLiteIndexFile(const TempSQLiteIndexFile&) = delete; + TempSQLiteIndexFile& operator=(const TempSQLiteIndexFile&) = delete; + + TempSQLiteIndexFile(TempSQLiteIndexFile&&) = default; + TempSQLiteIndexFile& operator=(TempSQLiteIndexFile&&) = default; + + const std::filesystem::path& GetIndexFilePath() { return m_indexFile; } + + ~TempSQLiteIndexFile(); + + private: + std::filesystem::path m_indexFile; + wil::unique_handle m_indexHanlde; + }; +} From 80595d3ae3fb6844c9694d37bb0cd5e12d2a8196 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Thu, 28 Apr 2022 18:54:40 -0700 Subject: [PATCH 04/13] build fix --- src/AppInstallerCommonCore/MsixInfo.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCommonCore/MsixInfo.cpp b/src/AppInstallerCommonCore/MsixInfo.cpp index 8c86fb5684..44c40b3451 100644 --- a/src/AppInstallerCommonCore/MsixInfo.cpp +++ b/src/AppInstallerCommonCore/MsixInfo.cpp @@ -302,7 +302,7 @@ namespace AppInstaller::Msix wil::unique_any signedMessage; wil::unique_hcertstore certStore; CRYPT_DATA_BLOB signatureBlob = { 0 }; - signatureBlob.cbData = signature.size(); + signatureBlob.cbData = static_cast(signature.size()); signatureBlob.pbData = signature.data(); THROW_LAST_ERROR_IF(!CryptQueryObject( CERT_QUERY_OBJECT_BLOB, @@ -406,7 +406,7 @@ namespace AppInstaller::Msix GUID verifyActionId = WINTRUST_ACTION_GENERIC_VERIFY_V2; - HRESULT verifyTrustResult = (HRESULT)WinVerifyTrust(static_cast(INVALID_HANDLE_VALUE), &verifyActionId, &trustData); + HRESULT verifyTrustResult = static_cast(WinVerifyTrust(static_cast(INVALID_HANDLE_VALUE), &verifyActionId, &trustData)); AICLI_LOG(Core, Info, << "Result for trust info validation of the msix: " << verifyTrustResult); result = verifyTrustResult == S_OK; From 6e358dae1e63733b0c5a6fcab5a3f90e405cf3e4 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Sun, 1 May 2022 12:50:46 -0700 Subject: [PATCH 05/13] fix --- src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h | 2 +- .../Microsoft/TempSQLiteIndexFile.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h index 4fa7ce7da1..e74520b579 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h +++ b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h @@ -164,10 +164,10 @@ namespace AppInstaller::Repository::Microsoft // Sets the last write time metadata value in the index. void SetLastWriteTime(); + TempSQLiteIndexFile m_tempIndexFileHandle; SQLite::Connection m_dbconn; Schema::Version m_version; std::unique_ptr m_interface; std::unique_ptr m_interfaceLock = std::make_unique(); - TempSQLiteIndexFile m_tempIndexFileHandle; }; } diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp index b6ea141f25..38cb6fa818 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp @@ -20,10 +20,10 @@ namespace AppInstaller::Repository::Microsoft m_indexFile = Runtime::GetPathTo(Runtime::PathName::Temp); m_indexFile /= tempFileName; - m_indexHanlde.reset(CreateFileW(m_indexFile.c_str(), GENERIC_READ | GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); + m_indexHanlde.reset(CreateFileW(m_indexFile.c_str(), GENERIC_WRITE, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); THROW_LAST_ERROR_IF(!m_indexHanlde); - packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexFile, progress); + packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexFile, progress, true); } TempSQLiteIndexFile::~TempSQLiteIndexFile() From e27543d584b74af5fd778fa28c7ff8d601582fc5 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Sun, 1 May 2022 21:09:50 -0700 Subject: [PATCH 06/13] work --- src/AppInstallerCommonCore/MsixInfo.cpp | 79 +++++++++++++++++++ .../Public/AppInstallerMsixInfo.h | 3 + .../Microsoft/TempSQLiteIndexFile.cpp | 2 +- 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/AppInstallerCommonCore/MsixInfo.cpp b/src/AppInstallerCommonCore/MsixInfo.cpp index 44c40b3451..d89e4f8c83 100644 --- a/src/AppInstallerCommonCore/MsixInfo.cpp +++ b/src/AppInstallerCommonCore/MsixInfo.cpp @@ -121,6 +121,68 @@ namespace AppInstaller::Msix WriteStreamToFile(stream.Get(), size, target, progress); } + + // Writes the stream (from current location) to the given file handle. + void WriteStreamToFileHandle(IStream* stream, UINT64 expectedSize, HANDLE target, IProgressCallback& progress) + { + constexpr ULONG bufferSize = 1 << 20; + std::unique_ptr buffer = std::make_unique(bufferSize); + + UINT64 totalBytesRead = 0; + + while (!progress.IsCancelled()) + { + ULONG bytesRead = 0; + HRESULT hr = stream->Read(buffer.get(), bufferSize, &bytesRead); + + if (bytesRead) + { + // If we got bytes, just accept them and keep going. + LOG_IF_FAILED(hr); + + THROW_LAST_ERROR_IF(INVALID_SET_FILE_POINTER == SetFilePointer(target, 0, nullptr, FILE_END)); + DWORD bytesWritten = 0; + THROW_LAST_ERROR_IF(!WriteFile(target, buffer.get(), bytesRead, &bytesWritten, nullptr)); + THROW_HR_IF(E_UNEXPECTED, bytesRead != bytesWritten); + totalBytesRead += bytesRead; + progress.OnProgress(totalBytesRead, expectedSize, ProgressType::Bytes); + } + else + { + // If given a size, and we have read it all, quit + if (expectedSize && totalBytesRead == expectedSize) + { + break; + } + + // If the stream returned an error, throw it + THROW_IF_FAILED(hr); + + // If we were given a size and didn't reach it, throw our own error; + // otherwise assume that this is just normal EOF. + if (expectedSize) + { + THROW_WIN32(ERROR_HANDLE_EOF); + } + else + { + break; + } + } + } + } + + // Writes the appx file to the given file handle. + void WriteAppxFileToFileHandle(IAppxFile* appxFile, HANDLE target, IProgressCallback& progress) + { + UINT64 size = 0; + THROW_IF_FAILED(appxFile->GetSize(&size)); + + ComPtr stream; + THROW_IF_FAILED(appxFile->GetStream(&stream)); + + WriteStreamToFileHandle(stream.Get(), size, target, progress); + } } bool GetBundleReader( @@ -578,4 +640,21 @@ namespace AppInstaller::Msix WriteAppxFileToFile(appxFile.Get(), target, progress); } + + void MsixInfo::WriteToFileHandle(std::string_view packageFile, HANDLE target, IProgressCallback& progress) + { + std::wstring fileUTF16 = Utility::ConvertToUTF16(packageFile); + + ComPtr appxFile; + if (m_isBundle) + { + THROW_IF_FAILED(m_bundleReader->GetPayloadPackage(fileUTF16.c_str(), &appxFile)); + } + else + { + THROW_IF_FAILED(m_packageReader->GetPayloadFile(fileUTF16.c_str(), &appxFile)); + } + + WriteAppxFileToFileHandle(appxFile.Get(), target, progress); + } } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h index 20a703f9d2..6cc303c31f 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -79,6 +79,9 @@ namespace AppInstaller::Msix // Writes the package's manifest to the given path. void WriteManifestToFile(const std::filesystem::path& target, IProgressCallback& progress); + // Writes the package file to the given file handle. + void WriteToFileHandle(std::string_view packageFile, HANDLE target, IProgressCallback& progress); + private: bool m_isBundle; Microsoft::WRL::ComPtr m_stream; diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp index 38cb6fa818..2668bc266a 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp @@ -23,7 +23,7 @@ namespace AppInstaller::Repository::Microsoft m_indexHanlde.reset(CreateFileW(m_indexFile.c_str(), GENERIC_WRITE, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); THROW_LAST_ERROR_IF(!m_indexHanlde); - packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexFile, progress, true); + packageInfo.WriteToFileHandle(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexHanlde.get(), progress); } TempSQLiteIndexFile::~TempSQLiteIndexFile() From c0ccd59b739d0ec1aadc370fdae888563e1c13f2 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Wed, 4 May 2022 19:16:39 -0700 Subject: [PATCH 07/13] tests and fixes --- .../AppInstallerCLIPackage.wapproj | 4 +- .../AppInstallerCLITests.vcxproj | 6 + .../AppInstallerCLITests.vcxproj.filters | 6 + src/AppInstallerCLITests/MsixInfo.cpp | 50 ++++--- .../PreIndexedPackageSource.cpp | 72 +++++----- src/AppInstallerCLITests/TestCommon.cpp | 75 ++++++++++- src/AppInstallerCLITests/TestCommon.h | 3 + .../TestData/index.1.0.0.0.signed.msix | Bin 0 -> 3823 bytes .../TestData/index.2.0.0.0.signed.msix | Bin 0 -> 3833 bytes src/AppInstallerCommonCore/MsixInfo.cpp | 125 +++++++++--------- .../Public/AppInstallerMsixInfo.h | 15 ++- .../PreIndexedPackageSourceFactory.cpp | 12 +- 12 files changed, 254 insertions(+), 114 deletions(-) create mode 100644 src/AppInstallerCLITests/TestData/index.1.0.0.0.signed.msix create mode 100644 src/AppInstallerCLITests/TestData/index.2.0.0.0.signed.msix diff --git a/src/AppInstallerCLIPackage/AppInstallerCLIPackage.wapproj b/src/AppInstallerCLIPackage/AppInstallerCLIPackage.wapproj index 0c49ab12e0..d659efee6b 100644 --- a/src/AppInstallerCLIPackage/AppInstallerCLIPackage.wapproj +++ b/src/AppInstallerCLIPackage/AppInstallerCLIPackage.wapproj @@ -71,7 +71,9 @@ - + + Designer + diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index 7fa93306a1..84707e0618 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -314,9 +314,15 @@ true + + true + true + + true + true diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index c1dbf3402e..6797ceb0fd 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -408,9 +408,15 @@ TestData + + TestData + TestData + + TestData + TestData diff --git a/src/AppInstallerCLITests/MsixInfo.cpp b/src/AppInstallerCLITests/MsixInfo.cpp index 84732d34a7..957af52d5b 100644 --- a/src/AppInstallerCLITests/MsixInfo.cpp +++ b/src/AppInstallerCLITests/MsixInfo.cpp @@ -3,6 +3,7 @@ #include "pch.h" #include "TestCommon.h" #include +#include "AppInstallerDownloader.h" using namespace std::string_literals; using namespace std::string_view_literals; @@ -11,6 +12,7 @@ using namespace AppInstaller; constexpr std::string_view s_MsixFile_1 = "index.1.0.0.0.msix"; constexpr std::string_view s_MsixFile_2 = "index.2.0.0.0.msix"; +constexpr std::string_view s_MsixFileSigned_1 = "index.1.0.0.0.signed.msix"; TEST_CASE("MsixInfo_GetPackageFamilyName", "[msixinfo]") { @@ -23,33 +25,21 @@ TEST_CASE("MsixInfo_GetPackageFamilyName", "[msixinfo]") REQUIRE(expectedFullName == actualFullName); } -TEST_CASE("MsixInfo_WriteManifestAndCompareToSelf", "[msixinfo]") +TEST_CASE("MsixInfo_CompareToSelf", "[msixinfo]") { TestDataFile index(s_MsixFile_1); Msix::MsixInfo msix(index.GetPath().u8string()); - TempFile manifest{ "msixtest_manifest"s, ".xml"s }; - ProgressCallback callback; - - msix.WriteManifestToFile(manifest, callback); - - REQUIRE(!msix.IsNewerThan(manifest)); + REQUIRE(!msix.IsNewerThan(index.GetPath().u8string())); } -TEST_CASE("MsixInfo_WriteManifestAndCompareToOlder", "[msixinfo]") +TEST_CASE("MsixInfo_CompareToOlder", "[msixinfo]") { TestDataFile index1(s_MsixFile_1); - Msix::MsixInfo msix1(index1.GetPath().u8string()); - - TempFile manifest{ "msixtest_manifest"s, ".xml"s }; - ProgressCallback callback; - - msix1.WriteManifestToFile(manifest, callback); - TestDataFile index2(s_MsixFile_2); Msix::MsixInfo msix2(index2.GetPath().u8string()); - REQUIRE(msix2.IsNewerThan(manifest)); + REQUIRE(msix2.IsNewerThan(index1)); } TEST_CASE("MsixInfo_WriteFile", "[msixinfo]") @@ -64,3 +54,31 @@ TEST_CASE("MsixInfo_WriteFile", "[msixinfo]") REQUIRE(1 == std::filesystem::file_size(file)); } + +TEST_CASE("MsixInfo_ValidateMsixTrustInfo", "[msixinfo]") +{ + TestDataFile notSigned{ s_MsixFile_1 }; + REQUIRE_FALSE(Msix::ValidateMsixTrustInfo(notSigned)); + + TestDataFile testSigned{ s_MsixFileSigned_1 }; + + // Remove the cert if already trusted + bool certExistsBeforeTest = UninstallCertFromSignedPackage(testSigned); + + REQUIRE_FALSE(Msix::ValidateMsixTrustInfo(testSigned)); + + // Add the cert to trusted + InstallCertFromSignedPackage(testSigned); + REQUIRE(Msix::ValidateMsixTrustInfo(testSigned)); + REQUIRE_FALSE(Msix::ValidateMsixTrustInfo(testSigned, true)); + + TestCommon::TempFile microsoftSigned{ "testIndex"s, ".msix"s }; + ProgressCallback callback; + Utility::Download("https://cdn.winget.microsoft.com/cache/source.msix", microsoftSigned.GetPath(), Utility::DownloadType::Index, callback); + REQUIRE(Msix::ValidateMsixTrustInfo(microsoftSigned, true)); + + if (!certExistsBeforeTest) + { + UninstallCertFromSignedPackage(testSigned); + } +} \ No newline at end of file diff --git a/src/AppInstallerCLITests/PreIndexedPackageSource.cpp b/src/AppInstallerCLITests/PreIndexedPackageSource.cpp index 591fe99152..d37528dac0 100644 --- a/src/AppInstallerCLITests/PreIndexedPackageSource.cpp +++ b/src/AppInstallerCLITests/PreIndexedPackageSource.cpp @@ -23,12 +23,10 @@ namespace fs = std::filesystem; constexpr std::string_view s_RepositorySettings_UserSources = "usersources"sv; -constexpr std::string_view s_MsixFile_1 = "index.1.0.0.0.msix"; -constexpr std::string_view s_MsixFile_2 = "index.2.0.0.0.msix"; -constexpr std::string_view s_Msix_FamilyName = "AppInstallerCLITestsFakeIndex_125rzkzqaqjwj"; -constexpr std::string_view s_AppxManifestFileName = "AppxManifest.xml"sv; +constexpr std::string_view s_MsixFile_1 = "index.1.0.0.0.signed.msix"; +constexpr std::string_view s_MsixFile_2 = "index.2.0.0.0.signed.msix"; +constexpr std::string_view s_Msix_FamilyName = "AppInstallerCLITestsFakeIndex_8wekyb3d8bbwe"; constexpr std::string_view s_IndexMsixName = "source.msix"sv; -constexpr std::string_view s_IndexFileName = "index.db"sv; void CopyIndexFileToDirectory(const fs::path& from, const fs::path& to) { @@ -71,6 +69,8 @@ TEST_CASE("PIPS_Add", "[pips]") TestDataFile index(s_MsixFile_1); CopyIndexFileToDirectory(index, dir); + bool shouldCleanCert = InstallCertFromSignedPackage(index); + SourceDetails details; details.Name = "TestName"; details.Type = AppInstaller::Repository::Microsoft::PreIndexedPackageSourceFactory::Type(); @@ -82,15 +82,15 @@ TEST_CASE("PIPS_Add", "[pips]") fs::path state = GetPathToFileDir(); REQUIRE(fs::exists(state)); - fs::path manifest = state; - manifest /= s_AppxManifestFileName; - REQUIRE(fs::exists(manifest)); - REQUIRE(fs::file_size(manifest) > 0); + fs::path indexMsix = state; + indexMsix /= s_IndexMsixName; + REQUIRE(fs::exists(indexMsix)); + REQUIRE(fs::file_size(indexMsix) > 0); - fs::path indexFile = state; - indexFile /= s_IndexFileName; - REQUIRE(fs::exists(indexFile)); - REQUIRE(fs::file_size(indexFile) > 0); + if (shouldCleanCert) + { + UninstallCertFromSignedPackage(index); + } } TEST_CASE("PIPS_UpdateSameVersion", "[pips]") @@ -101,6 +101,8 @@ TEST_CASE("PIPS_UpdateSameVersion", "[pips]") TestDataFile index(s_MsixFile_1); CopyIndexFileToDirectory(index, dir); + bool shouldCleanCert = InstallCertFromSignedPackage(index); + SourceDetails details; details.Name = "TestName"; details.Type = AppInstaller::Repository::Microsoft::PreIndexedPackageSourceFactory::Type(); @@ -117,6 +119,11 @@ TEST_CASE("PIPS_UpdateSameVersion", "[pips]") UpdateSource(details.Name, callback); REQUIRE(!progressCalled); + + if (shouldCleanCert) + { + UninstallCertFromSignedPackage(index); + } } TEST_CASE("PIPS_UpdateNewVersion", "[pips]") @@ -127,6 +134,8 @@ TEST_CASE("PIPS_UpdateNewVersion", "[pips]") TestDataFile indexMsix1(s_MsixFile_1); CopyIndexFileToDirectory(indexMsix1, dir); + bool shouldCleanCert = InstallCertFromSignedPackage(indexMsix1); + SourceDetails details; details.Name = "TestName"; details.Type = AppInstaller::Repository::Microsoft::PreIndexedPackageSourceFactory::Type(); @@ -138,13 +147,9 @@ TEST_CASE("PIPS_UpdateNewVersion", "[pips]") fs::path state = GetPathToFileDir(); REQUIRE(fs::exists(state)); - fs::path manifestPath = state; - manifestPath /= s_AppxManifestFileName; - std::string manifestContents1 = GetContents(manifestPath); - - fs::path indexPath = state; - indexPath /= s_IndexFileName; - std::string indexContents1 = GetContents(indexPath); + fs::path indexMsix = state; + indexMsix /= s_IndexMsixName; + std::string indexContents1 = GetContents(indexMsix); TestDataFile indexMsix2(s_MsixFile_2); CopyIndexFileToDirectory(indexMsix2, dir); @@ -155,11 +160,13 @@ TEST_CASE("PIPS_UpdateNewVersion", "[pips]") UpdateSource(details.Name, callback); REQUIRE(progressCalled); - std::string manifestContents2 = GetContents(manifestPath); - REQUIRE(manifestContents1 != manifestContents2); - - std::string indexContents2 = GetContents(indexPath); + std::string indexContents2 = GetContents(indexMsix); REQUIRE(indexContents1 != indexContents2); + + if (shouldCleanCert) + { + UninstallCertFromSignedPackage(indexMsix1); + } } TEST_CASE("PIPS_Remove", "[pips]") @@ -170,6 +177,8 @@ TEST_CASE("PIPS_Remove", "[pips]") TestDataFile index(s_MsixFile_1); CopyIndexFileToDirectory(index, dir); + bool shouldCleanCert = InstallCertFromSignedPackage(index); + SourceDetails details; details.Name = "TestName"; details.Type = AppInstaller::Repository::Microsoft::PreIndexedPackageSourceFactory::Type(); @@ -181,14 +190,15 @@ TEST_CASE("PIPS_Remove", "[pips]") fs::path state = GetPathToFileDir(); REQUIRE(fs::exists(state)); - fs::path manifest = state; - manifest /= s_AppxManifestFileName; - REQUIRE(fs::exists(manifest)); - - fs::path indexFile = state; - indexFile /= s_IndexFileName; - REQUIRE(fs::exists(indexFile)); + fs::path indexMsix = state; + indexMsix /= s_IndexMsixName; + REQUIRE(fs::exists(indexMsix)); RemoveSource(details.Name, callback); REQUIRE(!fs::exists(state)); + + if (shouldCleanCert) + { + UninstallCertFromSignedPackage(index); + } } diff --git a/src/AppInstallerCLITests/TestCommon.cpp b/src/AppInstallerCLITests/TestCommon.cpp index 67c0b1e074..b060300047 100644 --- a/src/AppInstallerCLITests/TestCommon.cpp +++ b/src/AppInstallerCLITests/TestCommon.cpp @@ -3,8 +3,9 @@ #include "pch.h" #include "TestCommon.h" #include "TestHooks.h" -#include "winget/GroupPolicy.h" -#include "winget/UserSettings.h" +#include +#include +#include namespace TestCommon { @@ -214,4 +215,74 @@ namespace TestCommon { AppInstaller::Settings::SetUserSettingsOverride(nullptr); } + + bool InstallCertFromSignedPackage(const std::filesystem::path& package) + { + auto [certContext, certStore] = AppInstaller::Msix::GetCertContextFromMsix(package); + + wil::unique_hcertstore trustedPeopleStore; + trustedPeopleStore.reset(CertOpenStore( + CERT_STORE_PROV_SYSTEM_W, + PKCS_7_ASN_ENCODING | X509_ASN_ENCODING, + NULL, + CERT_SYSTEM_STORE_LOCAL_MACHINE, + L"TrustedPeople")); + THROW_LAST_ERROR_IF(!trustedPeopleStore.get()); + + wil::unique_cert_context existingCert; + existingCert.reset(CertFindCertificateInStore( + trustedPeopleStore.get(), + PKCS_7_ASN_ENCODING | X509_ASN_ENCODING, + 0, + CERT_FIND_EXISTING, + certContext.get(), + nullptr)); + + // Add if it does not already exist in the store + if (!existingCert.get()) + { + THROW_LAST_ERROR_IF(!CertAddCertificateContextToStore( + trustedPeopleStore.get(), + certContext.get(), + CERT_STORE_ADD_NEW, + nullptr)); + + return true; + } + + return false; + } + + bool UninstallCertFromSignedPackage(const std::filesystem::path& package) + { + auto [certContext, certStore] = AppInstaller::Msix::GetCertContextFromMsix(package); + + wil::unique_hcertstore trustedPeopleStore; + trustedPeopleStore.reset(CertOpenStore( + CERT_STORE_PROV_SYSTEM_W, + PKCS_7_ASN_ENCODING | X509_ASN_ENCODING, + NULL, + CERT_SYSTEM_STORE_LOCAL_MACHINE, + L"TrustedPeople")); + THROW_LAST_ERROR_IF(!trustedPeopleStore.get()); + + wil::unique_cert_context existingCert; + existingCert.reset(CertFindCertificateInStore( + trustedPeopleStore.get(), + PKCS_7_ASN_ENCODING | X509_ASN_ENCODING, + 0, + CERT_FIND_EXISTING, + certContext.get(), + nullptr)); + + // Remove if it exists in the store + if (existingCert.get()) + { + THROW_LAST_ERROR_IF(!CertDeleteCertificateFromStore(existingCert.get())); + + return true; + } + + return false; + } } diff --git a/src/AppInstallerCLITests/TestCommon.h b/src/AppInstallerCLITests/TestCommon.h index d714073f49..4212a5961a 100644 --- a/src/AppInstallerCLITests/TestCommon.h +++ b/src/AppInstallerCLITests/TestCommon.h @@ -130,4 +130,7 @@ namespace TestCommon m_settings[S].emplace(std::move(value)); } }; + + bool InstallCertFromSignedPackage(const std::filesystem::path& package); + bool UninstallCertFromSignedPackage(const std::filesystem::path& package); } diff --git a/src/AppInstallerCLITests/TestData/index.1.0.0.0.signed.msix b/src/AppInstallerCLITests/TestData/index.1.0.0.0.signed.msix new file mode 100644 index 0000000000000000000000000000000000000000..0e97a4b40ab95890676ebcb5db658f1adf5a03c3 GIT binary patch literal 3823 zcmai12UL^WvJO3z2uMfNPz01-6bwZZx}ijh6r~d|2uWxHf>IK?peW^l^s3TP5H7u= zAXNmUh9Wgm6sf{Zj`BSBocrFJwf=whzxT}iYt1)n=GzE36}1Ea2mk<{MCKTi)c?pC z(hBO~f%No{f?}}OjF)x!X5RHBxXl4Gau4Q*bxiit;ppUV8$R7&S9WvD6Gv zT9@O-MYgx>v}*&@V8Dj)I=sDj;(mImW)v?C5e}DsPCXM;AHy`$*asE3N=<)tip?)X zJA~b4F_N*n4XP+sfQDPdEz6hZA1BFvGGfY0 zi9OpOCjOMJ+xgrpxpIB0#)LyrNp{D#gL}6JzfR)``S>k675nIR`qxWn;WWkqu^r&d zV8+vMaNOKG7OvMFE+hRQVg=J3YuQJmiFK!Y3^^{OT1536erxw`vQR=w#|cP8{ZXMO z;rBh_!M?m!bXw^gmChp@22T%vHa<)yr%AwAqNQe*$az@zkBH7|ojA*;qW(8sD=X8Z z43(DW900HCRqOcz)>K@FL*(^tfE}~~3|?5*C(!p4+IX`jLI-a+SIbDogM_Ya^=X|6UU_4t#*USY zDCX=oE?30Y6noS(#=D}zV(C~zD<_sy79)2E0NwV5TE`v`s!*r!qTJs38y5sU;nxyQ zjg}iH447n5>-TKmy|5nIsG~5HJ`5O*@!s-&Ks~T>^9VZLkQVxC!P=bnQKaRiY{G$t z{ZzPC)f3{~mIB^=MM|t}D2DRrwnOhq@}P($!^fJoJ>1Ia>ux&{*Q9-OvH0oDEU=8(4JW99wAf&nqB04?TA4`3p81xoHg6$Yml{nIH$!5CbYZN2a7a&F7tJ$uAX_oIs4p-pBRQon>)(VI*mCIXRB1Tt!$_Ogo|O^LA9F+6HZ`btBSv>T^>~ z|FbPQYOmUR<}7rCD_b?Efj(^4LkgC(z4sg=%>sT{?~^*|&sCvWU;Q4e5f9R~eXW%4u$cz3GSV9^`@;`!%BtgOxMko>sbT!UT!1We88~%vQ)^vX zD2S@|GNldivex`ujk);fxkyP&MG4U;VWY0%OhM#~+OFU5WnY&w^4bnpzILneuRd5!s2!VLL{V9#I+XQnDIE!ew-za!C@FG5Pf@Xt%DG{lc}3g8uR$J=NNbji>BUwW`9k1 zd>w(V+AafcL^gcw444ZV;4HrcfIP3f6uF%F3C$FFsgDN0;jIEjqth3yxN~ZIHm6D$ zPriC+!$Kp{t|Kk{Kt#&#gE4MJZ@)WA=Z<$)H=ptrQ(c0bdSYm3`5>rz(8Ofby&+wyHn}t3eGZ&Q=J@rI9W_+17Wp-~GqvvTgxbK7hw8!?O`!=^nc-@Xys-Ul zszkn9ukFmuVsBMlye~x|8kN$#d7no`Ntr2ylGK0Ilof8hx zBZZ=RxTLj^Va;i+_efQfiwoJGh0iXn%!^lCk=HdjbdT*SGbl>|TN`2wtIrWD5{HJT zjnY|5`f{y|tc3c9M^v4>z#WVPW0l3;sOjM|S56IPKY2{Jd~es5xPI+g&|(5#hg>P) zO}060wAKZ$o8Ei5EkHc3c$1c*}fi(iq6FTNcvyj&N?6P7Ke4KE3Ua3zXFJ-t}YaoVvidI9(S^K%1x z)Y7IyUB|t*PT$|J&$IE;mm{0&UtwxTzZxN#I8u~asy>cwwtDn!L+)pc1S~t+-v(gO z(+*v8jvD7qBss_5f6u)VQAQ{*eNs1vdlp;}CfML3pmxQlw!a?ITlW%gD8DQxzBw%= z=M@t+x>}Z%sOhJI>T-<`nvoyDUr{RZuD^RBJKaGA<61n^b=xXdUB>K`%{sd5Ev40F zzU`8=%WC=q@oxPk)=rG`GL;U4V(JBd0Sy>(!SrI0uKjYB@-mmcVZC64)JQ{bT0LnEkiZ63l$| zM&9B^gc7Yyqv(~TlYu?&2qt#Yd&SM=E_LTL)I!8qlOq!D2wfxBo4R)=S?kz z+>d>nk#y~IL*f~`V76-A#K65xysO0by?O(-yVa*!EuyksUJ$NXo-clq)6?|bIc`1f zJw)N+Y*EzNGQq(BDiQx>f%59JO!l$;P*}Fn3|JmIr%2^ zpYng-ixBYRSU)H2F)=3(TQZ6NEwQAnzftyP-ye-42J!>Q002Pb{cIh)`j@cbpAfFD z7&lizgp(86+!AYUgVA<#bit@Q+Bgb0IoL#^Z>WMPSSav@U^S$&7G8@GE;1ngUUgML z3IHGiswpFM^xHSa8o|K|kj}01Ekuyk;o;#nB1oG#4|6NH$_*e*sghCV1f+PND*1St zdw0hI*$|+lNY*^v7;4G)^f0|lStw<*Z9y}LP0#}GxG}Y*Xe3RGPG1{NLdYE+GRb}5MS%jL2DD` zSMhOCe5AGkL3YJ?a8`@Gu7MDBM0NytIQC&qo+68uaI!K|F?lJzgDRR#{F3646jN*; ze=(CG)nQx;m)sM4S^#R-ZYY;1ngyd#q-5R4aAjE{!YTl8s?omyA^RI3G*t zrJ${Y6~;@z%0lD}e(m^}7(j>yZW?1DCxpVwub}w#6L|j~jTUc($JI;2+`-lwdPIt8q@`ST>($!{OM1x;>b_ z#Ft1_fv^Yz-t62`@#!H@R3_C0%VvNyv`KKlg*IV|W=Wb}M(+wk=WrdUQ6CdSJvTkf zZ^aQlLp$lWYbYhdxpTYnzL8n=7`37TR=hYSWokS*g~BbE$+JhQ8$ns{Bx^-OQc2!l zPajGXE1|Vyb0@w5tzDa?Z>rCwnUbY3xDXnDXDJ{GNxKGty-f&F+ZEki5&H6QMd@mM7KVk9Z)yj-g8y_v_ zHeK=5M4{)&D6htTL%NfDkUOTf^4oi{IviFEBENCHim8>RGjw$>!>-S;nBFKUZM4DG zt-<7oW5W;JL>ej5Jl1@Zt+Bn=G>;W{i)0}T{H=DyI_*&l0W4AS+}@85j}d*>*4B3F zx=4j8#@4q}7l?}Ud5pSbTQ3c;QcwuB(=k12Z2QK9FM-qI(J_vt+x=BFZwdb|UjBda zDB3w%Vl~X22*qRfXviT&h%s!FdxJIBlNlTm6spdCcI?qDe)g*8ljn9iA*88+8fDu0 z>HZFd$_6j{IG-IKH(7eSjEz6pQa9=W(gfNYwq&G6q;7ElVpfj|X{ZW>2A!dBFo0?P z?!o@ZQXRcjb4&E|?c1OXofrF{=UmX@axtumA$x~zlJY36ga%C<#z@E~dh8WZCph6_ z@C&V_Wv6j>>*d5u{%;i+r9NDp@tSWORArV{o!eFSCTHi%tK>P^*X3{;@n#iWB*wf$ z!$`(9J1?TBvpz#c4`t7&RPoQ{J$?^t*MJF+I-TA8Gx%-Y-Ew z%a?dObH>rYptc(fNWC#%*dou;BU}jawvfqdOtRrF@k6*b(&!vs&P7!}-(Ji|uD`HJ z>uvu`TFWLnDo^!!sHC9M@+q4MTZs0;%CVQ9e{)N!eSuD{f3fIA$rUa0J%qW8wZ{^T zOu#h{+s>n%qh*nzrY@S>9pMws66P$kU~8;REga^R;Kxp+X=-+WNA(m`PD7HXx@9bVIM7{+zBVf3w94X;`oE92M8C;%adgyHj-SKtFny^rRr_DJyrcS}N zD2-C$ls~6A_L4C&ND3lr&gq%B)F3Ncz*HZoW|gePsrP0?(YgeExa7!}zF>!_LWjPo!q4Cr1gKDKMP@HKCMrZC*YyD8l zjwsN*;hq64>4N-DJu8_P!%bY_D$?`7`o4a^57R?@J3YxIG4Ld3OZXuv-1OsI6A1uN zLJ9!T;d4glqT04L4(4v|E*JqP39r|;e8e8xUP#*%5v8)M8X2_{FgRPE_0^vt?2)>_ z+M{SLD-MpD&R02ASGId-vC>>m>9Z)s;{;ykQmPH%hR=CKGHVqZg)fOnZNT%it1Ih| zzc+2lH!ttb4s3d-FS~nGeFea4BeLJnPeyhY&@!J}T(To^gM?j*0_AgkEas5r0U*pH zkbpKi`i?;0bJ~F*b{)QTqBaLJ8OjmRfe-}(c;?0DiCdcN5Qgje43>;I65l@m;PZES)wNZh(@l4THh-c2q@#?v-1ZGFbSB>c(C zjW$-x_fSazJH3?q!KkNo@V=R!6t3!;@vw~tZwBRuiV`cg?$sEn`tOa1`~+|c7mQZe zPfIHJEk>)_u{4y5mU{u5WZy}3)cCjD9bml^v3yNlO~ys`M~P=vqde?siP&AX$@Y-6 z2m84Uf90yyX;BlLc!4kP=AutzIp zDKHgMSL-uSsg45pk z!Vp%kE)7da>6d@7WZRFCd7Gq(AvOywpM-R_PNXD>T@?ZWwB4xDRsE)`Ch%eXPBzO_3R5twZY0hRHm22ePm>65>e8{tt*cbD z&aNBx=~5pX`d$ak)f;`%^Z)pzBu;Pl4ZK#nKl)Qa9riNt2u!+hkgsasnV?hPmEtso z{qaJSUM#(=k-?}U+h&s8<> za1#TMxNOLef*}z+UCCP?*cz;2YnGYsTto#3kVAM1C~1Hx zclRY|yFcU$`0_ok!2R!Yq&8_<0;V)FRytn&`vY?E>5T&R(Vr z?J*6{2`DcQI4Flx=MPqR+@b;jzie;|!#ETFSf5jHe!q+)(I&2}q%zy{X1~9hDRmhe zl|@EQzV>kw>K^n(`&~x$AD?d7=l0~k?iH_#xcC)kp8;3CL*w^MtI}}2(aqE_PJ$n^ z_wMIt3-0SPt&kj7&`@^rggmB(o656Pi_IvYxqJ1_^vM1u+x1zMr`BA;l=k=Pen6=E zSbVxYS)v_`H~#oEtxoQ}D3U5WRAH-W#8FHU2h}-z_qdrg9U_qOt*>VoF0VBzon*Lb zHniFnk3PJL-B#InHT7%4~k zp&-?Ws??6J6#bwN^OGx<)In3AkQ#mU#l<|=ejqisqzQB@iNE>dW;exQu9qZV|C*ok zbgo>yi}H<9PtjvbU!I-s(J;?6jhrnEbaZuxHJB^j+aO$Mw;4YMPntc^8GgX;GtntY zClGYPndsl;f1i-hsMA;{9=DH?&E5!m~0nZrB(mCOIt z`;-e1c)-7VU%kHqop}Fup78tITlh)%A6Ubm1N^6<`8xm signedMessage; + wil::unique_hcertstore certStore; + CRYPT_DATA_BLOB signatureBlob = { 0 }; + signatureBlob.cbData = static_cast(signature.size()); + signatureBlob.pbData = signature.data(); + THROW_LAST_ERROR_IF(!CryptQueryObject( + CERT_QUERY_OBJECT_BLOB, + &signatureBlob, + CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED, + CERT_QUERY_FORMAT_FLAG_BINARY, + 0, // Reserved parameter + NULL, // No encoding info needed + NULL, + NULL, + &certStore, + &signedMessage, + NULL)); + + // Get the signer size and information from the signed data message + // The properties of the signer info will be used to uniquely identify the signing certificate in the certificate store + DWORD signerInfoSize = 0; + THROW_LAST_ERROR_IF(!CryptMsgGetParam( + signedMessage.get(), + CMSG_SIGNER_INFO_PARAM, + 0, + NULL, + &signerInfoSize)); + + // Check that the signer info size is within reasonable bounds; under the max length of a string for the issuer field + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_DATA), !(signerInfoSize > 0 && signerInfoSize < STRSAFE_MAX_CCH)); + + std::vector signerInfoBuffer; + signerInfoBuffer.resize(signerInfoSize); + THROW_LAST_ERROR_IF(!CryptMsgGetParam( + signedMessage.get(), + CMSG_SIGNER_INFO_PARAM, + 0, + signerInfoBuffer.data(), + &signerInfoSize)); + + // Get the signing certificate from the certificate store based on the issuer and serial number of the signer info + CMSG_SIGNER_INFO* signerInfo = reinterpret_cast(signerInfoBuffer.data()); + CERT_INFO certInfo; + certInfo.Issuer = signerInfo->Issuer; + certInfo.SerialNumber = signerInfo->SerialNumber; + + wil::unique_cert_context certContext; + certContext.reset(CertGetSubjectCertificateFromStore( + certStore.get(), + X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, + &certInfo)); + THROW_LAST_ERROR_IF(!certContext.get()); + + return { std::move(certContext), std::move(certStore) }; + } + bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin) { bool result = true; @@ -354,65 +419,7 @@ namespace AppInstaller::Msix // First verify certificate chain if requested. if (verifyMicrosoftOrigin) { - // Retrieve raw signature from msix - MsixInfo msixInfo{ msixPath.u8string() }; - auto signature = msixInfo.GetSignature(); - THROW_HR_IF(E_UNEXPECTED, signature.size() <= P7xFileIdSize); - signature.erase(signature.begin(), signature.begin() + P7xFileIdSize); - - // Get the cert content - wil::unique_any signedMessage; - wil::unique_hcertstore certStore; - CRYPT_DATA_BLOB signatureBlob = { 0 }; - signatureBlob.cbData = static_cast(signature.size()); - signatureBlob.pbData = signature.data(); - THROW_LAST_ERROR_IF(!CryptQueryObject( - CERT_QUERY_OBJECT_BLOB, - &signatureBlob, - CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED, - CERT_QUERY_FORMAT_FLAG_BINARY, - 0, // Reserved parameter - NULL, // No encoding info needed - NULL, - NULL, - &certStore, - &signedMessage, - NULL)); - - // Get the signer size and information from the signed data message - // The properties of the signer info will be used to uniquely identify the signing certificate in the certificate store - DWORD signerInfoSize = 0; - THROW_LAST_ERROR_IF(!CryptMsgGetParam( - signedMessage.get(), - CMSG_SIGNER_INFO_PARAM, - 0, - NULL, - &signerInfoSize)); - - // Check that the signer info size is within reasonable bounds; under the max length of a string for the issuer field - THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_DATA), !(signerInfoSize > 0 && signerInfoSize < STRSAFE_MAX_CCH)); - - std::vector signerInfoBuffer; - signerInfoBuffer.resize(signerInfoSize); - THROW_LAST_ERROR_IF(!CryptMsgGetParam( - signedMessage.get(), - CMSG_SIGNER_INFO_PARAM, - 0, - signerInfoBuffer.data(), - &signerInfoSize)); - - // Get the signing certificate from the certificate store based on the issuer and serial number of the signer info - CMSG_SIGNER_INFO* signerInfo = reinterpret_cast(signerInfoBuffer.data()); - CERT_INFO certInfo; - certInfo.Issuer = signerInfo->Issuer; - certInfo.SerialNumber = signerInfo->SerialNumber; - - wil::unique_cert_context certContext; - certContext.reset(CertGetSubjectCertificateFromStore( - certStore.get(), - X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - &certInfo)); - THROW_LAST_ERROR_IF(!certContext.get()); + auto [certContext, certStore] = GetCertContextFromMsix(msixPath); // Get certificate chain context for validation CERT_CHAIN_PARA certChainParameters = { 0 }; diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h index 6cc303c31f..7e09187162 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -42,9 +42,6 @@ namespace AppInstaller::Msix // Gets the package location from the given full name. std::optional GetPackageLocationFromFullName(std::string_view fullName); - // Validate the msix file trust info, also validate the signing cert is from Microsoft origin if verifyMicrosoftOrigin is true; - bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin = false); - // MsixInfo class handles all appx/msix related query. struct MsixInfo { @@ -88,4 +85,16 @@ namespace AppInstaller::Msix Microsoft::WRL::ComPtr m_bundleReader; Microsoft::WRL::ComPtr m_packageReader; }; + + struct GetCertContextResult + { + wil::unique_cert_context CertContext; + wil::unique_hcertstore CertStore; + }; + + // Get cert context from a signed msix/msixbundle file. + GetCertContextResult GetCertContextFromMsix(const std::filesystem::path& msixPath); + + // Validate the msix file trust info, also validate the signing cert is from Microsoft origin if verifyMicrosoftOrigin is true; + bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin = false); } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index a825562fd9..fd385a4004 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -420,7 +420,15 @@ namespace AppInstaller::Repository::Microsoft } std::filesystem::path tempPackagePath = packagePath.u8string() + ".dlnd.msix"; - AppInstaller::Utility::Download(packageLocation, tempPackagePath, AppInstaller::Utility::DownloadType::Index, progress); + if (Utility::IsUrlRemote(packageLocation)) + { + AppInstaller::Utility::Download(packageLocation, tempPackagePath, AppInstaller::Utility::DownloadType::Index, progress); + } + else + { + std::filesystem::copy(packageLocation, tempPackagePath); + progress.OnProgress(100, 100, ProgressType::Percent); + } bool updateSuccess = false; if (progress.IsCancelled()) @@ -435,7 +443,7 @@ namespace AppInstaller::Repository::Microsoft } else { - AICLI_LOG(Repo, Info, << "Source update failed. Source package failed trust validation."); + AICLI_LOG(Repo, Error, << "Source update failed. Source package failed trust validation."); } if (!updateSuccess) From c0fe9505dcf3e7ab3f75fc362ec2195a18517d03 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Wed, 4 May 2022 19:38:01 -0700 Subject: [PATCH 08/13] spelling --- .github/actions/spelling/allow.txt | 12 ++++++++++++ .github/actions/spelling/expect.txt | 3 +++ .../Microsoft/PreIndexedPackageSourceFactory.cpp | 2 +- .../Microsoft/TempSQLiteIndexFile.cpp | 10 +++++----- .../Microsoft/TempSQLiteIndexFile.h | 2 +- 5 files changed, 22 insertions(+), 7 deletions(-) diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt index 5630c89465..f96cc61867 100644 --- a/.github/actions/spelling/allow.txt +++ b/.github/actions/spelling/allow.txt @@ -61,6 +61,7 @@ cmake cmdlet cmdlets cmp +CMSG CNG cnt codepage @@ -186,6 +187,9 @@ getline github githubusercontent globals +HCCE +hcertstore +HCRYPTMSG hfile HGLOBAL HIDECANCEL @@ -206,6 +210,7 @@ hstring html http https +HWND Hyperlink IActivation IApplication @@ -279,6 +284,7 @@ logsql logto LONGLONG LPCGUID +LPCSTR LPVOID mailto MAJORVERSION @@ -351,6 +357,7 @@ nuspec OAuth ODR ofstream +Oid opencode opensource openxmlformats @@ -382,6 +389,7 @@ PGP php PII pipssource +PKCS placeholders png posix @@ -492,6 +500,7 @@ src srwlock sscanf sstream +STATEACTION STATFLAG STATSTG STDAPI @@ -516,6 +525,7 @@ STRINGID STRINGIFY STRINGIZE stringstream +STRSAFE strstr strtoll subcontext @@ -639,6 +649,7 @@ winmeta winres winrt winsqlite +WINTRUST wix wmain WNS @@ -653,6 +664,7 @@ wrl WStr wstring wstringstream +WTD www xamarin xaml diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index ed9a178013..f18999d57b 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -120,6 +120,7 @@ EXEHASH experimentalfeatures fcb fd +fdw fedorapeople fileinuse fintimes @@ -278,7 +279,9 @@ parametermap pathparts pathpaths Patil +pb PCs +pcwsz pdp PEGI pfn diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index fd385a4004..49692d227f 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -419,7 +419,7 @@ namespace AppInstaller::Repository::Microsoft } } - std::filesystem::path tempPackagePath = packagePath.u8string() + ".dlnd.msix"; + std::filesystem::path tempPackagePath = packagePath.u8string() + ".dnld.msix"; if (Utility::IsUrlRemote(packageLocation)) { AppInstaller::Utility::Download(packageLocation, tempPackagePath, AppInstaller::Utility::DownloadType::Index, progress); diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp index 2668bc266a..37cfe9c91c 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp @@ -20,19 +20,19 @@ namespace AppInstaller::Repository::Microsoft m_indexFile = Runtime::GetPathTo(Runtime::PathName::Temp); m_indexFile /= tempFileName; - m_indexHanlde.reset(CreateFileW(m_indexFile.c_str(), GENERIC_WRITE, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); - THROW_LAST_ERROR_IF(!m_indexHanlde); + m_indexHandle.reset(CreateFileW(m_indexFile.c_str(), GENERIC_WRITE, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); + THROW_LAST_ERROR_IF(!m_indexHandle); - packageInfo.WriteToFileHandle(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexHanlde.get(), progress); + packageInfo.WriteToFileHandle(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexHandle.get(), progress); } TempSQLiteIndexFile::~TempSQLiteIndexFile() { if (!m_indexFile.empty() && std::filesystem::exists(m_indexFile)) { - if (m_indexHanlde) + if (m_indexHandle) { - m_indexHanlde.reset(); + m_indexHandle.reset(); } try diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h index ca94a4a544..5facf1cdd3 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h +++ b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h @@ -24,6 +24,6 @@ namespace AppInstaller::Repository::Microsoft private: std::filesystem::path m_indexFile; - wil::unique_handle m_indexHanlde; + wil::unique_handle m_indexHandle; }; } From cc99720944effd6757c76862f8b809b2cbba4fad Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Tue, 10 May 2022 20:40:37 -0700 Subject: [PATCH 09/13] Comments --- src/AppInstallerCLITests/MsixInfo.cpp | 9 +++- .../PreIndexedPackageSource.cpp | 24 +++++++++ src/AppInstallerCLITests/TestCommon.h | 2 + .../AppInstallerCommonCore.vcxproj | 2 + .../AppInstallerCommonCore.vcxproj.filters | 6 +++ src/AppInstallerCommonCore/ManagedFile.cpp | 49 +++++++++++++++++++ src/AppInstallerCommonCore/MsixInfo.cpp | 36 ++++++++++---- .../Public/AppInstallerMsixInfo.h | 3 +- .../Public/winget/ManagedFile.h | 36 ++++++++++++++ .../AppInstallerRepositoryCore.vcxproj | 2 - ...AppInstallerRepositoryCore.vcxproj.filters | 6 --- .../PreIndexedPackageSourceFactory.cpp | 20 ++++++-- .../Microsoft/SQLiteIndex.cpp | 12 ++--- .../Microsoft/SQLiteIndex.h | 8 +-- .../Microsoft/TempSQLiteIndexFile.cpp | 48 ------------------ .../Microsoft/TempSQLiteIndexFile.h | 29 ----------- 16 files changed, 182 insertions(+), 110 deletions(-) create mode 100644 src/AppInstallerCommonCore/ManagedFile.cpp create mode 100644 src/AppInstallerCommonCore/Public/winget/ManagedFile.h delete mode 100644 src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp delete mode 100644 src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h diff --git a/src/AppInstallerCLITests/MsixInfo.cpp b/src/AppInstallerCLITests/MsixInfo.cpp index 957af52d5b..e159266e24 100644 --- a/src/AppInstallerCLITests/MsixInfo.cpp +++ b/src/AppInstallerCLITests/MsixInfo.cpp @@ -3,7 +3,8 @@ #include "pch.h" #include "TestCommon.h" #include -#include "AppInstallerDownloader.h" +#include +#include using namespace std::string_literals; using namespace std::string_view_literals; @@ -57,6 +58,12 @@ TEST_CASE("MsixInfo_WriteFile", "[msixinfo]") TEST_CASE("MsixInfo_ValidateMsixTrustInfo", "[msixinfo]") { + if (!Runtime::IsRunningAsAdmin()) + { + INFO("Test requires admin privilege. Skipped."); + return; + } + TestDataFile notSigned{ s_MsixFile_1 }; REQUIRE_FALSE(Msix::ValidateMsixTrustInfo(notSigned)); diff --git a/src/AppInstallerCLITests/PreIndexedPackageSource.cpp b/src/AppInstallerCLITests/PreIndexedPackageSource.cpp index d37528dac0..4d0d826bf3 100644 --- a/src/AppInstallerCLITests/PreIndexedPackageSource.cpp +++ b/src/AppInstallerCLITests/PreIndexedPackageSource.cpp @@ -63,6 +63,12 @@ void CleanSources() TEST_CASE("PIPS_Add", "[pips]") { + if (!Runtime::IsRunningAsAdmin()) + { + INFO("Test requires admin privilege. Skipped."); + return; + } + CleanSources(); TempDirectory dir("pipssource"); @@ -95,6 +101,12 @@ TEST_CASE("PIPS_Add", "[pips]") TEST_CASE("PIPS_UpdateSameVersion", "[pips]") { + if (!Runtime::IsRunningAsAdmin()) + { + INFO("Test requires admin privilege. Skipped."); + return; + } + CleanSources(); TempDirectory dir("pipssource"); @@ -128,6 +140,12 @@ TEST_CASE("PIPS_UpdateSameVersion", "[pips]") TEST_CASE("PIPS_UpdateNewVersion", "[pips]") { + if (!Runtime::IsRunningAsAdmin()) + { + INFO("Test requires admin privilege. Skipped."); + return; + } + CleanSources(); TempDirectory dir("pipssource"); @@ -171,6 +189,12 @@ TEST_CASE("PIPS_UpdateNewVersion", "[pips]") TEST_CASE("PIPS_Remove", "[pips]") { + if (!Runtime::IsRunningAsAdmin()) + { + INFO("Test requires admin privilege. Skipped."); + return; + } + CleanSources(); TempDirectory dir("pipssource"); diff --git a/src/AppInstallerCLITests/TestCommon.h b/src/AppInstallerCLITests/TestCommon.h index 4212a5961a..41925af841 100644 --- a/src/AppInstallerCLITests/TestCommon.h +++ b/src/AppInstallerCLITests/TestCommon.h @@ -131,6 +131,8 @@ namespace TestCommon } }; + // Below cert installation/uninstallation methods require admin privilege, + // tests calling these functions should skip when not running with admin. bool InstallCertFromSignedPackage(const std::filesystem::path& package); bool UninstallCertFromSignedPackage(const std::filesystem::path& package); } diff --git a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj index b5c15e781f..0f8d0acdcb 100644 --- a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj +++ b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj @@ -308,6 +308,7 @@ + @@ -364,6 +365,7 @@ + diff --git a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters index 20527b7be7..120bfe039e 100644 --- a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters +++ b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters @@ -189,6 +189,9 @@ Public\winget + + Public\winget + @@ -326,6 +329,9 @@ Source Files + + Source Files + diff --git a/src/AppInstallerCommonCore/ManagedFile.cpp b/src/AppInstallerCommonCore/ManagedFile.cpp new file mode 100644 index 0000000000..9d0ad500c8 --- /dev/null +++ b/src/AppInstallerCommonCore/ManagedFile.cpp @@ -0,0 +1,49 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "winget/ManagedFile.h" +#include "AppInstallerLogging.h" + +namespace AppInstaller::Utility +{ + ManagedFile ManagedFile::CreateWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess, bool deleteOnExit) + { + ManagedFile file; + file.m_fileHandle.reset(CreateFileW(path.c_str(), desiredAccess, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); + THROW_LAST_ERROR_IF(!file.m_fileHandle); + file.m_filePath = path; + file.m_deleteFileOnExit = deleteOnExit; + + return file; + } + + ManagedFile ManagedFile::OpenWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess) + { + ManagedFile file; + file.m_fileHandle.reset(CreateFileW(path.c_str(), desiredAccess, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr)); + THROW_LAST_ERROR_IF(!file.m_fileHandle); + file.m_filePath = path; + + return file; + } + + ManagedFile::~ManagedFile() + { + if (m_deleteFileOnExit) + { + if (m_fileHandle) + { + m_fileHandle.reset(); + } + + try + { + std::filesystem::remove(m_filePath); + } + catch (...) + { + AICLI_LOG(Core, Info, << "Failed to remove managed file at: " << m_filePath); + } + } + } +} \ No newline at end of file diff --git a/src/AppInstallerCommonCore/MsixInfo.cpp b/src/AppInstallerCommonCore/MsixInfo.cpp index ce5110ec10..344d50cdf6 100644 --- a/src/AppInstallerCommonCore/MsixInfo.cpp +++ b/src/AppInstallerCommonCore/MsixInfo.cpp @@ -17,7 +17,7 @@ namespace AppInstaller::Msix namespace { // MSIX-specific header placed in the P7X file, before the actual signature - const DWORD P7xFileId = 0x58434b50; + const byte P7xFileId[] = { 0x50, 0x4b, 0x43, 0x58 }; const DWORD P7xFileIdSize = sizeof(P7xFileId); // Gets the version from the manifest reader. @@ -348,9 +348,7 @@ namespace AppInstaller::Msix { // Retrieve raw signature from msix MsixInfo msixInfo{ msixPath.u8string() }; - auto signature = msixInfo.GetSignature(); - THROW_HR_IF(E_UNEXPECTED, signature.size() <= P7xFileIdSize); - signature.erase(signature.begin(), signature.begin() + P7xFileIdSize); + auto signature = msixInfo.GetSignature(true); // Get the cert content wil::unique_any signedMessage; @@ -411,11 +409,13 @@ namespace AppInstaller::Msix bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin) { - bool result = true; + bool result = false; AICLI_LOG(Core, Info, << "Started trust validation of msix at: " << msixPath); try { + bool verifyChainResult = false; + // First verify certificate chain if requested. if (verifyMicrosoftOrigin) { @@ -453,11 +453,15 @@ namespace AppInstaller::Msix AICLI_LOG(Core, Info, << "Result for certificate chain validation of Microsoft origin: " << policyStatus.dwError); - result = certChainVerifySucceeded && policyStatus.dwError == ERROR_SUCCESS; + verifyChainResult = certChainVerifySucceeded && policyStatus.dwError == ERROR_SUCCESS; + } + else + { + verifyChainResult = true; } // If certificate chain origin validation is success or not requested, then validate the trust info of the file. - if (result) + if (verifyChainResult) { // Set up the structures needed for the WinVerifyTrust call WINTRUST_FILE_INFO fileInfo = { 0 }; @@ -467,10 +471,10 @@ namespace AppInstaller::Msix WINTRUST_DATA trustData = { 0 }; trustData.cbStruct = sizeof(WINTRUST_DATA); trustData.dwUIChoice = WTD_UI_NONE; - trustData.fdwRevocationChecks = WTD_REVOKE_NONE; + trustData.fdwRevocationChecks = WTD_REVOKE_WHOLECHAIN; trustData.dwUnionChoice = WTD_CHOICE_FILE; trustData.dwStateAction = WTD_STATEACTION_VERIFY; - trustData.dwProvFlags = WTD_CACHE_ONLY_URL_RETRIEVAL | WTD_REVOCATION_CHECK_NONE; + trustData.dwProvFlags = WTD_CACHE_ONLY_URL_RETRIEVAL; trustData.pFile = &fileInfo; GUID verifyActionId = WINTRUST_ACTION_GENERIC_VERIFY_V2; @@ -530,7 +534,7 @@ namespace AppInstaller::Msix } } - std::vector MsixInfo::GetSignature() + std::vector MsixInfo::GetSignature(bool getRawSignature) { ComPtr signatureFile; if (m_isBundle) @@ -552,6 +556,18 @@ namespace AppInstaller::Msix THROW_IF_FAILED(signatureStream->Stat(&stat, STATFLAG_NONAME)); THROW_HR_IF(E_UNEXPECTED, stat.cbSize.HighPart != 0); // Signature size should be small signatureSize = stat.cbSize.LowPart; + THROW_HR_IF(E_UNEXPECTED, signatureSize <= P7xFileIdSize); + + if (getRawSignature) + { + // Validate msix signature header + byte headerBuffer[P7xFileIdSize]; + DWORD headerRead; + THROW_IF_FAILED(signatureStream->Read(headerBuffer, P7xFileIdSize, &headerRead)); + THROW_HR_IF_MSG(E_UNEXPECTED, headerRead != P7xFileIdSize, "Failed to read signature header"); + THROW_HR_IF_MSG(E_UNEXPECTED, !std::equal(P7xFileId, P7xFileId + P7xFileIdSize, headerBuffer), "Unexpected msix signature header"); + signatureSize -= P7xFileIdSize; + } signatureContent.resize(signatureSize); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h index 7e09187162..5e340e2ced 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -59,7 +59,8 @@ namespace AppInstaller::Msix } // Full content of AppxSignature.p7x - std::vector GetSignature(); + // If getRawSignature is true, returns content of converted .p7s + std::vector GetSignature(bool getRawSignature = false); // Gets the package full name. std::wstring GetPackageFullNameWide(); diff --git a/src/AppInstallerCommonCore/Public/winget/ManagedFile.h b/src/AppInstallerCommonCore/Public/winget/ManagedFile.h new file mode 100644 index 0000000000..eb9e02936e --- /dev/null +++ b/src/AppInstallerCommonCore/Public/winget/ManagedFile.h @@ -0,0 +1,36 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once +#include +#include + +namespace AppInstaller::Utility +{ + // Struct that holds a file handle and may perform additional operations on exit + struct ManagedFile + { + ManagedFile() = default; + + ManagedFile(const ManagedFile&) = delete; + ManagedFile& operator=(const ManagedFile&) = delete; + + ManagedFile(ManagedFile&&) = default; + ManagedFile& operator=(ManagedFile&&) = default; + + HANDLE GetFileHandle() { return m_fileHandle.get(); } + const std::filesystem::path& GetFilePath() { return m_filePath; } + + // Always creates a new write locked file at the path given. desiredAccess is passed to CreateFile call. + static ManagedFile CreateWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess, bool deleteOnExit); + + // Always opens an exising file at the path given with write locked. desiredAccess is passed to CreateFile call. + static ManagedFile OpenWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess); + + ~ManagedFile(); + + private: + std::filesystem::path m_filePath; + wil::unique_handle m_fileHandle; + bool m_deleteFileOnExit = false; + }; +} \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj index 7040bdb73a..0f0df2c4b5 100644 --- a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj +++ b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj @@ -269,7 +269,6 @@ - @@ -336,7 +335,6 @@ - diff --git a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters index 5905fc068a..78d9b98747 100644 --- a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters +++ b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters @@ -267,9 +267,6 @@ Public\winget - - Microsoft - @@ -422,9 +419,6 @@ Source Files - - Microsoft - diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index 49692d227f..723ae2a572 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -4,10 +4,10 @@ #include "Microsoft/PreIndexedPackageSourceFactory.h" #include "Microsoft/SQLiteIndex.h" #include "Microsoft/SQLiteIndexSource.h" -#include "TempSQLiteIndexFile.h" #include #include +#include using namespace std::string_literals; using namespace std::string_view_literals; @@ -369,10 +369,24 @@ namespace AppInstaller::Repository::Microsoft THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_MISSING); } + // Put a write exclusive lock on the index package. + auto indexPackageLock = Utility::ManagedFile::OpenWriteLockedFile(packageLocation, 0); + + // Validate index package trust info. THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !Msix::ValidateMsixTrustInfo(packageLocation, WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin))); + // Create a temp lock exclusive index file. + GUID guid; + THROW_IF_FAILED(CoCreateGuid(&guid)); + WCHAR tempFileName[256]; + THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0); + auto tempIndexFilePath = Runtime::GetPathTo(Runtime::PathName::Temp); + tempIndexFilePath /= tempFileName; + auto tempIndexFile = Utility::ManagedFile::CreateWriteLockedFile(tempIndexFilePath, GENERIC_WRITE, true); + + // Populate temp index file. Msix::MsixInfo packageInfo(packageLocation.u8string()); - TempSQLiteIndexFile tempIndexFile{ packageInfo, progress }; + packageInfo.WriteToFileHandle(s_PreIndexedPackageSourceFactory_IndexFilePath, tempIndexFile.GetFileHandle(), progress); if (progress.IsCancelled()) { @@ -380,7 +394,7 @@ namespace AppInstaller::Repository::Microsoft return {}; } - SQLiteIndex index = SQLiteIndex::Open(tempIndexFile.GetIndexFilePath().u8string(), SQLiteIndex::OpenDisposition::Immutable, std::move(tempIndexFile)); + SQLiteIndex index = SQLiteIndex::Open(tempIndexFile.GetFilePath().u8string(), SQLiteIndex::OpenDisposition::Immutable, std::move(tempIndexFile)); // We didn't use to store the source identifier, so we compute it here in case it's // missing from the details. diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp index a65b6c8ac3..1a23bc3cfc 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp @@ -45,15 +45,15 @@ namespace AppInstaller::Repository::Microsoft return result; } - SQLiteIndex SQLiteIndex::Open(const std::string& filePath, OpenDisposition disposition, TempSQLiteIndexFile&& tempIndexFileHandle) + SQLiteIndex SQLiteIndex::Open(const std::string& filePath, OpenDisposition disposition, Utility::ManagedFile&& indexFile) { AICLI_LOG(Repo, Info, << "Opening SQLite Index for " << GetOpenDispositionString(disposition) << " at '" << filePath << "'"); switch (disposition) { case AppInstaller::Repository::Microsoft::SQLiteIndex::OpenDisposition::Read: - return { filePath, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::None, std::move(tempIndexFileHandle) }; + return { filePath, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::None, std::move(indexFile) }; case AppInstaller::Repository::Microsoft::SQLiteIndex::OpenDisposition::ReadWrite: - return { filePath, SQLite::Connection::OpenDisposition::ReadWrite, SQLite::Connection::OpenFlags::None, std::move(tempIndexFileHandle) }; + return { filePath, SQLite::Connection::OpenDisposition::ReadWrite, SQLite::Connection::OpenFlags::None, std::move(indexFile) }; case AppInstaller::Repository::Microsoft::SQLiteIndex::OpenDisposition::Immutable: { // Following the algorithm set forth at https://sqlite.org/uri.html [3.1] to convert to a URI path @@ -99,15 +99,15 @@ namespace AppInstaller::Repository::Microsoft target += "?immutable=1"; - return { target, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::Uri, std::move(tempIndexFileHandle) }; + return { target, SQLite::Connection::OpenDisposition::ReadOnly, SQLite::Connection::OpenFlags::Uri, std::move(indexFile) }; } default: THROW_HR(E_UNEXPECTED); } } - SQLiteIndex::SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags, TempSQLiteIndexFile&& tempIndexFileHandle) : - m_dbconn(SQLite::Connection::Create(target, disposition, flags)), m_tempIndexFileHandle(std::move(tempIndexFileHandle)) + SQLiteIndex::SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags, Utility::ManagedFile&& indexFile) : + m_dbconn(SQLite::Connection::Create(target, disposition, flags)), m_indexFile(std::move(indexFile)) { m_dbconn.EnableICU(); m_version = Schema::Version::GetSchemaVersion(m_dbconn); diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h index e74520b579..05fe0e613d 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h +++ b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h @@ -5,11 +5,11 @@ #include "Microsoft/Schema/ISQLiteIndex.h" #include "Microsoft/Schema/Version.h" #include "ISource.h" -#include "TempSQLiteIndexFile.h" #include #include #include #include +#include #include #include @@ -58,7 +58,7 @@ namespace AppInstaller::Repository::Microsoft }; // Opens an existing index database. - static SQLiteIndex Open(const std::string& filePath, OpenDisposition disposition, TempSQLiteIndexFile&& tempIndexFileHandle = {}); + static SQLiteIndex Open(const std::string& filePath, OpenDisposition disposition, Utility::ManagedFile&& indexFile = {}); // Gets the schema version of the index. Schema::Version GetVersion() const { return m_version; } @@ -152,7 +152,7 @@ namespace AppInstaller::Repository::Microsoft std::vector> GetDependentsById(AppInstaller::Manifest::string_t packageId) const; private: // Constructor used to open an existing index. - SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags, TempSQLiteIndexFile&& tempIndexFileHandle); + SQLiteIndex(const std::string& target, SQLite::Connection::OpenDisposition disposition, SQLite::Connection::OpenFlags flags, Utility::ManagedFile&& indexFile); // Constructor used to create a new index. SQLiteIndex(const std::string& target, Schema::Version version); @@ -164,7 +164,7 @@ namespace AppInstaller::Repository::Microsoft // Sets the last write time metadata value in the index. void SetLastWriteTime(); - TempSQLiteIndexFile m_tempIndexFileHandle; + Utility::ManagedFile m_indexFile; SQLite::Connection m_dbconn; Schema::Version m_version; std::unique_ptr m_interface; diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp deleted file mode 100644 index 37cfe9c91c..0000000000 --- a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. -#include "pch.h" -#include "TempSQLiteIndexFile.h" - -namespace AppInstaller::Repository::Microsoft -{ - namespace - { - static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv; - } - - TempSQLiteIndexFile::TempSQLiteIndexFile(AppInstaller::Msix::MsixInfo& packageInfo, AppInstaller::IProgressCallback& progress) - { - GUID guid; - THROW_IF_FAILED(CoCreateGuid(&guid)); - WCHAR tempFileName[256]; - THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0); - - m_indexFile = Runtime::GetPathTo(Runtime::PathName::Temp); - m_indexFile /= tempFileName; - - m_indexHandle.reset(CreateFileW(m_indexFile.c_str(), GENERIC_WRITE, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)); - THROW_LAST_ERROR_IF(!m_indexHandle); - - packageInfo.WriteToFileHandle(s_PreIndexedPackageSourceFactory_IndexFilePath, m_indexHandle.get(), progress); - } - - TempSQLiteIndexFile::~TempSQLiteIndexFile() - { - if (!m_indexFile.empty() && std::filesystem::exists(m_indexFile)) - { - if (m_indexHandle) - { - m_indexHandle.reset(); - } - - try - { - std::filesystem::remove(m_indexFile); - } - catch (...) - { - AICLI_LOG(Repo, Info, << "Failed to remove temp index file at: " << m_indexFile); - } - } - } -} \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h b/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h deleted file mode 100644 index 5facf1cdd3..0000000000 --- a/src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.h +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. -#pragma once -#include -#include -namespace AppInstaller::Repository::Microsoft -{ - // Holds a temp SQLite index file extracted from index package, exclusive to the process. - struct TempSQLiteIndexFile - { - TempSQLiteIndexFile() = default; - - TempSQLiteIndexFile(AppInstaller::Msix::MsixInfo& packageInfo, AppInstaller::IProgressCallback& progress); - - TempSQLiteIndexFile(const TempSQLiteIndexFile&) = delete; - TempSQLiteIndexFile& operator=(const TempSQLiteIndexFile&) = delete; - - TempSQLiteIndexFile(TempSQLiteIndexFile&&) = default; - TempSQLiteIndexFile& operator=(TempSQLiteIndexFile&&) = default; - - const std::filesystem::path& GetIndexFilePath() { return m_indexFile; } - - ~TempSQLiteIndexFile(); - - private: - std::filesystem::path m_indexFile; - wil::unique_handle m_indexHandle; - }; -} From f78810524ae8a31bb80a4e5b669bb1e61860d2da Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Tue, 10 May 2022 21:33:33 -0700 Subject: [PATCH 10/13] spelling --- .github/actions/spelling/allow.txt | 1 + src/AppInstallerCommonCore/Public/winget/ManagedFile.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt index d30a3167a3..0ad1f5eb73 100644 --- a/.github/actions/spelling/allow.txt +++ b/.github/actions/spelling/allow.txt @@ -640,6 +640,7 @@ wcout wcsicmp webpage wekyb +WHOLECHAIN wil wildcards WINAPI diff --git a/src/AppInstallerCommonCore/Public/winget/ManagedFile.h b/src/AppInstallerCommonCore/Public/winget/ManagedFile.h index eb9e02936e..8f90e42f79 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManagedFile.h +++ b/src/AppInstallerCommonCore/Public/winget/ManagedFile.h @@ -23,7 +23,7 @@ namespace AppInstaller::Utility // Always creates a new write locked file at the path given. desiredAccess is passed to CreateFile call. static ManagedFile CreateWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess, bool deleteOnExit); - // Always opens an exising file at the path given with write locked. desiredAccess is passed to CreateFile call. + // Always opens an existing file at the path given with write locked. desiredAccess is passed to CreateFile call. static ManagedFile OpenWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess); ~ManagedFile(); From de5ca81b1d212fc3dd9c1715771674bc85f78392 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Wed, 11 May 2022 18:18:25 -0700 Subject: [PATCH 11/13] More comments --- src/AppInstallerCLITests/MsixInfo.cpp | 10 ++-- .../PreIndexedPackageSource.cpp | 8 +-- src/AppInstallerCommonCore/MsixInfo.cpp | 13 +++-- .../Public/AppInstallerMsixInfo.h | 7 ++- .../Public/AppInstallerRuntime.h | 3 + .../Public/winget/ManagedFile.h | 4 +- src/AppInstallerCommonCore/Runtime.cpp | 12 ++++ .../PreIndexedPackageSourceFactory.cpp | 57 +++++++++++++------ 8 files changed, 79 insertions(+), 35 deletions(-) diff --git a/src/AppInstallerCLITests/MsixInfo.cpp b/src/AppInstallerCLITests/MsixInfo.cpp index e159266e24..b5fc248da2 100644 --- a/src/AppInstallerCLITests/MsixInfo.cpp +++ b/src/AppInstallerCLITests/MsixInfo.cpp @@ -18,7 +18,7 @@ constexpr std::string_view s_MsixFileSigned_1 = "index.1.0.0.0.signed.msix"; TEST_CASE("MsixInfo_GetPackageFamilyName", "[msixinfo]") { TestDataFile index(s_MsixFile_1); - Msix::MsixInfo msix(index.GetPath().u8string()); + Msix::MsixInfo msix(index.GetPath()); std::string expectedFullName = "AppInstallerCLITestsFakeIndex_1.0.0.0_neutral__125rzkzqaqjwj"; std::string actualFullName = msix.GetPackageFullName(); @@ -29,7 +29,7 @@ TEST_CASE("MsixInfo_GetPackageFamilyName", "[msixinfo]") TEST_CASE("MsixInfo_CompareToSelf", "[msixinfo]") { TestDataFile index(s_MsixFile_1); - Msix::MsixInfo msix(index.GetPath().u8string()); + Msix::MsixInfo msix(index.GetPath()); REQUIRE(!msix.IsNewerThan(index.GetPath().u8string())); } @@ -38,7 +38,7 @@ TEST_CASE("MsixInfo_CompareToOlder", "[msixinfo]") { TestDataFile index1(s_MsixFile_1); TestDataFile index2(s_MsixFile_2); - Msix::MsixInfo msix2(index2.GetPath().u8string()); + Msix::MsixInfo msix2(index2.GetPath()); REQUIRE(msix2.IsNewerThan(index1)); } @@ -46,7 +46,7 @@ TEST_CASE("MsixInfo_CompareToOlder", "[msixinfo]") TEST_CASE("MsixInfo_WriteFile", "[msixinfo]") { TestDataFile index(s_MsixFile_1); - Msix::MsixInfo msix(index.GetPath().u8string()); + Msix::MsixInfo msix(index.GetPath()); TempFile file{ "msixtest_file"s, ".bin"s }; ProgressCallback callback; @@ -60,7 +60,7 @@ TEST_CASE("MsixInfo_ValidateMsixTrustInfo", "[msixinfo]") { if (!Runtime::IsRunningAsAdmin()) { - INFO("Test requires admin privilege. Skipped."); + WARN("Test requires admin privilege. Skipped."); return; } diff --git a/src/AppInstallerCLITests/PreIndexedPackageSource.cpp b/src/AppInstallerCLITests/PreIndexedPackageSource.cpp index 4d0d826bf3..cdffb2945e 100644 --- a/src/AppInstallerCLITests/PreIndexedPackageSource.cpp +++ b/src/AppInstallerCLITests/PreIndexedPackageSource.cpp @@ -65,7 +65,7 @@ TEST_CASE("PIPS_Add", "[pips]") { if (!Runtime::IsRunningAsAdmin()) { - INFO("Test requires admin privilege. Skipped."); + WARN("Test requires admin privilege. Skipped."); return; } @@ -103,7 +103,7 @@ TEST_CASE("PIPS_UpdateSameVersion", "[pips]") { if (!Runtime::IsRunningAsAdmin()) { - INFO("Test requires admin privilege. Skipped."); + WARN("Test requires admin privilege. Skipped."); return; } @@ -142,7 +142,7 @@ TEST_CASE("PIPS_UpdateNewVersion", "[pips]") { if (!Runtime::IsRunningAsAdmin()) { - INFO("Test requires admin privilege. Skipped."); + WARN("Test requires admin privilege. Skipped."); return; } @@ -191,7 +191,7 @@ TEST_CASE("PIPS_Remove", "[pips]") { if (!Runtime::IsRunningAsAdmin()) { - INFO("Test requires admin privilege. Skipped."); + WARN("Test requires admin privilege. Skipped."); return; } diff --git a/src/AppInstallerCommonCore/MsixInfo.cpp b/src/AppInstallerCommonCore/MsixInfo.cpp index 344d50cdf6..a76e28d6e3 100644 --- a/src/AppInstallerCommonCore/MsixInfo.cpp +++ b/src/AppInstallerCommonCore/MsixInfo.cpp @@ -67,6 +67,8 @@ namespace AppInstaller::Msix // If we got bytes, just accept them and keep going. LOG_IF_FAILED(hr); + THROW_HR_IF_MSG(E_UNEXPECTED, expectedSize && totalBytesRead + bytesRead > expectedSize, "Read more bytes than expected size"); + file.write(buffer.get(), bytesRead); totalBytesRead += bytesRead; progress.OnProgress(totalBytesRead, expectedSize, ProgressType::Bytes); @@ -140,7 +142,8 @@ namespace AppInstaller::Msix // If we got bytes, just accept them and keep going. LOG_IF_FAILED(hr); - THROW_LAST_ERROR_IF(INVALID_SET_FILE_POINTER == SetFilePointer(target, 0, nullptr, FILE_END)); + THROW_HR_IF_MSG(E_UNEXPECTED, expectedSize && totalBytesRead + bytesRead > expectedSize, "Read more bytes than expected size"); + DWORD bytesWritten = 0; THROW_LAST_ERROR_IF(!WriteFile(target, buffer.get(), bytesRead, &bytesWritten, nullptr)); THROW_HR_IF(E_UNEXPECTED, bytesRead != bytesWritten); @@ -347,7 +350,7 @@ namespace AppInstaller::Msix GetCertContextResult GetCertContextFromMsix(const std::filesystem::path& msixPath) { // Retrieve raw signature from msix - MsixInfo msixInfo{ msixPath.u8string() }; + MsixInfo msixInfo{ msixPath }; auto signature = msixInfo.GetSignature(true); // Get the cert content @@ -534,7 +537,7 @@ namespace AppInstaller::Msix } } - std::vector MsixInfo::GetSignature(bool getRawSignature) + std::vector MsixInfo::GetSignature(bool skipP7xFileId) { ComPtr signatureFile; if (m_isBundle) @@ -558,7 +561,7 @@ namespace AppInstaller::Msix signatureSize = stat.cbSize.LowPart; THROW_HR_IF(E_UNEXPECTED, signatureSize <= P7xFileIdSize); - if (getRawSignature) + if (skipP7xFileId) { // Validate msix signature header byte headerBuffer[P7xFileIdSize]; @@ -609,7 +612,7 @@ namespace AppInstaller::Msix { THROW_HR_IF(E_NOT_VALID_STATE, m_isBundle); - MsixInfo other{ otherPackage.u8string() }; + MsixInfo other{ otherPackage }; THROW_HR_IF(E_INVALIDARG, other.m_isBundle); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h index 5e340e2ced..a350062389 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -47,6 +47,9 @@ namespace AppInstaller::Msix { MsixInfo(std::string_view uriStr); + template, int> = 0> + MsixInfo(const T& path) : MsixInfo(path.u8string()) {} + MsixInfo(const MsixInfo&) = default; MsixInfo& operator=(const MsixInfo&) = default; @@ -59,8 +62,8 @@ namespace AppInstaller::Msix } // Full content of AppxSignature.p7x - // If getRawSignature is true, returns content of converted .p7s - std::vector GetSignature(bool getRawSignature = false); + // If skipP7xFileId is true, returns content of converted .p7s + std::vector GetSignature(bool skipP7xFileId = false); // Gets the package full name. std::wstring GetPackageFullNameWide(); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h index aff42a9258..9d757b1411 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h @@ -64,6 +64,9 @@ namespace AppInstaller::Runtime // Gets the path to the requested location. std::filesystem::path GetPathTo(PathName path); + // Gets a new temp file path. + std::filesystem::path GetNewTempFilePath(); + // Determines whether the current OS version is >= the given one. // We treat the given Version struct as a standard 4 part Windows OS version. bool IsCurrentOSVersionGreaterThanOrEqual(const Utility::Version& version); diff --git a/src/AppInstallerCommonCore/Public/winget/ManagedFile.h b/src/AppInstallerCommonCore/Public/winget/ManagedFile.h index 8f90e42f79..7288c828a7 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManagedFile.h +++ b/src/AppInstallerCommonCore/Public/winget/ManagedFile.h @@ -17,8 +17,8 @@ namespace AppInstaller::Utility ManagedFile(ManagedFile&&) = default; ManagedFile& operator=(ManagedFile&&) = default; - HANDLE GetFileHandle() { return m_fileHandle.get(); } - const std::filesystem::path& GetFilePath() { return m_filePath; } + HANDLE GetFileHandle() const { return m_fileHandle.get(); } + const std::filesystem::path& GetFilePath() const { return m_filePath; } // Always creates a new write locked file at the path given. desiredAccess is passed to CreateFile call. static ManagedFile CreateWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess, bool deleteOnExit); diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index 955f7904e4..655f62a704 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -504,6 +504,18 @@ namespace AppInstaller::Runtime return result; } + std::filesystem::path GetNewTempFilePath() + { + GUID guid; + THROW_IF_FAILED(CoCreateGuid(&guid)); + WCHAR tempFileName[256]; + THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0); + auto tempFilePath = Runtime::GetPathTo(Runtime::PathName::Temp); + tempFilePath /= tempFileName; + + return tempFilePath; + } + bool IsCurrentOSVersionGreaterThanOrEqual(const Utility::Version& version) { DWORD versionParts[3] = {}; diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index 723ae2a572..71ed5c7ffc 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -21,6 +21,22 @@ namespace AppInstaller::Repository::Microsoft // TODO: This being hard coded to force using the Public directory name is not ideal. static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv; + struct PersistedIndexPackage + { + PersistedIndexPackage(const std::filesystem::path& path) + { + m_file = Utility::ManagedFile::OpenWriteLockedFile(path, 0); + } + + bool ValidateMsixTrustInfo(bool checkMicrosoftOrigin) + { + return Msix::ValidateMsixTrustInfo(m_file.GetFilePath(), checkMicrosoftOrigin); + } + + private: + Utility::ManagedFile m_file; + }; + // Construct the package location from the given details. // Currently expects that the arg is an https uri pointing to the root of the data. std::string GetPackageLocation(const SourceDetails& details) @@ -370,22 +386,17 @@ namespace AppInstaller::Repository::Microsoft } // Put a write exclusive lock on the index package. - auto indexPackageLock = Utility::ManagedFile::OpenWriteLockedFile(packageLocation, 0); + PersistedIndexPackage indexPackage{ packageLocation }; // Validate index package trust info. - THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !Msix::ValidateMsixTrustInfo(packageLocation, WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin))); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !indexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin))); // Create a temp lock exclusive index file. - GUID guid; - THROW_IF_FAILED(CoCreateGuid(&guid)); - WCHAR tempFileName[256]; - THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0); - auto tempIndexFilePath = Runtime::GetPathTo(Runtime::PathName::Temp); - tempIndexFilePath /= tempFileName; + auto tempIndexFilePath = Runtime::GetNewTempFilePath(); auto tempIndexFile = Utility::ManagedFile::CreateWriteLockedFile(tempIndexFilePath, GENERIC_WRITE, true); // Populate temp index file. - Msix::MsixInfo packageInfo(packageLocation.u8string()); + Msix::MsixInfo packageInfo(packageLocation); packageInfo.WriteToFileHandle(s_PreIndexedPackageSourceFactory_IndexFilePath, tempIndexFile.GetFileHandle(), progress); if (progress.IsCancelled()) @@ -425,7 +436,8 @@ namespace AppInstaller::Repository::Microsoft if (std::filesystem::exists(packagePath)) { // If we already have a trusted index package, use it to determine if we need to update or not. - if (Msix::ValidateMsixTrustInfo(packagePath, WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) && + PersistedIndexPackage indexPackage{ packagePath }; + if (indexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) && !packageInfo.IsNewerThan(packagePath)) { AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed"); @@ -449,15 +461,26 @@ namespace AppInstaller::Repository::Microsoft { AICLI_LOG(Repo, Info, << "Cancelling update upon request"); } - else if (Msix::ValidateMsixTrustInfo(tempPackagePath, WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin))) - { - std::filesystem::rename(tempPackagePath, packagePath); - AICLI_LOG(Repo, Info, << "Source update success."); - updateSuccess = true; - } else { - AICLI_LOG(Repo, Error, << "Source update failed. Source package failed trust validation."); + bool tempIndexPackageTrusted = false; + + { + // Extra scope to release the file lock right after trust validation. + PersistedIndexPackage tempIndexPackage{ tempPackagePath }; + tempIndexPackageTrusted = tempIndexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)); + } + + if (tempIndexPackageTrusted) + { + std::filesystem::rename(tempPackagePath, packagePath); + AICLI_LOG(Repo, Info, << "Source update success."); + updateSuccess = true; + } + else + { + AICLI_LOG(Repo, Error, << "Source update failed. Source package failed trust validation."); + } } if (!updateSuccess) From afa5499d5419728dd6ed5f9c3af2ccd707d2f3f9 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Wed, 11 May 2022 18:24:38 -0700 Subject: [PATCH 12/13] One more const --- .../Microsoft/PreIndexedPackageSourceFactory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index 71ed5c7ffc..fe1cb1ea37 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -28,7 +28,7 @@ namespace AppInstaller::Repository::Microsoft m_file = Utility::ManagedFile::OpenWriteLockedFile(path, 0); } - bool ValidateMsixTrustInfo(bool checkMicrosoftOrigin) + bool ValidateMsixTrustInfo(bool checkMicrosoftOrigin) const { return Msix::ValidateMsixTrustInfo(m_file.GetFilePath(), checkMicrosoftOrigin); } From d5c64d0345d74150e0684e408720eaee65561775 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 13 May 2022 16:43:42 -0700 Subject: [PATCH 13/13] Move PersistedMsixFile to common --- src/AppInstallerCLITests/MsixInfo.cpp | 15 +- src/AppInstallerCommonCore/MsixInfo.cpp | 194 +++++++++--------- .../Public/AppInstallerMsixInfo.h | 15 +- .../PreIndexedPackageSourceFactory.cpp | 28 +-- 4 files changed, 130 insertions(+), 122 deletions(-) diff --git a/src/AppInstallerCLITests/MsixInfo.cpp b/src/AppInstallerCLITests/MsixInfo.cpp index b5fc248da2..7bcc3dca6d 100644 --- a/src/AppInstallerCLITests/MsixInfo.cpp +++ b/src/AppInstallerCLITests/MsixInfo.cpp @@ -65,24 +65,29 @@ TEST_CASE("MsixInfo_ValidateMsixTrustInfo", "[msixinfo]") } TestDataFile notSigned{ s_MsixFile_1 }; - REQUIRE_FALSE(Msix::ValidateMsixTrustInfo(notSigned)); + Msix::WriteLockedMsixFile notSignedWriteLocked{ notSigned }; + REQUIRE_FALSE(notSignedWriteLocked.ValidateTrustInfo(false)); TestDataFile testSigned{ s_MsixFileSigned_1 }; + Msix::WriteLockedMsixFile testSignedWriteLocked{ testSigned }; // Remove the cert if already trusted bool certExistsBeforeTest = UninstallCertFromSignedPackage(testSigned); - REQUIRE_FALSE(Msix::ValidateMsixTrustInfo(testSigned)); + REQUIRE_FALSE(testSignedWriteLocked.ValidateTrustInfo(false)); // Add the cert to trusted InstallCertFromSignedPackage(testSigned); - REQUIRE(Msix::ValidateMsixTrustInfo(testSigned)); - REQUIRE_FALSE(Msix::ValidateMsixTrustInfo(testSigned, true)); + + REQUIRE(testSignedWriteLocked.ValidateTrustInfo(false)); + REQUIRE_FALSE(testSignedWriteLocked.ValidateTrustInfo(true)); TestCommon::TempFile microsoftSigned{ "testIndex"s, ".msix"s }; ProgressCallback callback; Utility::Download("https://cdn.winget.microsoft.com/cache/source.msix", microsoftSigned.GetPath(), Utility::DownloadType::Index, callback); - REQUIRE(Msix::ValidateMsixTrustInfo(microsoftSigned, true)); + + Msix::WriteLockedMsixFile microsoftSignedWriteLocked{ microsoftSigned }; + REQUIRE(microsoftSignedWriteLocked.ValidateTrustInfo(true)); if (!certExistsBeforeTest) { diff --git a/src/AppInstallerCommonCore/MsixInfo.cpp b/src/AppInstallerCommonCore/MsixInfo.cpp index a76e28d6e3..8dc7b93bf3 100644 --- a/src/AppInstallerCommonCore/MsixInfo.cpp +++ b/src/AppInstallerCommonCore/MsixInfo.cpp @@ -186,6 +186,98 @@ namespace AppInstaller::Msix WriteStreamToFileHandle(stream.Get(), size, target, progress); } + + bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin) + { + bool result = false; + AICLI_LOG(Core, Info, << "Started trust validation of msix at: " << msixPath); + + try + { + bool verifyChainResult = false; + + // First verify certificate chain if requested. + if (verifyMicrosoftOrigin) + { + auto [certContext, certStore] = GetCertContextFromMsix(msixPath); + + // Get certificate chain context for validation + CERT_CHAIN_PARA certChainParameters = { 0 }; + certChainParameters.cbSize = sizeof(CERT_CHAIN_PARA); + certChainParameters.RequestedUsage.dwType = USAGE_MATCH_TYPE_AND; + DWORD certChainFlags = CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL; + + wil::unique_cert_chain_context certChainContext; + THROW_LAST_ERROR_IF(!CertGetCertificateChain( + HCCE_LOCAL_MACHINE, + certContext.get(), + NULL, // Use the current system time for CRL validation + certStore.get(), + &certChainParameters, + certChainFlags, + NULL, // Reserved parameter; must be NULL + &certChainContext)); + + // Validate that the certificate chain is rooted in one of the well-known Microsoft root certs + CERT_CHAIN_POLICY_PARA policyParameters = { 0 }; + policyParameters.cbSize = sizeof(CERT_CHAIN_POLICY_PARA); + policyParameters.dwFlags = MICROSOFT_ROOT_CERT_CHAIN_POLICY_CHECK_APPLICATION_ROOT_FLAG; + CERT_CHAIN_POLICY_STATUS policyStatus = { 0 }; + policyStatus.cbSize = sizeof(CERT_CHAIN_POLICY_STATUS); + LPCSTR policyOid = CERT_CHAIN_POLICY_MICROSOFT_ROOT; + BOOL certChainVerifySucceeded = CertVerifyCertificateChainPolicy( + policyOid, + certChainContext.get(), + &policyParameters, + &policyStatus); + + AICLI_LOG(Core, Info, << "Result for certificate chain validation of Microsoft origin: " << policyStatus.dwError); + + verifyChainResult = certChainVerifySucceeded && policyStatus.dwError == ERROR_SUCCESS; + } + else + { + verifyChainResult = true; + } + + // If certificate chain origin validation is success or not requested, then validate the trust info of the file. + if (verifyChainResult) + { + // Set up the structures needed for the WinVerifyTrust call + WINTRUST_FILE_INFO fileInfo = { 0 }; + fileInfo.cbStruct = sizeof(WINTRUST_FILE_INFO); + fileInfo.pcwszFilePath = msixPath.c_str(); + + WINTRUST_DATA trustData = { 0 }; + trustData.cbStruct = sizeof(WINTRUST_DATA); + trustData.dwUIChoice = WTD_UI_NONE; + trustData.fdwRevocationChecks = WTD_REVOKE_WHOLECHAIN; + trustData.dwUnionChoice = WTD_CHOICE_FILE; + trustData.dwStateAction = WTD_STATEACTION_VERIFY; + trustData.dwProvFlags = WTD_CACHE_ONLY_URL_RETRIEVAL; + trustData.pFile = &fileInfo; + + GUID verifyActionId = WINTRUST_ACTION_GENERIC_VERIFY_V2; + + HRESULT verifyTrustResult = static_cast(WinVerifyTrust(static_cast(INVALID_HANDLE_VALUE), &verifyActionId, &trustData)); + AICLI_LOG(Core, Info, << "Result for trust info validation of the msix: " << verifyTrustResult); + + result = verifyTrustResult == S_OK; + } + } + catch (const wil::ResultException& re) + { + AICLI_LOG(Core, Error, << "Failed during msix trust validation. Error: " << re.GetErrorCode()); + result = false; + } + catch (...) + { + AICLI_LOG(Core, Error, << "Failed during msix trust validation."); + result = false; + } + + return result; + } } bool GetBundleReader( @@ -410,98 +502,6 @@ namespace AppInstaller::Msix return { std::move(certContext), std::move(certStore) }; } - bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin) - { - bool result = false; - AICLI_LOG(Core, Info, << "Started trust validation of msix at: " << msixPath); - - try - { - bool verifyChainResult = false; - - // First verify certificate chain if requested. - if (verifyMicrosoftOrigin) - { - auto [certContext, certStore] = GetCertContextFromMsix(msixPath); - - // Get certificate chain context for validation - CERT_CHAIN_PARA certChainParameters = { 0 }; - certChainParameters.cbSize = sizeof(CERT_CHAIN_PARA); - certChainParameters.RequestedUsage.dwType = USAGE_MATCH_TYPE_AND; - DWORD certChainFlags = CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL; - - wil::unique_cert_chain_context certChainContext; - THROW_LAST_ERROR_IF(!CertGetCertificateChain( - HCCE_LOCAL_MACHINE, - certContext.get(), - NULL, // Use the current system time for CRL validation - certStore.get(), - &certChainParameters, - certChainFlags, - NULL, // Reserved parameter; must be NULL - &certChainContext)); - - // Validate that the certificate chain is rooted in one of the well-known Microsoft root certs - CERT_CHAIN_POLICY_PARA policyParameters = { 0 }; - policyParameters.cbSize = sizeof(CERT_CHAIN_POLICY_PARA); - policyParameters.dwFlags = MICROSOFT_ROOT_CERT_CHAIN_POLICY_CHECK_APPLICATION_ROOT_FLAG; - CERT_CHAIN_POLICY_STATUS policyStatus = { 0 }; - policyStatus.cbSize = sizeof(CERT_CHAIN_POLICY_STATUS); - LPCSTR policyOid = CERT_CHAIN_POLICY_MICROSOFT_ROOT; - BOOL certChainVerifySucceeded = CertVerifyCertificateChainPolicy( - policyOid, - certChainContext.get(), - &policyParameters, - &policyStatus); - - AICLI_LOG(Core, Info, << "Result for certificate chain validation of Microsoft origin: " << policyStatus.dwError); - - verifyChainResult = certChainVerifySucceeded && policyStatus.dwError == ERROR_SUCCESS; - } - else - { - verifyChainResult = true; - } - - // If certificate chain origin validation is success or not requested, then validate the trust info of the file. - if (verifyChainResult) - { - // Set up the structures needed for the WinVerifyTrust call - WINTRUST_FILE_INFO fileInfo = { 0 }; - fileInfo.cbStruct = sizeof(WINTRUST_FILE_INFO); - fileInfo.pcwszFilePath = msixPath.c_str(); - - WINTRUST_DATA trustData = { 0 }; - trustData.cbStruct = sizeof(WINTRUST_DATA); - trustData.dwUIChoice = WTD_UI_NONE; - trustData.fdwRevocationChecks = WTD_REVOKE_WHOLECHAIN; - trustData.dwUnionChoice = WTD_CHOICE_FILE; - trustData.dwStateAction = WTD_STATEACTION_VERIFY; - trustData.dwProvFlags = WTD_CACHE_ONLY_URL_RETRIEVAL; - trustData.pFile = &fileInfo; - - GUID verifyActionId = WINTRUST_ACTION_GENERIC_VERIFY_V2; - - HRESULT verifyTrustResult = static_cast(WinVerifyTrust(static_cast(INVALID_HANDLE_VALUE), &verifyActionId, &trustData)); - AICLI_LOG(Core, Info, << "Result for trust info validation of the msix: " << verifyTrustResult); - - result = verifyTrustResult == S_OK; - } - } - catch (const wil::ResultException& re) - { - AICLI_LOG(Core, Error, << "Failed during msix trust validation. Error: " << re.GetErrorCode()); - result = false; - } - catch (...) - { - AICLI_LOG(Core, Error, << "Failed during msix trust validation."); - result = false; - } - - return result; - } - MsixInfo::MsixInfo(std::string_view uriStr) { if (Utility::IsUrlRemote(uriStr)) @@ -683,4 +683,14 @@ namespace AppInstaller::Msix WriteAppxFileToFileHandle(appxFile.Get(), target, progress); } + + WriteLockedMsixFile::WriteLockedMsixFile(const std::filesystem::path& path) + { + m_file = Utility::ManagedFile::OpenWriteLockedFile(path, 0); + } + + bool WriteLockedMsixFile::ValidateTrustInfo(bool checkMicrosoftOrigin) const + { + return ValidateMsixTrustInfo(m_file.GetFilePath(), checkMicrosoftOrigin); + } } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h index a350062389..2118ba37b5 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -1,7 +1,9 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once -#include +#include "AppInstallerProgress.h" +#include "winget/ManagedFile.h" + #include #include @@ -99,6 +101,13 @@ namespace AppInstaller::Msix // Get cert context from a signed msix/msixbundle file. GetCertContextResult GetCertContextFromMsix(const std::filesystem::path& msixPath); - // Validate the msix file trust info, also validate the signing cert is from Microsoft origin if verifyMicrosoftOrigin is true; - bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin = false); + struct WriteLockedMsixFile + { + WriteLockedMsixFile(const std::filesystem::path& path); + + bool ValidateTrustInfo(bool checkMicrosoftOrigin) const; + + private: + Utility::ManagedFile m_file; + }; } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp index fe1cb1ea37..e80b2ede73 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -21,22 +21,6 @@ namespace AppInstaller::Repository::Microsoft // TODO: This being hard coded to force using the Public directory name is not ideal. static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv; - struct PersistedIndexPackage - { - PersistedIndexPackage(const std::filesystem::path& path) - { - m_file = Utility::ManagedFile::OpenWriteLockedFile(path, 0); - } - - bool ValidateMsixTrustInfo(bool checkMicrosoftOrigin) const - { - return Msix::ValidateMsixTrustInfo(m_file.GetFilePath(), checkMicrosoftOrigin); - } - - private: - Utility::ManagedFile m_file; - }; - // Construct the package location from the given details. // Currently expects that the arg is an https uri pointing to the root of the data. std::string GetPackageLocation(const SourceDetails& details) @@ -386,10 +370,10 @@ namespace AppInstaller::Repository::Microsoft } // Put a write exclusive lock on the index package. - PersistedIndexPackage indexPackage{ packageLocation }; + Msix::WriteLockedMsixFile indexPackage{ packageLocation }; // Validate index package trust info. - THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !indexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin))); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !indexPackage.ValidateTrustInfo(WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin))); // Create a temp lock exclusive index file. auto tempIndexFilePath = Runtime::GetNewTempFilePath(); @@ -436,8 +420,8 @@ namespace AppInstaller::Repository::Microsoft if (std::filesystem::exists(packagePath)) { // If we already have a trusted index package, use it to determine if we need to update or not. - PersistedIndexPackage indexPackage{ packagePath }; - if (indexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) && + Msix::WriteLockedMsixFile indexPackage{ packagePath }; + if (indexPackage.ValidateTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) && !packageInfo.IsNewerThan(packagePath)) { AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed"); @@ -467,8 +451,8 @@ namespace AppInstaller::Repository::Microsoft { // Extra scope to release the file lock right after trust validation. - PersistedIndexPackage tempIndexPackage{ tempPackagePath }; - tempIndexPackageTrusted = tempIndexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)); + Msix::WriteLockedMsixFile tempIndexPackage{ tempPackagePath }; + tempIndexPackageTrusted = tempIndexPackage.ValidateTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)); } if (tempIndexPackageTrusted)