[remoting][win] Harden remote WebAuthn NMH trust verification [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 3 comments![]( "Open in Gerrit")
if (base::PathService::Get(base::DIR_SYSTEM, &system_dir)) {If Chrome is 64-bit but this native messaging host is 32-bit, `base::PathService::Get(base::DIR_SYSTEM, &system_dir)` will return `C:\\Windows\\SysWOW64` due to WOW64 redirection. However, the 64-bit Chrome launches `C:\\Windows\\System32\\cmd.exe`. This will cause the path comparison below to fail and reject the legitimate caller.
To avoid this bitness mismatch issue, you can check if `parent_image_path.BaseName().value() == L"cmd.exe"`. This is still secure because we skip to the grandparent and check if the grandparent is an allowed caller program signed by Google.
std::wstring subject_name;You can simplify this string allocation and avoid the manual null-terminator search by using `base::WriteInto` (from `"base/strings/string_util.h"`). `base::WriteInto` reserves `length` elements and correctly sets the size of the string to `length - 1`, expecting the Windows API to write the trailing null.
```cpp
std::wstring subject_name;
CertGetNameStringW(cert_context, CERT_NAME_SIMPLE_DISPLAY_TYPE, /*dwFlags=*/0,
/*pvTypePara=*/nullptr,
base::WriteInto(&subject_name, length), length);
```
wintrust_data.dwStateAction = WTD_STATEACTION_CLOSE;According to the [Microsoft documentation](https://learn.microsoft.com/en-us/windows/win32/api/wintrust/nf-wintrust-winverifytrust):
> If the WinVerifyTrust function returns a failure code, you should not call the WinVerifyTrust function again with the dwStateAction member set to WTD_STATEACTION_CLOSE.
Since you're calling it unconditionally, you should move this close operation inside the `if (trust_status == ERROR_SUCCESS)` block. (The original code had a memory leak by not calling this at all on success, so good catch on fixing that!).
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
3 comments
if (base::PathService::Get(base::DIR_SYSTEM, &system_dir)) {If Chrome is 64-bit but this native messaging host is 32-bit, `base::PathService::Get(base::DIR_SYSTEM, &system_dir)` will return `C:\\Windows\\SysWOW64` due to WOW64 redirection. However, the 64-bit Chrome launches `C:\\Windows\\System32\\cmd.exe`. This will cause the path comparison below to fail and reject the legitimate caller.
To avoid this bitness mismatch issue, you can check if `parent_image_path.BaseName().value() == L"cmd.exe"`. This is still secure because we skip to the grandparent and check if the grandparent is an allowed caller program signed by Google.
Done
You can simplify this string allocation and avoid the manual null-terminator search by using `base::WriteInto` (from `"base/strings/string_util.h"`). `base::WriteInto` reserves `length` elements and correctly sets the size of the string to `length - 1`, expecting the Windows API to write the trailing null.
```cpp
std::wstring subject_name;
CertGetNameStringW(cert_context, CERT_NAME_SIMPLE_DISPLAY_TYPE, /*dwFlags=*/0,
/*pvTypePara=*/nullptr,
base::WriteInto(&subject_name, length), length);
```
Done
According to the [Microsoft documentation](https://learn.microsoft.com/en-us/windows/win32/api/wintrust/nf-wintrust-winverifytrust):
> If the WinVerifyTrust function returns a failure code, you should not call the WinVerifyTrust function again with the dwStateAction member set to WTD_STATEACTION_CLOSE.Since you're calling it unconditionally, you should move this close operation inside the `if (trust_status == ERROR_SUCCESS)` block. (The original code had a memory leak by not calling this at all on success, so good catch on fixing that!).
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
1 comment
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted![]( "Open in Gerrit")
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[remoting][win] Harden remote WebAuthn NMH trust verification
* Use PathService instead of environment variables to query paths.
* Update IsBinaryTrusted to verify that a binary is signed by Google.
Verified: Tweaked code locally and verified that the logic works
properly for verifying the NMH caller (Chrome). Saw the "Signature
verified and publisher pinned for" log. Could not verify mojo caller
trust because the caller (e.g. desktop process) is unsigned, but it
should work as expected. I'll try ToT official build once CL is
submitted.
- M remoting/host/webauthn/remote_webauthn_caller_security_utils.cc
- M remoting/host/win/trust_util.cc
Code-Review: +1 by Joe Downing
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |