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.

Sign up for GitHub

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

Jump to bottom

Use NtCreateFile instead of NtOpenFile to open a file #93206

Merged
merged 1 commit into from
Feb 9, 2022
Merged

Use NtCreateFile instead of NtOpenFile to open a file #93206

merged 1 commit into from
Feb 9, 2022

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Jan 22, 2022 •
edited
Loading

Generally the internal Nt* functions should be avoided but when we do need to use one we should stick to the most commonly used for the job. To that end, this PR replaces NtOpenFile with NtCreateFile.

NOTE: The initial version of this comment hypothesised that this may help with some recent false positives from malware scanners. This hypothesis proved wrong. Sorry for the distraction.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2022
@the8472
Copy link
Member

the8472 commented Jan 22, 2022

I've no idea if this actually helps

Maybe do a stage1 build and upload the file in question to virustotal?

@ChrisDenton
Copy link
Member Author

Maybe do a stage1 build and upload the file in question to virustotal?

The results. 11/64 down from 15/63. Yay?

@nagisa
Copy link
Member

nagisa commented Jan 22, 2022 •
edited
Loading

Note to reviewers: NtCreateFile is declared in winternl.h and is an “internal” function, which NtOpenFile isn’t.

Here’s what MSDN has to say about internal APIs:

The functions and structures in Winternl.h are internal to the operating system and subject to change from one release of Windows to the next, and possibly even between service packs for each release. To maintain the compatibility of your application, you should use the equivalent public functions instead. Further information is available in the header file, Winternl.h, and the documentation for each function.

I think that's a pretty good reason to consider not merging this.

EDIT: huh, it seems there's NtCreateFile in ntifs.h too 😕

@ChrisDenton
Copy link
Member Author

@nagisa This is replacing use of NtOpenFile which is also an internal API. The reason for using one of these APIs is that there are no Win32 alternatives for opening a file relative to a directory handle and doing so is necessary to address CVE-2022-21658.

@nagisa
Copy link
Member

nagisa commented Jan 22, 2022 •
edited
Loading

Right, searching for NtOpenFile led me to NtOpenFile (ntifs.h) first which is not as apparently documented as internal, if at all. The first hit for NtCreateFile led me to NtCreateFile (winternl.h) which is very apparently internal.

Turns out both functions exist in either header; ntifs.h is specific to the drivers, though and is not relevant to us.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Jan 22, 2022 •
edited
Loading

Oh yeah, that is confusing. The winternl functions are documented as internal because they're located in the user space section. Whereas the other docs are in the driver section where everything should be considered internal by default.

I guess the docs don't really account for people arriving via a web search.

@klensy
Copy link
Contributor

klensy commented Jan 23, 2022

Maybe do a stage1 build and upload the file in question to virustotal?

The results. 11/64 down from 15/63. Yay?

Pressed reanalyze, and now 15/65.

@ChrisDenton
Copy link
Member Author

Oh hm, yeah not helpful after all then!

So at this point my only motivation for wanting this merged is just that if we must use an internal API, it should ideally be the one documented in winternl.

@the8472
Copy link
Member

the8472 commented Jan 23, 2022

there are no Win32 alternatives for opening a file relative to a directory handle and doing so is necessary to address CVE-2022-21658.

How is this handled in other language libraries? Are they all vulnerable to that CVE? Or do they use these functions too? If the latter then we can at least cite prior art.

@klensy
Copy link
Contributor

klensy commented Jan 23, 2022

Pressed reanalyze, and now 15/65.

17/65

AV soft: Wow, other 10 boys detect something, we should too!

@ChrisDenton
Copy link
Member Author

How is this handled in other language libraries? Are they all vulnerable to that CVE? Or do they use these functions too? If the latter then we can at least cite prior art.

C# does use NtCreateFile for directory traversal.

@the8472
Copy link
Member

the8472 commented Jan 23, 2022

That's managed by microsoft, they'd have an easier time to coordinate internal API breaks.

@ChrisDenton
Copy link
Member Author

That's managed by microsoft, they'd have an easier time to coordinate internal API breaks.

Sure. But this PR is only arguing that NtCreateFile is a better choice than NtOpenFile, nothing more. And there's not a lot of precedence I can find for addressing this CVE in other standard libraries.

@nagisa
Copy link
Member

nagisa commented Jan 23, 2022

So at this point my only motivation for wanting this merged is just that if we must use an internal API, it should ideally be the one documented in winternl.

Actually yeah. I tend to agree there. Could you please adjust the PR/commit title to specifically call out the function name as well? e.g.

Use NtCreateFile instead of NtOpenFile[…]

r=me otherwise

@ChrisDenton ChrisDenton changed the title Use create instead of open to open a file Use NtCreateFile instead of NtOpenFile to open a file Jan 23, 2022
@ChrisDenton
Copy link
Member Author

Done!

@LegionMammal978
Copy link
Contributor

Shouldn't the panic message be changed to "`NtCreateFile` not available"?

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Jan 24, 2022 •
edited
Loading

Whoops, missed that. Thanks!

@ChrisDenton
Copy link
Member Author

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned yaahc Feb 7, 2022
@nagisa
Copy link
Member

nagisa commented Feb 7, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 7, 2022

📌 Commit ac02fcc has been approved by nagisa

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 7, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 8, 2022
@matthiaskrgr
Rollup merge of rust-lang#93206 - ChrisDenton:ntopenfile, r=nagisa
0fce680 
Use `NtCreateFile` instead of `NtOpenFile` to open a file

Generally the internal `Nt*` functions should be avoided but when we do need to use one we should stick to the most commonly used for the job. To that end, this PR replaces `NtOpenFile` with `NtCreateFile`.

NOTE: The initial version of this comment hypothesised that this may help with some recent false positives from malware scanners. This hypothesis proved wrong. Sorry for the distraction.
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 8, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 8, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2022
@bors
Auto merge of rust-lang#93778 - matthiaskrgr:rollup-yfngdao, r=matthi…
9a5a961 
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91950 (Point at type when a `static` `#[global_allocator]` doesn't `impl` `GlobalAlloc`)
 - rust-lang#92715 (Do not suggest char literal for zero-length strings)
 - rust-lang#92917 (Don't constrain projection predicates with inference vars in GAT substs)
 - rust-lang#93206 (Use `NtCreateFile` instead of `NtOpenFile` to open a file)
 - rust-lang#93732 (add fut/back compat tests for implied trait bounds)
 - rust-lang#93764 (:arrow_up: rust-analyzer)
 - rust-lang#93767 (deduplicate `lcnr` in mailmap)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9cb39a6 into rust-lang:master Feb 9, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 9, 2022
@ChrisDenton ChrisDenton deleted the ntopenfile branch April 28, 2022 20:28
@sunfishcode sunfishcode mentioned this pull request Aug 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nagisa nagisa

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
9 participants
@ChrisDenton @rust-highfive @the8472 @nagisa @klensy @LegionMammal978 @bors @yaahc @rustbot