fix: on Windows use UTF-8 strings instead of system default locale strings #6190

farfella · 2020-01-19T22:48:11Z

The *A version of Windows API functions (e.g., CreateFileA, CreateEventA, etc.) operate on system locale strings and not UTF-8 strings. Furthermore, projects generated with cmake use multi-byte character set. So, CreateFile is defined as CreateFileA vs CreateFileW. This causes weird display and input issues observable by the end-user and reported as bugs.

In this PR, all these cases in the code (only osquery code and not externs) now call the *W versions of the Windows API functions, e.g., CreateFileW, CreateEventW, respectively, and then narrow them to UTF-8 using osquery's wstringToString.

In some cases, we can safely call the *A functions, so these were kept as is (that is, osquery is choosing the input and this input is correct with the *A functions).

Verified all test-cases passed locally.

…rings

osquery/tables/system/windows/ntfs_acl_permissions.cpp

osquery/tables/system/windows/processes.cpp

farfella · 2020-01-20T01:52:09Z

Thanks @Smjert for your help with the Chrome extensions test! :)

Smjert

Thanks again for this effort!

osquery/events/windows/ntfs_event_publisher.cpp

osquery/filesystem/windows/fileops.cpp

osquery/tables/system/windows/ntfs_acl_permissions.cpp

osquery/tables/system/windows/registry.cpp

osquery/utils/system/windows/env.cpp

Smjert

Last review round then I think it's good to go!
Please also remember to format the code :).

osquery/utils/system/windows/env.cpp

farfella · 2020-01-27T00:46:26Z

Last review round then I think it's good to go!
Please also remember to format the code :).

Haha! Thanks. Yeah... I just need to set up style-check validator locally. Relying on IDE works 95% of the time.

Smjert · 2020-01-27T13:03:14Z

Last review round then I think it's good to go!
Please also remember to format the code :).

Haha! Thanks. Yeah... I just need to set up style-check validator locally. Relying on IDE works 95% of the time.

We also have two CMake targets for this: format_check, which checks the code after everything has been committed and format, to actually format the code, but it has to be either in the stage area or unstaged.

merging osquery\osquery master

theopolis · 2020-02-10T04:34:29Z

osquery/utils/system/windows/env.cpp

+
+  return wstringToString(buf.data());
+
+} // namespace osquery


nitpick, this comment looks like the result of a stray copy-paste

Thanks! Updated.

sroache · 2020-02-21T12:43:36Z

I was testing with this branch to see if it would fix some unicode issues encountered and it fixed most of them!
However when I queried the file table for a path that contained Chinese characters I did not get the expected path returned
SELECT * FROM fiile WHERE path LIKE "C:\%\cmd.exe
Where I have copied cmd.exe into C:\Test and C:\新建文件夹 only returned the copy in C:\Test.
(Where "新建文件夹" was the "new folder" text after creating a folder on a Chinese machine)

My current guess is that it has something to do with converting from unicode to UTF-8 before checking if the path exists in WindowsFindFile (fileops.cpp). I think we would also probably need to keep the path as a unicode string when checking the constraint/regex.

farfella · 2020-02-21T12:51:02Z

Thanks for validating @sroache! I will take a look. It should call FindFirstFileW if it isn’t since FindFirstFileA is locale specific— I will check your case.

farfella · 2020-02-24T01:45:10Z

I confirm what @sroache is seeing: I'd prefer to fix this issue in another PR after this is merged in vs adding more changes to this PR.

fix: on Windows use UTF-8 strings instead of system default locale st…

64ab084

…rings

farfella commented Jan 19, 2020

View reviewed changes

osquery/tables/system/windows/ntfs_acl_permissions.cpp Outdated Show resolved Hide resolved

farfella commented Jan 19, 2020

View reviewed changes

osquery/tables/system/windows/processes.cpp Outdated Show resolved Hide resolved

farfella commented Jan 19, 2020

View reviewed changes

osquery/tables/system/windows/processes.cpp Outdated Show resolved Hide resolved

farfella commented Jan 19, 2020

View reviewed changes

osquery/tables/system/windows/processes.cpp Outdated Show resolved Hide resolved

farfella added 4 commits January 19, 2020 19:32

add SQL_TEXT, code style check, and revert EnvVar args to std::string

8fe8326

adding missing updates

9fef4c6

reverting additional files forgotten in prev commit

a9cefda

more missing reverts

2a76576

farfella added 2 commits January 19, 2020 22:22

bad typo: length miscalculation

a2be033

adding successful conversion check to retry build failure

71a1b7d

directionless added this to the 4.2.0 milestone Jan 21, 2020

theopolis previously approved these changes Jan 25, 2020

View reviewed changes

Smjert added the do not merge Do not merge PR as it's pending on some discussion or external factor. Reviewer should have context. label Jan 25, 2020

Smjert requested changes Jan 25, 2020

View reviewed changes

Smjert removed the do not merge Do not merge PR as it's pending on some discussion or external factor. Reviewer should have context. label Jan 25, 2020

adjusting code as per review

f3ad8f0

farfella dismissed theopolis’s stale review via f3ad8f0 January 26, 2020 05:51

farfella added 3 commits January 26, 2020 00:55

removing TODO comment and created ticket

0718ee3

check style

8e1ae05

check style

5c67f6e

Smjert requested changes Jan 26, 2020

View reviewed changes

osquery/utils/system/windows/env.cpp Outdated Show resolved Hide resolved

osquery/utils/system/windows/env.cpp Outdated Show resolved Hide resolved

osquery/utils/system/windows/env.cpp Outdated Show resolved Hide resolved

farfella added 2 commits January 26, 2020 09:35

Updating utils/system/BUCK to add utils/conversions dependencies

dc71c56

updates per PR comments and buck build failure

2b4149e

Smjert previously approved these changes Jan 27, 2020

View reviewed changes

muffins added bug Windows labels Jan 27, 2020

directionless modified the milestones: 4.2.0, 4.2.1 Jan 31, 2020

farfella and others added 2 commits February 5, 2020 18:43

Merge pull request #1 from osquery/master

1835564

merging osquery\osquery master

merging with upstream master

5b710f0

farfella dismissed Smjert’s stale review via 5b710f0 February 6, 2020 00:23

accomodate updated function prototype

f60a049

theopolis reviewed Feb 10, 2020

View reviewed changes

farfella added 2 commits February 10, 2020 07:48

remove stray copy-paste

5093357

formatting: remove extra line

9a5dec8

theopolis approved these changes Feb 10, 2020

View reviewed changes

theopolis merged commit b63dc57 into osquery:master Feb 28, 2020

This was referenced Mar 8, 2020

Fix: broken Unicode filename searches on Windows #6289

Closed

Fix: broken Unicode filename searches on Windows #6291

Merged

farfella mentioned this pull request Mar 23, 2020

select last_run_message from scheduled_tasks incorrect on different language packs #6304

Closed

polak785 mentioned this pull request Apr 27, 2020

error while marshaling: proto: field \"kolide.agent.LogCollection.Log.Data\" contains invalid UTF-8 kolide/launcher#445

Closed

farfella deleted the utf8_instead_of_system_locale branch August 16, 2020 23:30

Smjert mentioned this pull request Oct 25, 2020

osquery appears to ignore files with names that contain multibyte characters over U+00FF on Windows #4150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: on Windows use UTF-8 strings instead of system default locale strings #6190

fix: on Windows use UTF-8 strings instead of system default locale strings #6190

farfella commented Jan 19, 2020

farfella commented Jan 20, 2020

Smjert left a comment

Smjert left a comment •

edited

Loading

farfella commented Jan 27, 2020

Smjert commented Jan 27, 2020 •

edited

Loading

theopolis Feb 10, 2020

farfella Feb 10, 2020

sroache commented Feb 21, 2020

farfella commented Feb 21, 2020

farfella commented Feb 24, 2020

fix: on Windows use UTF-8 strings instead of system default locale strings #6190

fix: on Windows use UTF-8 strings instead of system default locale strings #6190

Conversation

farfella commented Jan 19, 2020

farfella commented Jan 20, 2020

Smjert left a comment

Choose a reason for hiding this comment

Smjert left a comment • edited Loading

Choose a reason for hiding this comment

farfella commented Jan 27, 2020

Smjert commented Jan 27, 2020 • edited Loading

theopolis Feb 10, 2020

Choose a reason for hiding this comment

farfella Feb 10, 2020

Choose a reason for hiding this comment

sroache commented Feb 21, 2020

farfella commented Feb 21, 2020

farfella commented Feb 24, 2020

Smjert left a comment •

edited

Loading

Smjert commented Jan 27, 2020 •

edited

Loading