keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.42k stars 1.48k forks source link

FdoSecrets: client fingerprinting #6458

Open michaelk83 opened 3 years ago

michaelk83 commented 3 years ago

Following my comment on #2726 and after having read the discussions on #4733, #5747, and others, I'm wondering if FdoSecrets client fingerprinting would be a useful addition?

Proposal

  1. When an FdoSecrets client connects for the 1st time, after its executable path has been determined by the implemented means and user has authorized access, obtain a fingerprint of the executable for future reference. This can be as simple as a SHA hash of the executable.
  2. Store the executable path, fingerprint, and access authorization in a hidden entry in the DB. This entry should NOT be accessible via FdoSecrets.
  3. When the client attempts to connect again afterwards, obtain again its fingerprint, and compare to the stored fingerprint.
  4. If the fingerprints don't match, display a warning "fingerprint changed" and require re-authorization.

Advantages

  1. Further raises the bar on client impersonation attacks.
  2. Provides a foundation for authorize-once per client. Currently (as I understand), authorization is required for each new client connection.
  3. Side-effect: detects (some forms of) tampering with the client executable.

Limitations

  1. Other (potentially easier) attack vectors are still available in userspace, such as LD_PRELOAD attacks.
  2. Re-authorization is required when a client executable is updated to a new version. Personally, I consider this a feature, and it's already the case e.g. when Python is updated (e.g. /usr/bin/python3.6 changes to /usr/bin/python3.7).
  3. Scripts running on the same binary (e.g. on python) still can't be distinguished. That's a known limitation of the existing binary detection code, and fingerprinting won't fix that.
  4. Performance: hashing can be slow for large client binaries.
jpathy commented 3 years ago

I don't think this should be done.

  1. /usr/bin/python3.6 hash match doesn't really guarantee the script it is executing is not modified. In fact this would be easier to do than patching/replacing python. And getting actual cmdline of a process is really not possible in linux, if we assume that process to be hostile.
  2. Since a lot of shell scripts might use secret-tool and the shell script itself might be attacker controlled, it gives a false sense of security.
  3. The other limitations you mentioned also apply.

Just showing the pid should be enough. If we want to give a bit more information what might be more useful is the parent process tree of the process like pstree does: image

michaelk83 commented 3 years ago

The binary path detection is already implemented, IIRC in #5747 . This proposal only adds fingerprinting on top of that.

You are correct that scripts can't be distinguished. That's a known limitation of the binary detection code. But binaries can be (python vs kmail, for example). So if you see "python", this doesn't really help much, other than "Is python supposed to be running at all right now?". But if you see "kmail", you can be reasonably sure it's kmail.

Scripts using secret-tool is just another category of scripts (it should still be possible to distinguish secret-tool from python, though).

jpathy commented 3 years ago

The binary path detection is already implemented,

My screenshot was about showing the parent process branches upto the trunk as a treeview not just about the binary path. This is arguably more useful than just showing the pid but will need a bit more work.

But if you see "kmail", you can be reasonably sure it's kmail.

Since you are proposing we remember the decision to not show any authorization prompt if hashes match, this is really bad to just store secret-tool or python hash and pretend the client is legit.

Besides changing "core" binaries(like python, kmail etc installed by the pkgmgr) will require root on most systems and since having root means you can already do all the naughty things like ptrace. So it will be useful for compiled user executables. Most users will have custom scripts if they want to interface with secrets protocol and this binary hashing scheme is not useful.

michaelk83 commented 3 years ago

The binary path detection is already implemented,

My screenshot was about showing the parent process branches upto the trunk as a treeview not just about the binary path. This is arguably more useful than just showing the pid but will need a bit more work.

I know, and I agree that showing the process tree can be useful. But that's a separate feature. I was just saying that the binary detection logic is not introduced in this proposal, and will not be affected by this proposal. This proposal acts after the binary has been detected by the existing means (and authorized by the user).

The merits and limitations of the binary detection have been discussed at length in #5747 and its linked issues. Most of what you're saying is not new. And most of your arguments regarding hashing apply just as much to the (existing) binary detection alone.

I do agree with much of what you're saying. My current view is that the primary merit of this proposal is as a foundation for authorize-once per client.

Since you are proposing we remember the decision to not show any authorization prompt if hashes match, this is really bad to just store secret-tool or python hash and pretend the client is legit.

The decision on whether to ask again for the same binary or not, should be up to the user, and separate for each binary. IMO it should default to "ask every time". I agree it would be wise to blacklist certain binaries from authorize-once. But it can still be useful with other binaries.

jpathy commented 3 years ago

And most of your arguments regarding hashing apply just as much to the (existing) binary detection alone.

Afaiu the current scheme remembers the authorization for the process lifetime(client/keepassxc). Your proposal is to remember this decision beyond this.

There is no additional security benefit to hashing compared to just simply storing the binary path if we do this. And hashing will show unnecessary warnings when we update and it will just keep changing the DB(this is what i don't like the most). Might as well store the path and let user decide.

michaelk83 commented 3 years ago

Your proposal is to remember this decision beyond this.

Upon user choice, per binary.

There is no additional security benefit to hashing compared to just simply storing the binary path if we do this.

I think there may be (as noted in the OP), but I agree it's minor at best. The thing is, the binary detection is not bullet-proof. Checking the binary hash ensures that it really is the same binary that the user has previously authorized.

jpathy commented 3 years ago

Checking the binary hash ensures that it really is the same binary

But not its library dependencies which might have changed. Particularly the path is more secure in distributions like nixos/guix than the hash of the binary.

michaelk83 commented 3 years ago

Good point re libraries, but the same applies to the binary path alone (without hashing).

Re "path is more secure", the path would always be checked anyway, since you need it to find the hash. The hash would only be an additional check on top of that.

jpathy commented 3 years ago

With paths it would work like the allow list as the urls used in the browser plugin data. It would be simple and not make the DB change unnecessarily. In nixpkgs the path is derived from all the dependencies, so it is actually more secure, and the binary hashes are not needed at all.

michaelk83 commented 3 years ago

It would still work the same with or without hashing. The primary identifier is still the path, either way. The hash is just an extra internal check that's transparent to the user (other than having to re-authorize when the binary is modified).

I don't expect the DB to change very often because of the hashes. Binaries aren't updated that often. If you work with a lot of passwords, the entry data can change more frequently.

I don't mind if this fingerprinting will be optional. I agree that authorize-once can still work without it, based on only the paths. But I do think it can add a bit of extra confidence.

Anyway, the final decision is not up to me (and can be made later).

Aetf commented 2 years ago

I just discovered this SO answer https://stackoverflow.com/a/39001204. Basically you can get the original binary file content regardless of whether the path has been replaced with a new executable or not. So binary hashing becomes more reliable than I initially thought about it.

Regarding limitations on shell/python/other scripts, what @droidmonkey said in https://github.com/keepassxreboot/keepassxc/issues/7571#issuecomment-1075014332 can be useful. I.e. we can check the path/hash of not only the calling parent, but also any parent processes in the tree. This can be quite powerful and ideally should also come with an editor so the user can add/remove rules.

By default clicking on "Allow All & Future" will add a new rule matching only the calling parent, but the user should be warned when the calling process looks like a common scripting binary.