Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
StayPirate opened this issue May 21, 2024 · 8 comments
Open

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

StayPirate opened this issue May 21, 2024 · 8 comments
Labels

Comments

@StayPirate
Copy link

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 StayPirate added the bug label May 21, 2024
@StayPirate
Copy link
Author

Is #10618 somehow related to this?

@phoerious
Copy link
Member

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
Copy link
Author

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
Copy link
Member

phoerious commented May 21, 2024

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
Copy link

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
Copy link

darix commented Jul 16, 2024

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

@droidmonkey
Copy link
Member

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants