Keepassxc: Partial password may be leaked if it contains "{" and "}"

[yan12125]

Expected Behavior

No password is exposed unless explicit user actions (copy, auto type, export, ...) or pre-authentication (keepasshttp, ...)

Current Behavior

If a password contains {foobar} and it can't be resolved, an error message with partial password is printed to the terminal. For example:

Can't resolve placeholder {foobar} for entry with uuid 12345678

Possible Solution

1. Allow disabling placeholder resolution. Making ResolveMaximumDepth configurable should work.
2. Disable error messages that may contain sensitive information (unconditionally or only in RELEASE builds)
3. Don't print error messages related to placeholders
4. Don't run keepassxc from terminal

Steps to Reproduce (for bugs)

1. Run keepassxc from the terminal
2. Create a new entry
3. Set its password to foobar{foobar}
4. Check terminal messages

Context

I noticed quite a few "Can't resolve placeholder" messages in my terminal after bumping to the latest git-master. They are annoying and might have security issues. And seems such invalid placeholders are common in long generated passwords.

Debug Info

KeePassXC - Version 2.2.2
Revision: 5718f3d

Libraries:

• Qt 5.9.2
• libgcrypt 1.8.1

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 4.13.9-1-ARCH

Enabled extensions:

• Auto-Type

[TheZ3ro]

Nice catch

[TheZ3ro]

Setting milestone to v2.3.0 because the regression is in #1078 that will be available in 2.3.0

[TheZ3ro]

@frostasm what do you think about this? I know that the warning is there for let the user know that a placeholder is not supported (for example).

Most users won't run KeePassXC from terminal so they won't even read the warning, should we remove it entirely or should we remove the {placeholder} print from it?

ex qWarning("Can't resolve placeholder for entry with uuid %s", qPrintable(uuid().toHex()));

[frostasm]

@TheZ3ro tomorrow I'll try to offer some sort of solution

[TheZ3ro]

@frostasm You don't have to solve it youself if you don't want :smile: I can do it myself, just want to know your opinion since you added that chunk of code

[frostasm]

@TheZ3ro I need some time to think about this problem ;)

[frostasm]

@TheZ3ro I think we need remove qWarning for Unknown placeholders. And maybe in the future, we should add some validation for input fields, for example when we save the entry.

[rkiyanchuk]

Maybe it's better to exclude password field from any kind of parsing (for placeholders, etc)? Since data in the password field can be quite unpredicted, this could help avoid bugs due to poor sanitizing or parsing errors altogether.

[TheZ3ro]

@zoresvit this isn't a viable option since the password field can contain placeholders (that need to be parsed)
for exaple try cloning an entry.

[rkiyanchuk]

@TheZ3ro Sounds scary :) But thanks for the explanation.

[frostasm]

@zoresvit

@TheZ3ro was talking about cloning entries with references

[TheZ3ro]

Maybe we can set the password field as "special" and parse for placeholder only if it starts for { and ends for }

[frostasm]

Unfortunately, a random password can begin with {and end with } :(

[TheZ3ro]

Yup, even better since you will leak the full password 😝 (ironic btw)

[droidmonkey]

Checking for .begin("{REF:") && .end("}") would rule out 99.99999% of randomized passwords.

[TheZ3ro]

But this way you assume that a password must start with a reference placeholder otherwise no placeholder will be parsed.
If we want we can limit passwords to 1 reference only placeholder but I don't think this is a good approach.
I'm thinking of passwords like {REF:}some_more_data

First thing is stop reporting errors in release mode (#ifndef QT_DEBUG)

[rkiyanchuk]

@TheZ3ro While I don't see any use case for references and especially placeholders in the password field, if it is required by some, the only way to avoid ambiguity here is to store the reference information elsewhere. Like having separate variable for indicating the field contains placeholder/reference and specifying what placeholder exactly to look for. Otherwise it's just changing the possibility of conflict between random password and intentional placeholder.

[frostasm]

My main goal was to report problems with incorrect placeholders in the records and this can be done in other ways. For example, we can add a tool to check the correctness of placeholders in the database. So I agree with @TheZ3ro and think it will be enough do not print passwords in release mode for closing this issue.

[adolfogc]

@frostasm @TheZ3ro Building on @zoresvit's suggestion: what about using an entry's additional attribute for specifying such behaviour? We could agree on using the special name of resolve-password as a flag for "the password field contains a placeholder that needs to be resolved". If no such "flag" exists for a given entry, no placeholder resolving is done for its password field's content.

[TheZ3ro]

If the db comes from KeePass (that doesn't implement this special attribute flag) the password will still leaks.
I think it's fine to suppress errors in Release builds