Load index from validated msix for unpackaged context #2139

yao-msft · 2022-05-05T03:19:03Z

Validation: Added new tests and fixed existing tests. Also did manual verification that source opening works as expected.

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS · 2022-05-09T22:52:36Z

src/AppInstallerCLITests/TestCommon.cpp

@@ -214,4 +215,74 @@ namespace TestCommon
    {
        AppInstaller::Settings::SetUserSettingsOverride(nullptr);
    }
+
+    bool InstallCertFromSignedPackage(const std::filesystem::path& package)


Not a fan of requiring the tests to run as admin.
Also not a fan of the tests changing the security profile of the machine they are run on.

Could we possibly test with a package signed by something already trusted? #Resolved

I made the tests to skip if not running with admin. But I cannot find a way to "mark" the tests as skipped. So I just output an info message.

JohnMcPMS · 2022-05-09T22:57:23Z

src/AppInstallerCommonCore/MsixInfo.cpp

-        ComPtr<IStream> otherStream;
-        THROW_IF_FAILED(SHCreateStreamOnFileEx(otherManifest.c_str(), 
-            STGM_READ | STGM_SHARE_DENY_WRITE | STGM_FAILIFTHERE, 0, FALSE, nullptr, &otherStream));
+        MsixInfo other{ otherPackage.u8string() };


Should we just have a MsixInfo(const std::filesystem::path&) constructor? #Resolved

I just tried and since the other constructor is std::string_view, compiler gives ambiguous overloaded function call unless we update all the callers

I asked about this and found out that this is a reasonable way to do it:

template<typename T, std::enable_if_t<std::is_same_v<T, std::filesystem::path>, int> = 0> MsixInfo(const T& path) { ConstructFromPath(path); }

Where ConstructFromPath can then be implemented in the .cpp.

JohnMcPMS · 2022-05-09T22:59:49Z

src/AppInstallerCommonCore/MsixInfo.cpp

+        MsixInfo msixInfo{ msixPath.u8string() };
+        auto signature = msixInfo.GetSignature();
+        THROW_HR_IF(E_UNEXPECTED, signature.size() <= P7xFileIdSize);
+        signature.erase(signature.begin(), signature.begin() + P7xFileIdSize);


GetSignature(true) to skip the first 4 bytes if they match the file id? erase here is expensive and isn't actually verifying that the bytes match the id. #Resolved

JohnMcPMS · 2022-05-09T23:01:25Z

src/AppInstallerCommonCore/MsixInfo.cpp

+
+    bool ValidateMsixTrustInfo(const std::filesystem::path& msixPath, bool verifyMicrosoftOrigin)
+    {
+        bool result = true;


Suggested change

bool result = true;

bool result = false;

Bugs should not result in returning a 👍 verification. #Resolved

JohnMcPMS · 2022-05-09T23:08:04Z

src/AppInstallerCommonCore/MsixInfo.cpp

+                WINTRUST_DATA trustData = { 0 };
+                trustData.cbStruct = sizeof(WINTRUST_DATA);
+                trustData.dwUIChoice = WTD_UI_NONE;
+                trustData.fdwRevocationChecks = WTD_REVOKE_NONE;


Shouldn't we be doing revocation checks? #Resolved

JohnMcPMS · 2022-05-09T23:35:25Z

src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp


                if (!std::filesystem::exists(packageLocation))
                {
                    AICLI_LOG(Repo, Info, << "Data not found at " << packageLocation);
                    THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_MISSING);
                }

-                SQLiteIndex index = SQLiteIndex::Open(packageLocation.u8string(), SQLiteIndex::OpenDisposition::Read);
+                THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !Msix::ValidateMsixTrustInfo(packageLocation, WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin)));


This appears to be vulnerable to a time of check vs time of use attack; nothing is preventing the package from being replaced between the check here and opening the file later.

You need the TempSQLiteIndexFile (or another helper) that can lock the file from being modified, then do the validation and extraction. #Resolved

JohnMcPMS · 2022-05-09T23:58:24Z

src/AppInstallerRepositoryCore/Microsoft/TempSQLiteIndexFile.cpp

+{
+    namespace
+    {
+        static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv;


This shouldn't be defined in a second location. #Resolved

JohnMcPMS · 2022-05-10T00:14:23Z

src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h

@@ -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 = {});


I don't love this being passed in, but I understand the need. Can we change the type to WriteLockedFile or something more generic? That could be used for both the package and index files (with a create bool that controls both open and remove on destruction functionality).

Then also just move the "extract the index file" part out of the type as it isn't really necessary to be included. #Resolved

github-actions · 2022-05-11T04:31:07Z

@check-spelling-bot Report

Unrecognized words, please review:

exising
WHOLECHAIN

Previously acknowledged words that are now absent

activatable amd Archs dsc enr FWW Globals hackathon lww mytool OSVERSION Packagedx parametermap symlink Uninitialize WDAG whatif wsb

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:yao-msft/winget-cli.git repository
on the loadmsixindex branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1123174846" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

JohnMcPMS · 2022-05-11T21:23:23Z

src/AppInstallerCLITests/MsixInfo.cpp

+{
+    if (!Runtime::IsRunningAsAdmin())
+    {
+        INFO("Test requires admin privilege. Skipped.");


Suggested change

INFO("Test requires admin privilege. Skipped.");

WARN("Test requires admin privilege. Skipped.");

Not sure how this behaves instead, maybe it is more obvious? #Resolved

JohnMcPMS · 2022-05-11T21:35:27Z

src/AppInstallerCommonCore/Public/winget/ManagedFile.h

+        ManagedFile(ManagedFile&&) = default;
+        ManagedFile& operator=(ManagedFile&&) = default;
+
+        HANDLE GetFileHandle() { return m_fileHandle.get(); }


Suggested change

HANDLE GetFileHandle() { return m_fileHandle.get(); }

HANDLE GetFileHandle() const { return m_fileHandle.get(); }

``` #Resolved

JohnMcPMS · 2022-05-11T21:35:37Z

src/AppInstallerCommonCore/Public/winget/ManagedFile.h

+        ManagedFile& operator=(ManagedFile&&) = default;
+
+        HANDLE GetFileHandle() { return m_fileHandle.get(); }
+        const std::filesystem::path& GetFilePath() { return m_filePath; }


Suggested change

const std::filesystem::path& GetFilePath() { return m_filePath; }

const std::filesystem::path& GetFilePath() const { return m_filePath; }

``` #Resolved

JohnMcPMS · 2022-05-11T21:37:53Z

src/AppInstallerCommonCore/MsixInfo.cpp

+                    // 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));


Do we need to set this every time through the loop? #Resolved

JohnMcPMS · 2022-05-11T21:40:05Z

src/AppInstallerCommonCore/MsixInfo.cpp

+                else
+                {
+                    // If given a size, and we have read it all, quit
+                    if (expectedSize && totalBytesRead == expectedSize)


We don't appear to be doing anything to prevent writing more than expectedSize. Is that intentional? #Resolved

JohnMcPMS · 2022-05-11T21:45:22Z

src/AppInstallerCommonCore/MsixInfo.cpp

@@ -335,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)


getRawSignature being true, but the code then modifying the signature, doesn't make sense to me. Maybe this was supposed to be if (!getRawSignature)? Or should the name be changed to skipFileID? #Resolved

JohnMcPMS · 2022-05-11T21:50:22Z

src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp

+                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);


I would either create a new function GetNewTempFilePath or GetPathTo(PathName::NewTempFile), but both would return the same type of path you are constructing here. This is definitely reusable. #Resolved

by looking at GetPathTo() code, it's more of returning a directory caller can work on. I'll create GetNewTempFilePath() then

JohnMcPMS · 2022-05-11T21:57:20Z

src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp

-                    // 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)) &&


You need another

auto indexPackageLock = Utility::ManagedFile::OpenWriteLockedFile(packagePath, 0);

to prevent a TOCTOU here as well (between validation and the IsNewerThan).

It might even be worth making a helper type like:

struct PersistedIndexPackage { PersistedIndexPackage(path); bool ValidateMsixTrustInfo(bool checkMSOrigin); private: ManagedFile m_fileLock; }

Then remove the free function ValidateMsixTrustInfo. That way you must lock the file to validate it, and hopefully that translates into the appropriate lifetime of the lock in the calling code. #Resolved

yao-msft · 2022-05-12T01:52:26Z

/azp run

azure-pipelines · 2022-05-12T01:52:34Z

Azure Pipelines successfully started running 1 pipeline(s).

JohnMcPMS · 2022-05-13T22:29:56Z

src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp

+
+            bool ValidateMsixTrustInfo(bool checkMicrosoftOrigin) const
+            {
+                return Msix::ValidateMsixTrustInfo(m_file.GetFilePath(), checkMicrosoftOrigin);


The reason to move to this model was that the only way to call ValidateMsixTrustInfo would be to create a PersistedIndexPackage. If you leave the free function ValidateMsixTrustInfo around, someone can still use it without the file lock. #Resolved

yao-msft added 8 commits April 28, 2022 13:51

Test

f88107e

Signature validation

0febe83

build

058a44c

build fix

80595d3

fix

6e358da

work

e27543d

tests and fixes

c0ccd59

spelling

c0fe950

yao-msft requested a review from a team as a code owner May 5, 2022 03:19

JohnMcPMS requested changes May 10, 2022

View reviewed changes

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels May 10, 2022

yao-msft added 2 commits May 10, 2022 21:24

Comments

cc99720

Merge remote-tracking branch 'upstream/master' into loadmsixindex

92d66b0

spelling

f788105

JohnMcPMS requested changes May 11, 2022

View reviewed changes

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels May 11, 2022

yao-msft added 2 commits May 11, 2022 18:18

More comments

de5ca81

One more const

afa5499

JohnMcPMS reviewed May 13, 2022

View reviewed changes

Move PersistedMsixFile to common

d5c64d0

JohnMcPMS approved these changes May 16, 2022

View reviewed changes

yao-msft merged commit 0b1142f into microsoft:master May 16, 2022

yao-msft deleted the loadmsixindex branch May 16, 2022 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load index from validated msix for unpackaged context #2139

Load index from validated msix for unpackaged context #2139

yao-msft commented May 5, 2022 •

edited by ghost

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

yao-msft May 11, 2022

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

yao-msft May 11, 2022

JohnMcPMS May 11, 2022

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 10, 2022 •

edited by yao-msft

Loading

github-actions bot commented May 11, 2022 •

edited

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

yao-msft May 11, 2022

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

yao-msft commented May 12, 2022

azure-pipelines bot commented May 12, 2022

JohnMcPMS May 13, 2022 •

edited by yao-msft

Loading

	INFO("Test requires admin privilege. Skipped.");
	WARN("Test requires admin privilege. Skipped.");

	HANDLE GetFileHandle() { return m_fileHandle.get(); }
	HANDLE GetFileHandle() const { return m_fileHandle.get(); }
	``` #Resolved

	const std::filesystem::path& GetFilePath() { return m_filePath; }
	const std::filesystem::path& GetFilePath() const { return m_filePath; }
	``` #Resolved

Load index from validated msix for unpackaged context #2139

Load index from validated msix for unpackaged context #2139

Conversation

yao-msft commented May 5, 2022 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS May 9, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

yao-msft May 11, 2022

Choose a reason for hiding this comment

JohnMcPMS May 9, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

yao-msft May 11, 2022

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022

Choose a reason for hiding this comment

JohnMcPMS May 9, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 9, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 9, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 9, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 9, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 10, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

github-actions bot commented May 11, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

yao-msft May 11, 2022

Choose a reason for hiding this comment

JohnMcPMS May 11, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

yao-msft commented May 12, 2022

azure-pipelines bot commented May 12, 2022

JohnMcPMS May 13, 2022 • edited by yao-msft Loading

Choose a reason for hiding this comment

yao-msft commented May 5, 2022 •

edited by ghost

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 9, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 10, 2022 •

edited by yao-msft

Loading

github-actions bot commented May 11, 2022 •

edited

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 11, 2022 •

edited by yao-msft

Loading

JohnMcPMS May 13, 2022 •

edited by yao-msft

Loading