Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent null characters from getting into SQLite #2289

Merged
merged 5 commits into from
Jul 5, 2022

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Jun 29, 2022

Fixes #2273

Issue

The root of #2273 and some previous crashes in the same area is that SQLite does not handle null characters in strings in an intuitive way. Specifically, you can insert a string with null characters, but selecting that value out as a string results in a value that is terminates at the first null character. However, equality comparisons against that string value in the database require the full value, including nulls, to be true.

All of this lead to the following:

  1. Embedded null characters in the DisplayVersion of an ARP entry were read in
  2. This value was inserted into the temporary database holding the system information
  3. On selecting the "latest version" from the database, we select all version strings
  4. With the highest version value, we select the row id back
    a. Here is where the bug strikes, the selected version string will not equal the value in the database due to the null character
  5. The code rolls up to having no installed version despite explicitly being the installed database

The previous mitigation prevented the crash by simply dropping that package from list, but this change allows those entries to show again.

Change

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.

Validation

Added unit tests covering the changed behaviors.
Manually inserted a null character into a DisplayVersion registry entry on my machine:

PS > winget list test
No installed package found matching input criteria.
PS > wingetdev list test
Name                         Id                                     Version
------------------------------------------------------------------------------
AppInstallerTestExeInstaller {A499DD5E-8DC5-4AD2-911A-BCD0263295E9} 1.0.0.0  1
Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner June 29, 2022 22:42
@ghost ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jun 29, 2022
CompositeTestSetup setup{ CompositeSearchBehavior::AvailablePackages };
setup.Available->Everything.Matches.emplace_back(MakeInstalled(), Criteria());

// We are mostly testing to see if a null installed version causes an AV or not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

installed

nit: available

@@ -102,6 +102,7 @@
#define APPINSTALLER_CLI_ERROR_PORTABLE_UNINSTALL_FAILED ((HRESULT)0x8A150057)
#define APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED ((HRESULT)0x8A150058)
#define APPINSTALLER_CLI_ERROR_UNSUPPORTED_ARGUMENT ((HRESULT)0x8A150059)
#define APPINSTALLER_CLI_ERROR_BIND_WITH_EMBEDDED_NULL ((HRESULT)0x8A150060)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0x8A150060

0x8A15005A

Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@JohnMcPMS JohnMcPMS merged commit de4aa01 into microsoft:master Jul 5, 2022
@JohnMcPMS JohnMcPMS deleted the no-null-chars branch July 5, 2022 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in list command
2 participants