Incorrect protected fields decryption

[korovan-software]

In a rare cases, when database was formatted not perfectly by other library, protected fields decrypted incorrectly. But if I try to open it in original KeePass application, it decrypts everything correctly.

I can not understand the difference in chacha20 implementations, the only thing I figured out is that input parameters for chacha20.NewCipher() are exactly the same as in KeePass. So chacha stream initialized correctly.

There is the database (master password "123"): test.zip

It contains incorrect (empty) UUIDs, so I disabled your UUID length check just to try to restore database:

func (u *UUID) UnmarshalText(text []byte) error {
    id := make([]byte, base64.StdEncoding.DecodedLen(len(text)))
    length, err := base64.StdEncoding.Decode(id, text)
    if err != nil {
        return err
    }
    if length == 0 { // TODO: remove in next release
        return nil
    }
    if length != 16 {
        return ErrInvalidUUIDLength
    }
    copy((*u)[:], id[:16])
    return nil
}

Open file using modified gokeepasslib:

Open file in Keepass:

Could you please help to investigate the issue. It would be great if gokeepasslib will handle and repair such databases like KeePass, KeepassXC, Keeweb, AuthPass and others does.

[tobischo]

So gokeepasslib is simply following the given order in XML for the chacha20 stream encryption/decryption. My best guess without having looked into it yet is that the order of the entries is somehow off in the file compared to how the stream cipher was applied. Keepass2 probably uses a different way to order the entries or disregards or specifically regards certain entries.

The faulty UUIDs are also something you'd expect it to fix automatically or was that just a side note to make the file work for me?

[korovan-software]

UUID check disable is just a side note or scope of new feature. I guess it is a one line fix if we can just assign newUUID() in case it is empty. I’ll try to do it separately and provide a merge request.

But if order of items encryption depends on UUID values, it can be done in this issue as well.

[tobischo]

I think that is a separate issue then.

So for the background of what the issue is: My idea with the order of things being decrypted was right.

In the file you provided the groups are in the XML data before the entries. In my personal files and all the other files that I have seen that have been generated by keepass2, the entries were before the groups in the XML

See xml_examples.zip for an example from the kdbx4.kdbx from the examples directory and from the test.kdbx you provided.

During decrypt, it first checks for the entries and then it checks for the groups, so kind of breadth first search. https://github.com/tobischo/gokeepasslib/blob/00d6e82e75290ea22be851898f3ff14d3f3a4d04/crypto.go#L103

For your file, if we swap those lines it works, however it would break it for all the other files where this is not the case.

I also looked at authpass, where they just seem to grab the protected elements independent of that: https://github.com/authpass/kdbx.dart/blob/5cf6b8db9326aab165ccd1b564fccfedd79334eb/lib/src/kdbx_format.dart#L741 Which is relatively easy if you work on XML and not on structs, because that results in swapping the order anyway.

Based on my observation from the files form keepass2, the order in the struct is also defined: entries first, then groups https://github.com/tobischo/gokeepasslib/blob/00d6e82e75290ea22be851898f3ff14d3f3a4d04/group.go#L24 which the go xml marshaller also observes when writing it back into a file.

So of course the best solution here would be to be able to deal with that independent of the order, so that it doesn't matter as much if one of those other keepass tools does not return it in the same order

[korovan-software]

Thanks for the investigation. Yes I think the best solution is read any order, but write in correctly defined order as gokeepass does now. KeePass also behaves the same way. If you save the database with KeePass, then it will successfully open in gokeepasslib.

[tobischo]

The problems I see there are:

• With the standard XML decoder into a struct, the order is not preserved.
• When reading and writing the file without unprotecting and protecting it again, the order will also be changed and then the file is broken, because another tool will just assume that the order in the data stream matches the order of encryption with the stream cipher, independent of depth.

Therefore there are 2 issues:

• At the time of unlocking, we need to retrieve the correct order
• Before writing the files, we need to make sure that the entries are locked in the order we write the files, even before writing them out

The ordering part could be retrieved from the raw XML data or by keeping track during the XML un-marshal.

The writing part could be solved by unlocking and relocking it at least once before writing - could also be done during unmarshalling already.

I am still wondering how to best provide the scope during the unmarshalling for both the numbering and the immediate unlock and lock approach once done reading everything without, as I would like to avoid some package local variable that would cause issues when you have multiple files decoded at the same time in the same go process. The standard UnmarshalXML interface is also not made or even intended for this use case (See: https://groups.google.com/g/golang-nuts/c/Pof3i4IV4vQ)

[tobischo]

I prepared #74 as a proof of concept of how it could work.

Not entirely sure if that is the option I want to go with, but it would be great getting your feedback on whether that would work for you

[korovan-software]

Thanks, the fix is working for me. But I would like to investigate the issue a little bit more, because I like gokeepass clear and simple structure and this fix will add some complexity to the code.

[tobischo]

I agree that this is not the best solution in terms of implementation.

Dealing with the decrypting in the proper order will add some additional complexity though either way.

The question is whether we can come up with an approach that fits and whether that approach is workable without breaking changes.

I would like to avoid having to release a v4 over this.

So I am open for ideas

[tobischo]

I refactored it a bit to be simpler.

[korovan-software]

I'm not sure if it is good idea, but maybe we can just prepare list of entries UUIDs on database open as it ordered in the xml. Then on UnlockProtectedEntries() we can create map of all entries UUID to entry reference and iterate through initial list of UUIDs. But such approach requires that all UUID is unique and will not work with empty UUIDs, so empty UUIDs must be repaired firstly.

[tobischo]

That would also require keeping that state and passing through this list or a mapping - that was, aside from the potential conflicts, the part that felt really not suitable. I would like to avoid that part.

While the solution I prepared does not solve all possible orders, it does improve on it. If we come across an example where we have Entry and Group XML elements mixed, rather than either all of one and then all of the other, then I think we can revisit that that.

Also in my eyes the UUID issue is something rather unrelated and only occurred in the same example file, which means that there are 2 things "wrong" in the tool that generated the file.

I will merge #74 now, as that solution does not block us from implementing another approach should it become necessary.

[keepassium]

In a rare cases, when database was formatted not perfectly by other library

@Footmau, can you please name that library?

The generator property of your test.kdbx says "KeePass". I have recently received reports about some really odd (mis)formatting in DBs also supposedly generated by KeePass. So it would be very useful to know the culprit, at least to let them know about the issues.

[korovan-software]

@keepassium most probable it was kdbx.dart library.