Keepassxc: Database size decreased by 35% after saving with KeePassXC

[chuim]

Expected Behavior

I have a years old database always operated using KeePass on Windows. I opened it for the first time yesterday with KeePassXC 2.4.1 on Mac and made minor modifications to it. I expected the final saved size of the database to be only slightly larger to accommodate the new data.

Current Behavior

The size of the database file saved by KeePassXC was 731 KiB, a significant decrease from the original 1.1 MiB.

Possible Solution

I'm aware this might not be a bug but given the significant difference in size I wanted to be sure. Maybe there's a way to dump the full contents of both DB files so that I can diff them and figure out what changed?

I haven't (yet) dumped the contents to CSV but I suspect that the current state of each entry might not reveal what actually changed.

Steps to Reproduce

I don't know how to repro but I have both versions of the DB -- pre and post save with KeePassXC -- safely stored thanks to Google Drive file version tracking. So I'm not worried that about lost data (the edits were minimal) and I can also run tests/comparisons myself if instructed to do so.

I also tested just saving the original database without any edits and the size reduction is approximately the same (it is a little bit smaller, as expected).

Context

I can share some details about my DB:

• It's a years old DB with a long history and hundreds of entries.
• I use a favicon downloader plugin so I have many custom icons embedded in the database.
• I checked the "Max history size" setting and it's set to 6 MB so I don't expect it to have caused this.
• Other DB settings that might be related: I use the recycle bin and enabled compression.

Debug Info

KeePassXC - Version 2.4.1
Revision: 7bafe65

Qt 5.12.2
Debugging mode is disabled.

Operating system: macOS Mojave (10.14)
CPU architecture: x86_64
Kernel: darwin 18.6.0

Enabled extensions:

• Auto-Type
• Browser Integration
• SSH Agent
• KeeShare (only unsigned sharing)
• YubiKey
• TouchID

Cryptographic libraries:
libgcrypt 1.8.4

[phoerious]

You probably had a large attachment in the history of your entry which got dumped according to the max history length setting (there are two settings, one is size, one is length).

[chuim]

Maybe many small attachments then... But notice that:

• I didn't see that entry in the KeePassXC database settings window. So even if it exists it's jarring that it's taken into account without allowing it to be known and edited.
• And if that was indeed set, did KeePass not honored it when it saved the original/larger file? This would indicate a (potentially significant) difference in behavior between it and KeePassXC.

[droidmonkey]

The other issue, which was fixed in 2.4.2, is if the same attachment was in the database binary data store multiple times, KeePassXC would discard the duplicate binary data, but not replumb the pointers to the singular attachment data. I suspect that KeePass itself was/is at fault for storing duplicated attachment data against it's own documentation.

We had to pull the 2.4.2 dmg, but it'll be back after this weekend.

[chuim]

I used KeePass to export the "before" and "unmodified after" databases -- exact same content -- to the "KeePass XML (2.x)" format and there was already a significant difference in size between the XML files. Further inspecting the diff between them revealed that the version saved with KeePassXC reduced the size of many icons (based on their "Data" tag contents).

Going back to KeePass I noticed that icons looked very similar between the two opened databases. I looked at one specific icon that presented the largest data size difference and there was minimal visual difference.

So it seems that KeePassXC runs some form of optimization of the icon data before saving them that KeePass does not. I'm guessing it resizes them to the small form factor they are generally presented at or maybe simply discards any higher DPI versions that might be embedded in the same favicon file. Can anyone confirm that a logic like this indeed exists?

But anyway, I agree with the KeePassXC choice in this case: why keep so much more data laying around when one will only ever see very small icons anyway? This might even be a nice improvement to propose for KeePass itself. :)

[droidmonkey]

From what I can read from the code we do not scale the image data on load or save. However, if you reload the icon from file or favicon download feature, we will limit the maximum resolution to 128x128.

[chuim]

There must be something else going on during save as the only thing I did in KeePassXC was opening the original database and immediately saving it.

As further evidence, these attached files show the before and after difference for that icon I mentioned with the largest data size difference. These were exported using KeePass (opening an entry, clicking on the icon, then "Export...") to PNG format. Two things are clear:

• The original has a 256x256 resolution while the one saved by KeePassXC is 16x16.
• They are not the same image, the original having a white background while the saved one seems to have a transparent background.

This made me think that the format of this image as stored in the original KeePass database is a multi-DPI one. To confirm this I went to the original website for this favicon -- SmashWords -- and downloaded it (attached ICO file; ZIPed so that GitHub will accept it). Opening it in GIMP allowed me to confirm that:

• It is a multi-DPI file.
• Its largest resolution is 256x256 and matches the exported PNG form the original database.
• Its smallest resolution is 16x16 and matches the KeePassXC exported PNG.

So it seems that KeePass keeps the original ICO format when storing it in the database and KeePassXC selected only the smallest DPI image from it and discarded all others when saving. I'm guessing this might be some enforcement of icon encoding format by KeePassXC during saves, while KeePass just keeps whatever it used initially.

SmashWords-favicon.zip

icon-16x16-saved-by-KeePassXC:

icon-256x256-original-from-KeePass:

[droidmonkey]

This could be an issue with Qt as well. I will run a couple tests, but it is good that this is isolated since we can now figure out where the fault lies.

[droidmonkey]

Indeed we do force a write as PNG for saving icon image data. We technically should not be dictating the format to write as. It is interesting that Qt handles ICO to PNG conversion by saving the lowest resolution of multi-DPI image.

https://github.com/keepassxreboot/keepassxc/blob/develop/src/format/KdbxXmlWriter.cpp#L175

[droidmonkey]

OK, I did some research. The way we handle custom icons is pretty terrible if they are multi-resolution ICO files. Here is what we are doing:

• Read: XML Data -> QByteArray -> Database::Metadata::QImage
• Display: QImage/QPixmap
• Write: Database::Metadata::QImage::Save -> XML Data

We store in our database model the processed QImage format of the ICO. This causes the first layer (16x16) to be read and displayed everywhere. Instead we should be storing the raw QByteArray data in the database model and only loading QImage/QPixmap representations for display in the GUI. The correct sequence should be:

• Read: XML Data -> QByteArray -> Database::Metadata::QByteArray
• Display: QImageReader(data) -> Choose correct resolution using QImageReader::jumpToNextImage() and QImageReader::size() -> QImage/QPixmap -> GUI
• Write: Database::Metadata::QByteArray -> XML Data

[chuim]

I'm trying to take a stab at fixing this issue, and hopefully that's alright? It might take a while though as I have to get used with the code base and with Qt. But I've got a few initial questions:

• From the your solution description I understand we should store ICO (or any other format) as raw bytes in memory and in the DB, correct?
• Why not just select the most suitable image -- which seems to be the best/closest to 128x128 --, discard the rest and save as single layer PNG?
• And to confirm: does KeePassXC only uses/cares about 128x128 icons?
• I found some usage of 16x16 in KeePass1Reader.cpp but I'm guessing that's just backwards compatibility?

Any further directions are appreciated as well. :)

[droidmonkey]

We can settle on 128x128 PNG for simplicity (also keeps things tidy). Storing the raw bytes in the db class has two advantages: we can further separate gui code from core code and the image format is fully preserved from load to save

[chuim]

The con of storing the original file is that we are potentially keeping way more data than it's needed for its intended usage, unnecessarily inflating the database file size. For instance, in the case of the (extreme) example icon above, the ICO file is almost 400 KiB while having only the 128x128 in PNG format amounts to 11 KiB. WDYT?

[droidmonkey]

Let's force convert everything to 128x128 PNG. We have to properly load multi size ico's though and select the right size to use. Its a bit of a pain.

[chuim]

OK, I'll go that route, which will already be less painful requiring less API changes to work with QByteArray. And IIUC your recommendation, I'll keep the image manipulation logic contained in the core code.

[chuim]

I have code uploaded that already compiles and passes all existing tests. The main changes are in Metadata, KdbxXml(Reader|Writer) and Tools (added image manipulation helpers).

I still need to do some cleanup and add my own tests, especially for the new Tools methods. But Let me know if you have any comments.

But one thing I realized is that all entry icons I see in KeePassXC's UI are presented in 16x16. Why then choose/prefer the 128x128 images? Are there plans to present them in higher resolution in the future? Otherwise, it would be preferable to prefer the 16x16 versions as scaled-down icons will look less sharp that icons specifically created for the lower resolution.

[droidmonkey]

High dpi screens use the extra data

[chuim]

I see. Now I understand the will to store more than just a single high res
icon. Sorry for not realizing that sooner.

I'll see if I can come up with something that fits both requirements. Maybe
going back to storing raw bytes, keeping all image sizes between 16x16 and
128x128, and figuring out a way to detect the screen DPI to select the most
suitable image?

[droidmonkey]

That is a lot of work, especially since Qt should be handling this in the backend. You can setup an experiment by creating several icon sizes on a UI element and see if the QPixmap loaded with a multi-res ico automatically scales appropriately.

[txtsd]

Why was this closed?

[droidmonkey]

The topic of this issue is all over the place. I will open a new issue constrained to better handling of database icons.