Fix backtraces on Windows/GNU

[petrochenkov]

Query name of the current executable each time we are going to read debuginfo from it.
See #33985 (comment), other messages in #33985, and #21889 for more details.

Fixes #33985
r? @alexcrichton

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

It looks like this patch to libbacktrace hasn't made its way upstream yet, perhaps we could continue to just pass it manually?

[retep998]

Can't you just call GetCurrentProcess?

[petrochenkov]

No, I tried it first actually and it didn't work. AFAIU, GetCurrentProcess() returns a pseudo handle -1 and QueryFullProcessImageName needs a real non--1 process handle.

[retep998]

Ah okay. Yay for inconsistent process functions!

[retep998]

This is super unfortunate.
Yet another reason to avoid MinGW and friends on Windows.

[petrochenkov]

@alexcrichton

It looks like this patch to libbacktrace hasn't made its way upstream yet, perhaps we could continue to just pass it manually?

I don't plan to submit this patch upstream (it's not in general libbacktrace style and probably needs to be completely reworked before upstreaming), so it'll need to be reapplied on every libbacktrace update (not a big deal).
I'll try to open an issue on their tracker and ask how to better proceed with upstreaming.

[alexcrichton]

@petrochenkov hm I little confused. We didn't need a patch before, so how come we need a patch now? Why not just specify the binary via the standard means we did before?

[petrochenkov]

@alexcrichton

We didn't need a patch before, so how come we need a patch now? Why not just specify the binary via the standard means we did before?

We specified the binary via the standard means and didn't need this patch before May 14, but then #33554 was merged to address security concerns described in #21889 and backtraces were broken.
Now we either need to apply this patch or revert #33554. A bit of background and alternatives are described in #33985 (comment).

[alexcrichton]

I think I'm still not following. #21889 was an observation that if you hand arbitrary input to libbacktrace it's got a high chance of segfaulting/crashing. This means that if you can control the input passed to libbacktrace you may be able to take control of arbitrary executables. (emphasis on may)

This, when combined with us passing env::current_exe() meant that you could control what input was passed to libbacktrace via hard links on unix and such. That vector was addressed in #33554 which removed us calling current_exe. All of this was also taken in the context that libbacktrace's default behavior on Linux (/proc/self/...) is not affected by those problems. Access through those paths are guaranteed to be the same file you were executed with.

So, my takeaway from that is:

• We have a method of giving the current file name to libbacktrace without modifying libbacktrace
• We conservatively stopped doing this everywhere, including Windows.
• It's now believed that Windows is not affected by the same "vulnerability". In other words this method of learning the current path name is immune to allowing someone to provide arbitrary input to libbacktrace.

So given all that, why are we modifying libbacktrace? Why are we not just changing the behavior so only on Windows we do the same thing here but externally?

I'm harping on this because we're inevitably going to update libbacktrace in the future, which will then inevitably overwrite these changes if they're not upstream.

[petrochenkov]

@alexcrichton

We have a method of giving the current file name to libbacktrace without modifying libbacktrace

Without modifying libbacktrace we have a method of giving the file name at moment of time t.
libbacktrace will read this file at moment of time t + delta when it may be invalidated.

It's now believed that Windows is not affected by the same "vulnerability".

Windows is indeed affected by this hypothetical vulnerability - current_exe-like functions on Windows return a concrete filename, like C:\my_program.exe and not some kind of meta-name like /proc/self/exe that can't be invalidated.

(Personally, I'm not especially concerned by this "vulnerability" and initially proposed to simply revert #33554 on Windows, but there was some opposition.)

[alexcrichton]

Without modifying libbacktrace we have a method of giving the file name at moment of time t.
libbacktrace will read this file at moment of time t + delta when it may be invalidated.

How does modifying libbacktrace fix this problem?

Windows is indeed affected by this hypothetical vulnerability

This... sounds bad?

[petrochenkov]

How does modifying libbacktrace fix this problem?

By making executable filename retrieval happen immediately before reading the file.

[alexcrichton]

I.. don't think that fixes the bug?

The bug at least as I am aware of it on Unix is:

• First, you start a process
• Next, you overwrite that binary with arbitrary data
• Process panics
• Process reads arbitrary data, attempting to interpret it as a backtrace

Regardless if where in the process we read the path name, you're vulnerable no matter what if the path being looked at is in any way overwritable.

[petrochenkov]

@alexcrichton
Well, then #33554 achieved nothing on Linux, because libbacktrace still reads the file found under /proc/self/exe even if it's overwritten.

On Windows files being executed are locked and can't be modified, so the situation is better.
(Of course, this can be broken too given enough effort.)

[alexcrichton]

From what I'm led to believe /proc/self/exe can't be overwritten. It's process-local and refers to the exact file that was used to start a process. So in that sense Linux is fine.

If Windows files can't be modified while they're being executed then this sounds plausible, but also means that there's no race, which means there's no need to modify libbacktrace.

[petrochenkov]

If Windows files can't be modified while they're being executed then this sounds plausible, but also means that there's no race, which means there's no need to modify libbacktrace.

They can't be modified, but they can be renamed :)
And the old name can be used for the malicious file.

To clarify, my goal is to fix backtraces, not to build an impenetrable defense.
If you think simply providing some executable path to backtrace_create_state is enough, then I'll do that instead.

[alexcrichton]

Ok, so taking a step back here, I feel like we're talking past each other. Here's how I see the current state of play:

• The libbacktrace library is embedded in almost all Rust programs. It's a large wad of code written in C and not necessarily thoroughly audited for correctness. We've seen segfaults in the past which we've had to fix.
• The libs team has chosen to pessimistically assume that arbitrary input fed to libbacktrace will cause arbitrary code execution.
• Arbitrary input is primarily fed to libbacktrace through changing file contents between when the current executable's path was found and when it was read. Linux is the only platform impervious to this as this race cannot happen in /proc

So, given that, it is impossible to not expose this problem on Windows. There is no path on Windows which represents the current executable impervious to these race conditions. And finally, given that, the current stance of the libs team is that if we pass file names to libbacktrace we're opening up the standard library to a possible arbitrary code execution exploit on Windows.

In short, my current understanding leads me to the conclusion that this fix is not possible to do while preventing these theoretical problems with libbacktrace.

So that brings us to the problem of backtraces. Backtraces are currently broken on MinGW (#33985) presumably because libbacktrace can't find all the dwarf debug info. This PR is fixing that.

And finally, putting all that together, my conclusion would be that the MinGW backtrace issue is WONTFIX if this is the only solution. Other possible vectors include:

• Figure out another method of giving libbacktrace dwarf data
• Figure out another method of backtraces on MinGW
• Lift the restriction of passing the filename to libbacktrace on Windows

The first two options are hard, the latter would require more than me from the libs team to weigh in. I would personally be against it.

[petrochenkov]

So, given that, it is impossible to not expose this problem on Windows. There is no path on Windows which represents the current executable impervious to these race conditions.

This is wrong. There's no race condition on Windows if the file id (name, descriptor, handle) is retrieved simultaneously with reading from that file.
This PR does close approximation to this - the window between retrieving file id and reading from it is reduced to an absolute minimum - the attack can't be performed unless the process can be stopped at few specific instructions. It can be completely closed, but it will require a bit more invasive refactoring in libbacktrace - we'll need to get the file handle without going through filename.

EDIT: Moved discussion of alternatives to #33985 to avoid derailing the thread.

[petrochenkov]

To add another perspective, if the attacker has access to filesystem, then what prevents him from writing specially crafted DWARF right into our executable (before or after it's launched)?
The more I think about it, the more I'm convinced that all this discussion doesn't make any sense until someone with actual security expertise puts it into practical context.
(Regardless, I want backtraces working.)

[retep998]

There's no race condition on Windows if the file id (name, descriptor, handle) is retrieved simultaneously with reading from that file.

There is still totally a race condition, it is just a much narrower time window, which makes exploitation harder.

I am of the opinion that if the attacker has control over the the executable file, then you've already lost. I can't think of any scenarios where an attacker would only have control over the executable file after it was executed.

[arielb1]

@retep998

For example, if the executable is run from some "cache" dir, and there is some process that renames files after they are run. That would be a vulnerability in all Rust applications, so exploitation opportunities abound.

[petrochenkov]

@retep998

There is still totally a race condition, it is just a much narrower time window

This is exactly what I wrote in the next paragraph. I think this is good enough, but the race can be completely removed if we circumvent file names and work only with handles (I haven't investigated how to do this yet).
Anyway, I'm not sure we are not trying to board up the windows while the doors are wide open.

[retep998]

I'm not aware of any functions that allow retrieving a handle to the process file directly (I can't even find the handle in the PEB!). As far as I can tell, the only option to enable libbacktrace to actually work without allowing this vulnerability is to fix all the bugs in libbacktrace so that it doesn't have any vulnerabilities which could be exploited by this vulnerability.

@arielb1 Would it be unrealistic to expect everyone to respect a big scary warning that says to not do that?

[alexcrichton]

This PR does close approximation to this - the window between retrieving file id and reading from it is reduced to an absolute minimum

As @retep998, a "small race condition" is still just that, a race condition. Can't work around that fact.

we'll need to get the file handle without going through filename

Spot on! This would indeed solve the problem, and is essentially how the solution on Linux works.

To add another perspective, if the attacker has access to filesystem ...

I don't think it's really too productive to talk about attacks here. The current stance of the libs team is that this race is unacceptable. Reversing that decision shouldn't happen as part of this thread and should likely happen with discussion on an issue.

In light of that the purpose of the PR here would be to re-enable nice backtraces on MinGW while also preserving the same guarantees we had before, that is never giving possibly arbitrary information to libbacktrace. The PR as-is does not do this (it still has the same race, as I mentioned before).

[petrochenkov]

@alexcrichton

To add another perspective, if the attacker has access to filesystem ...

I don't think it's really too productive to talk about attacks here.
the same guarantees we had before, that is never giving possibly arbitrary information to libbacktrace.

The point of that message is that we are giving arbitrary information to libbacktrace right now on all platforms, if this arbitrary information is written into debuginfo section of the original executable file.

So, removal of the race (which I'm still going to pursue) still doesn't give any guarantees, only makes it a bit harder to come up with an exploit.

[arielb1]

@petrochenkov

The case I am worried about is:

1. Daemon has a cache directory that contains files (or directories!) from various sources under source-determined names.
2. Some of the files in that directory are trusted (e.g. they came from a trustworthy source). The program can run trusted executables.
3. The daemon can be made to rename files/directories under its control, clearing the "trusted" status (there is no rename -> execute race, because the program is single-threaded/uses a lock/whatever).

In that case, an attacker can cause a trusted Rust program to be run, rename it, download an untrusted file with the same name that exploits libbacktrace, and cause the trusted program to panic.

This is a bit obscure, but it is a potential vulnerability every time you run a Rust program - the "daemon" does not even have to be written in Rust. That's not something we want.

[alexcrichton]

@petrochenkov the exploit on linux was this:

• There's a setuid binary that when executed by anyone will run as root (it's trusted)
• You then hard link that setuid binary to a user-owned location
• When run, env::current_exe will return this user-owned location
• After the binary is run, you delete the user-owned location and replace it with user data
• Later, the setuid binary (running as root) reads the untrusted data, interpreting it as trusted data.

This may or may not directly translate to Linux, but the idea is that you do not need write access to a file to exploit this problem. That is, it's not black and white in that sense.

[petrochenkov]

@retep998

I'm not aware of any functions that allow retrieving a handle to the process file directly (I can't even find the handle in the PEB!).

ReactOS sources is a good reference for Windows internals. Here, for example we can see that file identifier is kept inside of PEPROCESS structure as a kernel object (if I use the terminology correctly), and here's how it's turned into a file path.
I haven't finished reading yet, but I hope this file object pops up to the surface again somewhere in WinAPI.

[alexcrichton]

Nice find! Can we find any official documentation though for NtAreMappedFilesTheSame? Is that part of the API that's subject to change/breakage at any time?

[petrochenkov]

@alexcrichton
Not, unfortunately. Many NT APIs are officially documented as part of Windows Driver Kit, but not this function.
NT APIs are generally de facto stable because they are used by software in practice, NtAreMappedFilesTheSame, in particular, is available since XP and used on GitHub 3265 times, its implementation is available from Wine or ReactOS.
This PR also follows "best-practices" of working with unofficial NT APIs and loads the symbol dynamically. So, if NtAreMappedFilesTheSame suddenly disappears, then we get <unknown>s in backtraces again, this is the best that can be done.

[alexcrichton]

@petrochenkov yeah we'll be fine if the symbol disappears, but if the symbol changes ABI (e.g. different function signature) that'll cause segfaults, likely exploitable ones too.

@brson, @vadimcn, thoughts on using the private API of NtAreMappedFilesTheSame

[vadimcn]

I rate the chance of this API just changing its signature as very low. Microsoft has been pretty conservative with even undocumented APIs, especially ones that's been there for a long time (I found a reference to NtAreMappedFilesTheSame from as far back as 1998).

I think this PR is a fine short/medium term solution. In the long term we are going to re-write libbacktrace in Rust, right? :)

There may be other ways though: AFAIK, opening a file without FILE_SHARE_DELETE sharing mode will prevent anyone else from moving it. So perhaps we could query process image file name, open the file, then query again and compare the paths?

[alexcrichton]

@vadimcn I think that'd still be susceptible to the same race. Once you witness the filename it could get changed, and then wouldn't querying the current path still return that original file?

[vadimcn]

I thought @petrochenkov has determined that QueryFullProcessImageName returns the actual current file name?

[petrochenkov]

So, the sequence is:
T1. We query the path P1 of executable E using QueryFullProcessImageName.
T2. The original executable E is replaced with something named X, possibly X = E.
T3. We open path P1 without FILE_SHARE_DELETE, from now P1 is X are inseparable, i.e. P = P1 => content(P) = X.
T4. We query the path P2 of executable E using QueryFullProcessImageName.
T5. If P2 = P1 then content(P2) = X since at least T3. content(P2) = E at T4, so E == X since at least T3.

Looks like an alternative.

[petrochenkov]

This may require more invasive changes in libbacktrace though.
We need to use CreateFileW instead of posix open to pass FILE_SHARE_DELETE and to be able to validate unicode paths.

[alexcrichton]

@petrochenkov I'm not sure I quite follow your explanation but if it handles the race that sounds good to me. Presumably we could implement that entirely outside libbacktrace, right? That is, we do all the querying business to ensure that QueryFullProcessImageName is locked with FILE_SHARE_DELETE to not change. When that happens we can pass the path name manually to libbacktrace and then it'd open it up as usual?

(I like the idea of no changes to libbacktrace!)

[petrochenkov]

@alexcrichton

Presumably we could implement that entirely outside libbacktrace, right? That is, we do all the querying business to ensure that QueryFullProcessImageName is locked with FILE_SHARE_DELETE to not change. When that happens we can pass the path name manually to libbacktrace and then it'd open it up as usual

So the proposed action sequence is:

1. We query executable path P with QueryFullProcessImageName.
2. We lock (possibly replaced) file named P by opening it without FILE_SHARE_DELETE and throwing away the handle.
3. We query QueryFullProcessImageName again to validate that we locked the correct file.
4. We verify that P is ASCII to make sure libbacktrace will open the correct file when the time comes.
5. We initialize libbacktrace with ascii(P) using backtrace_create_state.
6. libbacktrace opens some file by name ascii(P) and it will be guaranteed to refer to our executable because P is locked.

---

There are two things I dislike here:

1. Executable (or some other file P) is locked for too long, since libbacktrace initialization until the process death (current implementation doesn't have this problem).
2. Non-ASCII paths are not supported (current implementation has much less problem - only paths not in the current locale are unsupported). This can be fixed by replacing "verify that P is ASCII" with "verify that P fits into current locale", and "initialize libbacktrace with ascii(P)" with "initialize libbacktrace with localize(P)", but this is the last thing I'd like to touch 😰

[alexcrichton]

Once the file is opened we can release the lock, right? And yes non-ascii paths wouldn't be supported but that seems to me like an acceptable tradeoff to not having to patch libbacktrace (and having this silently regress or prevent upgrades in the future)

[petrochenkov]

Once the file is opened we can release the lock, right?

Yep, it looks like libbacktrace successfully opens the file only once, so it can probably be unlocked after the first successfully obtained backtrace.

And yes non-ascii paths wouldn't be supported but that seems to me like an acceptable tradeoff to not having to patch libbacktrace

The ASCII issue affects home directories of all people with username in native non-ASCII language, while patched libbacktrace affects only a person updating our copy of libbacktrace (likely me), 1-2 times a year max.
Honestly, I just don't want to spend time on a useless rewrite bringing its own problems, I'd better do something else. A fix for the issue is implemented, it works, maintenance cost of patched libbacktrace is negligible and NtAreMappedFilesTheSame is as stable as it can be.

[alexcrichton]

The libs team discussed this a bit during triage yesterday, and the general sentiment was that we should avoid modifying libbacktrace and instead stick to Rust code where possible.

It seems like the only downside of not modifying libbacktrace is that it doesn't support non-ascii filenames, which mostly comes up with home directories. In that sense it seems like we could perhaps use a relative path to the current directory which likely won't include the username most of the time and cover almost all cases for debugging.

[petrochenkov]

I'll close this for now then since implementation external to libbacktrace won't share code with this implementation.
Hopefully, I'll be able to return to this before the next release.