tobischo / gokeepasslib

A library to read and write keepass 2 files written in go
MIT License
251 stars 30 forks source link

Encode corrupts data #75

Closed sitnikovv closed 2 years ago

sitnikovv commented 2 years ago
  1. Extract test.kdbx test.zip
  2. Run code on last version (v3.2.4):
    openFile, _ := os.OpenFile("test.kdbx", os.O_RDWR, 0644)
    saveFile, _ := os.OpenFile("test1.kdbx", os.O_RDWR|os.O_CREATE, 0644)
    db := gokeepasslib.NewDatabase()
    db.Credentials = gokeepasslib.NewPasswordCredentials("test")
    gokeepasslib.NewDecoder(openFile).Decode(db)
    db.UnlockProtectedEntries()
    entry := db.Content.Root.Groups[0].Groups[1].Groups[0].Entries[0] // DATA/DIR1/DIR2/FILE2
    fmt.Println("before encode PASSWORD: ", entry.GetContent(entry.Values[1].Key)) // password2
    keepassEncoder := gokeepasslib.NewEncoder(saveFile)
    keepassEncoder.Encode(db)
    fmt.Println("after encode PASSWORD: ", entry.GetContent(entry.Values[1].Key)) // password
    openFile.Close()
    saveFile.Close()
    os.Remove("test1.kdbx")

    password corrupted

if you run code on version 3.1.0 - all ok, data not corrupted

tobischo commented 2 years ago

Does the same thing happen if you lock the entries again before encoding it?

sitnikovv commented 2 years ago

Hmm ... If the entries before the encode are locked, no data corruption occurs. Thank you, I understood my mistake, but you can't find anything like this in the description (404 on examples) :) Is it possible to issue an error during encode if protected entries are not locked? Or forcibly lock before encode and unlock after it (if previously protected entries were unlocked)? To prevent others from making the same mistake as me.

tobischo commented 2 years ago

Yes, it should be possible.

Actually something like that is happening now already, as the internal XML structure is not always sorted the same way depending on which Keepass implemention was used to create the files. That would appear to break your file as well.

If the struct would keep state on whether it was already unlocked or not, it would probably be able to cover your case.

sitnikovv commented 2 years ago

Thank you

tobischo commented 2 years ago

The example file and the code you provided did not work for me btw

Regardless, it probably makes sense to have auto locking on encoding for cases where it is not yet locked. Otherwise the passwords will be stored unlocked and next time it will scramble the content when it is unlocked again - which is the same thing that happens if you call unlock twice