keepassxreboot / keepassxc

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

Clear recent file history registry entry when opening database key files #10286

Open Flip-ops opened 6 months ago

Flip-ops commented 6 months ago

Summary

Opensavepidlmru saves last 20 files opened/saved via Windows Common Dialog 2.

Known Problem

The Windows registry holds the knowledge as to which file is being used as a security key file. So an attacker could use a logged in users session, keylog the password the user enters for the database. Download the database file. Check registry for the security key file, download that. Then the attacker has all the keys to the kingdom and only needs Keepass installed, provide the newly fetched database file and along that the security key file to gain access to entries such as passwords etc.

Solution?

So KeePassXC could, as a possible solution, after one logs in with security key file: Open any file 20 times in a folder Keepass knows, for example its own folder (as long as that file is not the security key used). This would therfore overwrite the history in the registry for which security key is acctually used. This is not ultimate or even optimal, but maybe a bit more secure.

Example code (I’m not good at C++)

`

include // Include for file handling

include // Include for input/output

include // Include for getenv

int main() { const char* homeDir = getenv("HOME"); // Get the HOME environment variable

std::string logFilePath;
if (homeDir) 
{
    logFilePath = std::string(homeDir) + "/.cache/log.txt"; // If HOME is set, use it to construct the log file path
} 
else 
{
    logFilePath = "./log.txt"; // Fallback log file path in the current working directory
}

std::ofstream logFile(logFilePath.c_str()); // Open the log file

if (!homeDir) 
{
    logFile << "Error: HOME environment variable not set.\n"; 
    return 1; // If HOME is not set, print an error to the log file and return 1
}

for(int i = 0; i < 20; ++i) // Loop 20 times
{
    std::ifstream file("readme.md"); // Try to open the file "readme.md"

    if(file.is_open()) // If the file is opened successfully
    {
        logFile << "File opened successfully for the " << i+1 << " time.\n"; // Write to the log file
        file.close(); // Close the file

        if (!file.is_open()) 
        {
            logFile << "File closed successfully.\n"; // Write to the log file
        } 
        else 
        {
            logFile << "Failed to close the file.\n"; 
            return 1; // If the file is not closed successfully, write to the log file and return 1
        }
    } 
    else 
    {
        logFile << "Failed to open the file.\n"; 
        return 1; // If the file is not opened successfully, write to the log file and return 1
    }
}
return 0; // If everything is successful, return 0

} `

droidmonkey commented 6 months ago

So an attacker could use a logged in users session, keylog the password the user enters for the database. Download the database file.

You've lost right there. Nothing we do with some scheme to try to flush registry settings will save you.

Overall we do not recommend using a key file at all, it's just a silly carryover from KeePass2. Buy a Yubikey if you want two factor login for your database.

Flip-ops commented 6 months ago

So an attacker could use a logged in users session, keylog the password the user enters for the database. Download the database file.

You've lost right there. Nothing we do with some scheme to try to flush registry settings will save you.

Overall we do not recommend using a key file at all, it's just a silly carryover from KeePass2. Buy a Yubikey if you want two factor login for your database.

Sure I know there are other options such as physical security keys to buy. But it’s still a carryover as you say, a feature available, and isnt the goal to keep Keepass options secure?

This was my thinking: Either erase the option or make it a bit more secure?

Please explain why my suggestion wouldn’t raise the security even a tiny bit. What am I missing?

droidmonkey commented 6 months ago

For one that is weird behavior of the application to do what you suggested. I definitely wouldn't implement a scheme to create and open a bunch of random files in an attempt to spoof Windows file history.

One could technically just erase that line in the registry itself. However, Windows usually stores this information in a hundred different locations...

Flip-ops commented 6 months ago

Yeah I agree it’s wierd. As I mentioned it’s not an optimal solution. I’d like to see it erase the registry value, but I didn’t know a good solution for that either.

Well I think the erasing option still sounds good; as it raises the bar a tiny bit for an option that enables a lower level of the security, compared to recomended options.

If you could solve the erasing, then it would at least make finding the key file one step harder and less obvious for an attacker. Users that use the key file for additional security would get a bit more security.

Flip-ops commented 6 months ago

Thank you :) and I don’t know if this help since I’m not good in C++, but I dont know how to write a solution that erases the registry values that the program itself created, so as not to delete history made by other programs.

`#include

include

int main() { HKEY hKey; LONG result = RegOpenKeyEx(HKEY_CURRENT_USER, TEXT("Software\Microsoft\Windows\CurrentVersion\Explorer\ComDlg32\OpenSavePidlMRU"), 0, KEY_ALL_ACCESS, &hKey);

if (result == ERROR_SUCCESS)
{
    result = RegDeleteValue(hKey, TEXT("add value for key to delete"));

    if (result == ERROR_SUCCESS)
    {
        std::cout << "Successfully deleted the value." << std::endl;
    }
    else
    {
        std::cout << "Failed to delete the value." << std::endl;
    }

    RegCloseKey(hKey);
}
else
{
    std::cout << "Failed to open the key." << std::endl;
}

return 0;

} `

Vinfall commented 6 months ago

I think you are not getting the point too.

So an attacker could use a logged in users session, keylog the password the user enters for the database. Download the database file. Check registry for the security key file, download that.

It's based on the assumption that If a malicious entity has access to your registry, keyboard input and file system, it ever wants to check a password manager before hijacking all your files & asking for crypto compensation, stealing those credentials in files or browsers or wherever with more established hacking tools and many other illegal stuff, which is IMO very unlikely. It's not at least the most urgent thing to do for an attacker, if not the last.

Don't get me wrong, I agree it's a bit insecure as a tradeoff between convenience and security, but to me you seem to overthink about it. If there is anything wrong with this scheme, you should really reconsider about your threat model, encrypt your keyfiles with LUKS/VeraCrypt, or... buy a physical key instead.

edunfelt commented 6 months ago

To interject in favor of @Flip-ops's point of view here, it can still be of interest to ensure that the existing features are implemented as securely as possible, despite being generally recommended against among security enthusiasts more broadly. There are a number of reasons for this:

whindsaks commented 3 months ago

This would therfore overwrite the history in the registry for which security key is acctually used

Windows does not store the history of randomly opened files, only files picked by the user in Open/Save dialogs. There are some things one can do to prevent/reduce this; set OFN_DONTADDTORECENT/FOS_DONTADDTORECENT, call IFileDialog::ClearClientData.

Flip-ops commented 1 month ago

This would therfore overwrite the history in the registry for which security key is acctually used

Windows does not store the history of randomly opened files, only files picked by the user in Open/Save dialogs. There are some things one can do to prevent/reduce this; set OFN_DONTADDTORECENT/FOS_DONTADDTORECENT, call IFileDialog::ClearClientData.

No I don’t think this is relevant. That would make for example recently opened documents not show up in recent. But wont clear the Registry value Opensavepidlmru that we talk about here. But if you have tested and validated your suggestion and got a different result, then by all means please show me more :)

whindsaks commented 1 month ago

But wont clear the Registry value Opensavepidlmru that we talk about here.

It will not clear the registry of old entries but OFN_DONTADDTORECENT/FOS_DONTADDTORECENT are flags that say, for this open file dialog, don't remember the file the user chose this one time, perfect for the password file.

Flip-ops commented 1 month ago

Have you validated that using Keepass with a security key file creates a value where you are suggesting intervention?

whindsaks commented 1 month ago

I did not validate it, no.