Skip to content

Commit

Permalink
Prevent null characters from getting into SQLite (#2289)
Browse files Browse the repository at this point in the history 
In order to prevent null characters from entering SQLite at all, the string binding functions are updated to throw if an embedded null is present.  Additionally, the consistency check is updated to detect strings with embedded null characters.

In order to prevent attempting to send null characters into the database at all, the `NormalizedString` type used throughout the manifest is updated to also replace any null characters with spaces.  This will allow those packages to be listed now.
  • Loading branch information
@JohnMcPMS
JohnMcPMS authored Jul 5, 2022
1 parent b642938 commit de4aa01
Show file tree
Hide file tree
Showing 21 changed files with 1,145 additions and 939 deletions.
191 changes: 100 additions & 91 deletions src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,16 @@
</Filter>
<Filter Include="TestData\MultiFileManifestV1_3">
<UniqueIdentifier>{52b26063-ccff-40ae-a420-243327dcca25}</UniqueIdentifier>
</Filter>
</Filter>
<Filter Include="Source Files\CLI">
<UniqueIdentifier>{b35e1a8b-1961-46d5-98e5-adc8e52c54a9}</UniqueIdentifier>
</Filter>
<Filter Include="Source Files\Common">
<UniqueIdentifier>{d055e02a-59c9-4ae4-9405-579dac6ff59b}</UniqueIdentifier>
</Filter>
<Filter Include="Source Files\Repository">
<UniqueIdentifier>{13d4d227-0f04-4e57-a663-c3c535438ab3}</UniqueIdentifier>
</Filter>
</ItemGroup>
<ItemGroup>
<ClInclude Include="pch.h">
Expand Down Expand Up @@ -59,146 +68,146 @@
<ClCompile Include="main.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="SQLiteWrapper.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="SQLiteIndex.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="TestCommon.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="YamlManifest.cpp">
<ClCompile Include="TestSource.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="Downloader.cpp">
<ClCompile Include="TestSettings.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="LanguageUtilities.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="ARPChanges.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="Settings.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Command.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="Sources.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Completion.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="WorkFlow.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="CompositeSource.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="Synchronization.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Correlation.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="MsixInfo.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="CustomHeader.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="PreIndexedPackageSource.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Dependencies.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="SQLiteIndexSource.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Downloader.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="HashCommand.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="ExperimentalFeature.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="Versions.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="GroupPolicy.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="Strings.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="HashCommand.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="Command.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="HttpClientHelper.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="UserSettings.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="InstallerMetadataCollectionContext.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="ExperimentalFeature.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="JsonHelper.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="Completion.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="LanguageUtilities.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="PredefinedInstalledSource.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="ManifestComparator.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="CompositeSource.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="MsiExecArguments.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="TestSource.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="MsixInfo.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="Registry.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="MsixManifest.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="NameNormalization.cpp">
<Filter>Source Files</Filter>
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="PackageCollection.cpp">
<Filter>Source Files</Filter>
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="Regex.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="PackageTrackingCatalog.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="ARPChanges.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="PredefinedInstalledSource.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="GroupPolicy.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="PreIndexedPackageSource.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="WorkflowGroupPolicy.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Regex.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="TestSettings.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Registry.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="ManifestComparator.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="RestClient.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="RestHelper.cpp">
<Filter>Source Files</Filter>
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="JsonHelper.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="RestInterface_1_0.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="RestClient.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="RestInterface_1_1.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="RestInterface_1_0.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="SearchRequestSerializer.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="TestRestRequestHandler.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Settings.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="HttpClientHelper.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Sources.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="SearchRequestSerializer.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="SQLiteIndex.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="CustomHeader.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="SQLiteIndexSource.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="MsiExecArguments.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="SQLiteWrapper.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
<ClCompile Include="RestInterface_1_1.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Strings.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="Dependencies.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Synchronization.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="PackageTrackingCatalog.cpp">
<ClCompile Include="TestRestRequestHandler.cpp">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="Correlation.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="UserSettings.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="InstallerMetadataCollectionContext.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="Versions.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
<ClCompile Include="MsixManifest.cpp">
<Filter>Source Files</Filter>
<ClCompile Include="WorkFlow.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="WorkflowGroupPolicy.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
<ClCompile Include="YamlManifest.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
Expand Down Expand Up @@ -479,7 +488,7 @@
</CopyFileToFolders>
<CopyFileToFolders Include="TestData\InstallFlowTest_UnsupportedArguments.yaml">
<Filter>TestData</Filter>
</CopyFileToFolders>
</CopyFileToFolders>
<CopyFileToFolders Include="TestData\InstallerArgTest_Msi_WithSwitches.yaml">
<Filter>TestData</Filter>
</CopyFileToFolders>
Expand All @@ -506,7 +515,7 @@
</CopyFileToFolders>
<CopyFileToFolders Include="TestData\UpdateFlowTest_Exe_UnsupportedArgs.yaml">
<Filter>TestData</Filter>
</CopyFileToFolders>
</CopyFileToFolders>
<CopyFileToFolders Include="TestData\UpdateFlowTest_ExeDependencies.yaml">
<Filter>TestData</Filter>
</CopyFileToFolders>
Expand Down
24 changes: 22 additions & 2 deletions src/AppInstallerCLITests/CompositeSource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@ struct ComponentTestSource : public TestSource
// A helper to create the sources used by the majority of tests in this file.
struct CompositeTestSetup
{
CompositeTestSetup() : Composite("*Tests")
CompositeTestSetup(CompositeSearchBehavior behavior = CompositeSearchBehavior::Installed) : Composite("*Tests")
{
Installed = std::make_shared<ComponentTestSource>("InstalledTestSource1");
Available = std::make_shared<ComponentTestSource>("AvailableTestSource1");
Composite.SetInstalledSource(Source{ Installed });
Composite.SetInstalledSource(Source{ Installed }, behavior);
Composite.AddAvailableSource(Source{ Available });
}

Expand Down Expand Up @@ -942,3 +942,23 @@ TEST_CASE("CompositeSource_TrackingFound_NotInstalled", "[CompositeSource]")

REQUIRE(result.Matches.empty());
}

TEST_CASE("CompositeSource_NullInstalledVersion", "[CompositeSource]")
{
CompositeTestSetup setup;
setup.Installed->Everything.Matches.emplace_back(MakeAvailable(), Criteria());

// We are mostly testing to see if a null installed version causes an AV or not
SearchResult result = setup.Search();
REQUIRE(result.Matches.size() == 0);
}

TEST_CASE("CompositeSource_NullAvailableVersion", "[CompositeSource]")
{
CompositeTestSetup setup{ CompositeSearchBehavior::AvailablePackages };
setup.Available->Everything.Matches.emplace_back(MakeInstalled(), Criteria());

// We are mostly testing to see if a null available version causes an AV or not
SearchResult result = setup.Search();
REQUIRE(result.Matches.size() == 1);
}
29 changes: 28 additions & 1 deletion src/AppInstallerCLITests/SQLiteIndex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3153,4 +3153,31 @@ TEST_CASE("SQLiteIndex_ManifestArpVersion_ValidateManifestAgainstIndex", "[sqlit
// Add different version should result in failure.
manifest.Version = "10.1";
REQUIRE_THROWS(ValidateManifestArpVersion(&index, manifest));
}
}

TEST_CASE("SQLiteIndex_CheckConsistency_FindEmbeddedNull", "[sqliteindex]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());

SQLiteIndex index = CreateTestIndex(tempFile, Schema::Version::Latest());

Manifest manifest;
manifest.Id = "Foo";
manifest.Version = "10.0";
manifest.Installers.push_back({});
manifest.Installers[0].InstallerType = InstallerTypeEnum::Exe;
manifest.Installers[0].AppsAndFeaturesEntries.push_back({});
manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayVersion = "1.0";
manifest.Installers[0].AppsAndFeaturesEntries.push_back({});
manifest.Installers[0].AppsAndFeaturesEntries[1].DisplayVersion = "1.1";

index.AddManifest(manifest, "path");

// Inject a null character using SQL without binding since we block it
Connection connection = Connection::Create(tempFile, Connection::OpenDisposition::ReadWrite);
Statement update = Statement::Create(connection, "Update versions set version = '10.0'||char(0)||'After Null' where version = '10.0'");
update.Execute();

REQUIRE(!index.CheckConsistency(true));
}
14 changes: 14 additions & 0 deletions src/AppInstallerCLITests/SQLiteWrapper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.
#include "pch.h"
#include "TestCommon.h"
#include <AppInstallerErrors.h>
#include <SQLiteWrapper.h>
#include <SQLiteStatementBuilder.h>

Expand Down Expand Up @@ -286,6 +287,19 @@ TEST_CASE("SQLiteWrapper_EscapeStringForLike", "[sqlitewrapper]")
REQUIRE(expected == output);
}

TEST_CASE("SQLiteWrapper_BindWithEmbeddedNull", "[sqlitewrapper]")
{
Connection connection = Connection::Create(SQLITE_MEMORY_DB_CONNECTION_TARGET, Connection::OpenDisposition::Create);

CreateSimpleTestTable(connection);

int firstVal = 1;
std::string secondVal = "test";
secondVal[1] = '\0';

REQUIRE_THROWS_HR(InsertIntoSimpleTestTable(connection, firstVal, secondVal), APPINSTALLER_CLI_ERROR_BIND_WITH_EMBEDDED_NULL);
}

TEST_CASE("SQLBuilder_SimpleSelectBind", "[sqlbuilder]")
{
Connection connection = Connection::Create(SQLITE_MEMORY_DB_CONNECTION_TARGET, Connection::OpenDisposition::Create);
Expand Down
14 changes: 13 additions & 1 deletion src/AppInstallerCLITests/Strings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ TEST_CASE("NormalizedString", "[strings]")
// Ligature fi => f + i
std::string_view input2 = u8"\xFB01";
REQUIRE(NormalizedString(input2) == u8"fi");

// Embedded null
std::string_view input3{ "Test\0Case", 9 };
REQUIRE(NormalizedString(input3) == "Test Case");
}

TEST_CASE("Trim", "[strings]")
Expand Down Expand Up @@ -203,4 +207,12 @@ TEST_CASE("SplitIntoWords", "[strings]")
// 私のテスト = "My test" according to an online translator
// Split as "私" "の" "テスト"
REQUIRE(SplitIntoWords("\xe7\xa7\x81\xe3\x81\xae\xe3\x83\x86\xe3\x82\xb9\xe3\x83\x88") == std::vector<std::string>{ "\xe7\xa7\x81", "\xe3\x81\xae", "\xe3\x83\x86\xe3\x82\xb9\xe3\x83\x88" });
}
}

TEST_CASE("ReplaceEmbeddedNullCharacters", "[strings]")
{
std::string test = "Test Parts";
test[4] = '\0';
ReplaceEmbeddedNullCharacters(test);
REQUIRE(test == "Test Parts");
}
Loading

0 comments on commit de4aa01

Please sign in to comment.