search

keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.18k stars 1.47k forks source link

Discussion about CVE-2024-33900 and CVE-2024-33901 #10784

Open StayPirate opened 5 months ago

StayPirate commented 5 months ago

CVE-2024-33900 and CVE-2024-33901 have been assigned by Mitre to this highly professional report.

IMO being able to dump plaintext creds from the process memory, when the database is unlocked, is a normal behavior and not a bug. The reporter also mentions:

When the database is closed or locked the probability is 1 in 10 respectively 4 in 10.

which indeed sounds weird.

My following reasoning is purely hypothetical due to poor details shared by the reporter and me not having reproduced the bug.

When the database gets locked, I expect there will be some allocated memory being freed. That memory might contain data from when the database was unlocked and should be zeroed before being released. Do you think that non-zeroed freed memory could be the cause?

StayPirate commented 5 months ago

Is https://github.com/keepassxreboot/keepassxc/pull/10618 somehow related to this?

phoerious commented 5 months ago

We zero out all sensitive memory that we allocate directly and we try to zero out all memory that we don't control by overriding the delete operator (see https://github.com/keepassxreboot/keepassxc/blob/develop/src/core/Alloc.cpp#L58).

Unfortunately, this doesn't work in all cases and Qt being Qt can leave traces behind. My guess is that these traces come primarily from buffer reallocations. This is a known issue and nothing new. On platforms that support it, we have access restrictions in place to make it much harder to dump memory in the first place.

StayPirate commented 5 months ago

Thanks for the prompt answer, may I ask which platforms support it? Is this problem only confined to Windows builds or does it also affect Linux?

phoerious commented 5 months ago

On Linux, we can only disable Coredumps. On MacOS, we prevent ptrace, and on Windows we prevent memory access via DACLs.

The problem itself affects all platforms and probably all password managers in existence, unless they use a special hardened UI kit (I wouldn't know of any).

krieghof commented 3 months ago

On Linux, we can only disable Coredumps. On MacOS, we prevent ptrace, and on Windows we prevent memory access via DACLs.

The problem itself affects all platforms and probably all password managers in existence, unless they use a special hardened UI kit (I wouldn't know of any).

TIL about mlock and how it prevents pages from swapping. What do you think about using mlock, could KepassXC benefit from it?

darix commented 3 months ago

you can not really mlock the whole memory that is managed by Qt ?

droidmonkey commented 3 months ago

At some point the data needs to be landed at the UI layer. We can certainly limit the amount of data that lands there (which we do) but it's really a losing game.

krieghof commented 3 months ago

you can not really mlock the whole memory that is managed by Qt ?

Yeah, I don't think it is possible to mlock the memory that is managed by Qt. I was thinking about something else, maybe this is even off topic and should be a new/other issue.

I only skimmed through the manpage, so please correct me if I'm wrong or if my logic is flawed.

mlock prevents pages from being swapped to disk.

Imagine a scenario where memory containing the unlocked database gets swapped to disk and the PC crashes. Now, the disk with the swap partition contains the unlocked database, even though the PC is off. This wouldn't be an issue if the swap partition is encrypted, but otherwise, it poses a security concern.