Keepassxc: Legacy key upgrade dialog is hard to understand

[matthewblain]

When I open my password database, I get a dialog telling me that blah blah something might be out dated and I should do something unspecified to fix something.

The dialog should be more explicit about what this means and, more importantly, how to fix it.

Current Behavior

On opening a database, a warning containing technical terms is shown, with only and OK button and no way to either learn more nor to fix it.

Possible Solution

Some options:

• Offer to fix it then and there. Add a button [Help me fix it] which goes through... whatever process that is.
• Offer to show more info. A [learn more] button could bring up either another dialog, or just open a URL.
• Cheap but ugly solution is to put a URL in the text of the dialog box.

Steps to Reproduce (for bugs)

1: Open KeePassXc
2: Open a database. I assume this only applies where the database has a key file, but who knows.

Result:
See a dialog box:

Legacy key file format
---------------------------
You are using a legacy key file format which may become
unsupported in the future.

Please consider generating a new key file.
---------------------------
OK   
---------------------------

Expected:
Dialog box which tells me how to fix it, or even better helps me fix it directly.

(While looking for this I found issue #1698 which might be related in terms of UX?)

Debug Info

KeePassXC - Version 2.3.3
Revision: 0a155d8

Libraries:

• Qt 5.10.1
• libgcrypt 1.8.2

Operating system: Windows 10 (10.0)
CPU architecture: x86_64
Kernel: winnt 10.0.17134

Enabled extensions:

• Auto-Type
• Browser Integration
• Legacy Browser Integration (KeePassHTTP)
• SSH Agent
• YubiKey

[droidmonkey]

Ping @phoerious

[phoerious]

I expected users to know what a key file is when they use one. :confused:

[matthewblain]

Maybe.

I actually know what a key file is at a conceptual level. I'm sure I could figure out how to upgrade it in the UI. I could certainly dig into the implementation. But mostly I use KeePassXC as an every day tool and don't want to think about it. I set up my key and database using a different KeePass variant many years ago and have since mostly forgotten about it.

I do agree this might be a rare enough case that simple documentation is enough. Include a URL in the dialog box which points to a wiki page here or something. It should answer:

• What is the mechanism to "generate a new key file"? Is it the menu item called "change master key"? Something else? What gotchas (e.g. issue 1698) are there?
  
  
  
  • Maybe a reminder that I need to distribute the key file to the places where it's needed. (I have the DB file auto-sync'd, but not the key file for obvious reasons.) Though this is something a user should be expected to know.
  
  
  • What are the implications of me not doing it? Can I just say "OK" forever?
  
  
  • What are the implications of me doing it? Will it break something else--e.g. version 1.0 of this program (don't much care) or some other KeePass compatible program (might care)?
  
  
  • Heck, why is this happening? Is it because of some security issue identified, something else? This and the previous question could easily be a link to some article on the topic.
  
  

[phoerious]

I'm not blaming you and you are not even the first to not understand the message. We haven't received many reports, but a few. I just wonder why this is, because everywhere else the term "key file" seems to be clear enough.
We are simply phasing out an old format in favour of a newer format.

[nniesen]

I generated a new key file with stock keepass 2.41 using "Database > Change master key..." and dragging the mouse around. I am still getting the nag about legacy format and have no idea why it thinks the file is a legacy format.

[phoerious]

KeePass2 != KeePassXC. KeePass2 still generates those legacy key files.

[nniesen]

KeePass2 != KeePassXC. KeePass2 still generates those legacy key files.

Thanks, for now I'll just ignore the warning until I can get rid of this Mac at work an back on KeePass2.

Perhaps KeePassXC will need to removed, as a _Port_, from the keepass.info pages because KeePass2 can use any file as a key file.

[phoerious]

You can generate one with KeePassXC and use it with both KeePass2 and KeePassXC. KeePassXC will generate any file. KeePass2 does not. It can use one, but if you use their generation feature, it will create a legacy key file which has a specific format. You can inspect it by opening it in a text editor.

[nniesen]

That's good to know. I use my databases/keys on multiple devices so I need to make sure they work with the KeePass2 Ports that I can use on those devices. If every port started using "proprietary" file formats that would be a problem.

[phoerious]

That's exactly why we deprecated the more specific legacy formats. We still support them and probably will for some time to come, but it makes no sense to have anything but a plain stream of random bytes which is not parsed or interpreted in any way other than calculating a hash from it. KeePass2 advertises support for arbitrary key files, but the generator still generates an XML file, which is confusing to users at best.

[droidmonkey]

@nniesen that is just a warning, not an error. There is a checkbox on the warning to stop it from showing ever again.

[kksieski]

This is also an issue for me. I don't know how to remove the legacy key and add a new key.

[nniesen]

@kksieski: With the database open: Database > Change master key.... There you can Create a file with random data. You could also just check the box to never show the warning. It's kind of a bogus warning since KeePass was made more permissive and can use any file as a key, including the .key xml files generated from KeePass2.

[phoerious]

It's not a bogus warning. Reading an XML key file and adding the contained byte string onto our master key is not the same as hashing the full file and adding that onto our key. Hashing works with any file of any length, but the result is obviously not the same as reading the plaintext from the file. We deprecated the XML or TXT .key files in favour of simply hashing a file, since we consider that the only sane and sensible way of using a key file. With multiple formats, we always need to check the contents of the file for whether we need to parse it or hash it, which is plain stupid.

Unfortunately, KeePass still generates these legacy files. Everybody talks about how KeePass allows you to use just any file you want as a key file, but if you use that software to generate one, you end up with the opposite of that. When you generate a key file with KeePassXC, we write a long string of random bytes to the file. To calculate the master key, we hash the file and add it to the key. When you generate a key file with KeePass, it will generate a 32 byte random string, wrap it into a useless XML structure and write that to the file. Thus in order to calculate the master key, we need to parse the XML, extract the verbatim 32 byte string and then add it to the key (without hashing it), instead of simply running the whole file through a hash function. And to make matters worse, we need to check if the XML matches some non-existent specification, because you could of course use any other XML file as key file, but then we need to hash it and not try to find a 32 byte random string in it.

[droidmonkey]

What is even more inane about the KeePass XML key file is that it makes it trivially easy to identify the file that is your keyfile! The "standard" format is a dead giveaway.

[phoerious]

Addendum: our key file loading code looks like this with each loadSomething function being another 15 lines of code:

bool FileKey::load(QIODevice* device)
{
    m_type = None;

    // we may need to read the file multiple times
    if (device->isSequential()) {
        return false;
    }

    if (device->size() == 0) {
        return false;
    }

    // try different legacy key file formats
    if (!device->reset()) {
        return false;
    }
    if (loadXml(device)) {
        m_type = KeePass2XML;
        return true;
    }

    if (!device->reset()) {
        return false;
    }
    if (loadBinary(device)) {
        m_type = FixedBinary;
        return true;
    }

    if (!device->reset()) {
        return false;
    }
    if (loadHex(device)) {
        m_type = FixedBinaryHex;
        return true;
    }

    // if no legacy format was detected, generate SHA-256 hash of key file
    if (!device->reset()) {
        return false;
    }
    if (loadHashed(device)) {
        m_type = Hashed;
        return true;
    }

    return false;
}

If we could finally get rid of these dopey tests where we probe for four different formats trying to guess the correct one, it could look like this:

bool FileKey::load(QIODevice* device)
{
    if (device->size() == 0) {
        return false;
    }
    return loadHashed(device);
}

In fact, we could even drop the size check, because the hash of an empty string is well-defined.

[nniesen]

@phoerious This finally made it through my thick skull and made sense:

Reading an XML key file and adding the contained byte string onto our master key is not the same as hashing the full file and adding that onto our key. Hashing works with _any_ file of _any_ length, but the result is obviously not the same as reading the plaintext from the file. We deprecated the XML or TXT .key files in favour of simply hashing a file, since we consider that the only sane and sensible way of using a key file. With multiple formats, we always need to check the contents of the file for whether we need to parse it or hash it, which is plain stupid.

When the database master key is changed it stores the hash inside the database, you need to know if that hash comes from hashing the file or grabbing a hash out of the file. I can see there are multiple ways you could try to handle that which is why the conundrum exists. I can also see how this is hard to explain, especially to someone using files from KeePass2,

I liked the @matthewblain suggestion of a link to a page with more information. I'm not sure how I'd word it, but a comment like the following might better explain what's wrong:

To support backward compatibility, databases using legacy key file formats must use the legacy key file hashing strategy to unlock the database. To use the new key file hashing strategy that is compatible with all KeePass implementations, use a key file generator that does not create files matching the legacy formats.

[phoerious]

I created a proposal write-up for deprecating legacy key files in KeePass and posted it to their SourceForge ticket system: https://sourceforge.net/p/keepass/feature-requests/2434/
Let's see what the response is.