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

Implement Secure Event Input for password fields on macOS #4738

Closed
saf-dmitry opened this issue May 13, 2020 · 25 comments · Fixed by #11623
Closed

Implement Secure Event Input for password fields on macOS #4738

saf-dmitry opened this issue May 13, 2020 · 25 comments · Fixed by #11623

Comments

@saf-dmitry
Copy link

Overview

Keyboard monitor active when entering password to unlock database

Steps to Reproduce

Keyboard monitor status can be seen on menu bar icon of txt expansion tool (e.g. Typinator)

Expected Behavior

Keyboard monitor should be temporarily deactivated when cursor in master password field

Actual Behavior

Keyboard monitor active when cursor in master password field

Context

KeePassXC - Version 2.5.3
Revision: f8c962b

Operating System: macOS 10.14.6

@saf-dmitry saf-dmitry added the bug label May 13, 2020
@phoerious
Copy link
Member

Is that some third-party application?

@droidmonkey
Copy link
Member

This has nothing to do with our application.

@saf-dmitry
Copy link
Author

This has nothing to do with our application.

Sorry, what I actually mean is, that the Secure Event Input is not activated when entering master password in KeePassXC password field.

EnableSecureEventInput was implemented in Mac OS X 10.3, to provide a secure means for a process to protect keyboard input to a custom data entry field. This function protects keyboard entry so that keyboard events cannot be intercepted by a keyboard intercept process. A Security Update implemented for Mac OS X 10.4 changes how the system affects keyboard intercept processes when a process uses EnableSecureEventInput.

@droidmonkey
Copy link
Member

See #3307

@saf-dmitry
Copy link
Author

Well, you are right, the Technical Note is too old. Nevertheless, text expansion software on macOS can read keystrokes and perform text expansion when entering password in KeePassXC password field, which is a safety issue. Other password managers like KeePassX or MacPass do inhibit this by activating Secure Input.

@droidmonkey
Copy link
Member

droidmonkey commented May 14, 2020

This is something that should be implemented by Qt, to be honest, for all obscured text edit fields. I doubt that KeePassX implements this feature, we are a direct fork of KeePassX and they did not have this capability implemented.

@droidmonkey droidmonkey reopened this May 14, 2020
@droidmonkey droidmonkey changed the title Keyboard monitor active when entering master password Implement Secure Event Input for password fields on macOS May 14, 2020
@droidmonkey droidmonkey added this to the v2.7.0 milestone May 14, 2020
@varjolintu
Copy link
Member

https://github.com/varjolintu/keepassxc/tree/feature/macos_secure_line_edit - I started this almost year ago, but didn't finish it. Maybe we could try to get this to work.

@saf-dmitry
Copy link
Author

I doubt that KeePassX implements this feature, we are a direct fork of KeePassX and they did not have this capability implemented.

Yes, it is interesting, because in my tests KeePassX v2.0.3 (using Qt 4.8.7 and libcrypt 1.7.3) does activate Secure Input on macOS 10.14.6.

@droidmonkey droidmonkey modified the milestones: v2.7.0, v2.8.0 Mar 21, 2022
@ShikiSuen
Copy link

This is an advice: If implementing this, please make sure it gets disabled immediately right after finishing using it.
Otherwise, all 3rd-party input methods will be prohibited from being activated by user.

@whispy
Copy link

whispy commented Dec 14, 2023

How is this not a high priority on the roadmap? As it currently stands, ANY application with the correct permissions on a Mac can read your password as you enter it. This seems like an incredible security flaw.

One argument against this is that if your Mac has a keylogger, you are already pwned. However, I don't think that is the case — it is far better to be keylogged and have your master password still be protected, than to be keylogged and have your master password be breached. Defense in depth is better than no defense at all.

What am I missing here? Why is this not a top priority?

@whispy
Copy link

whispy commented Dec 14, 2023

I would be willing to fund a bounty on this. I am not sure the level of effort required to implement it, so am not sure what would be a reasonable bounty. Say, $150?

@droidmonkey
Copy link
Member

droidmonkey commented Dec 14, 2023

As discussed above, this has some nasty side effects if it is not handled carefully. It is also unknown if this will work with Qt. Arguably, Qt should be providing this as an option. I don't even know how to test if this is even effective once implemented.

@droidmonkey
Copy link
Member

I just spent the past half hour trying to find any documentation for this feature. It doesn't exist. I'm closing this unless someone can produce documentation that indicates how this is implemented or if its even still a feature of macOS.

@droidmonkey droidmonkey closed this as not planned Won't fix, can't repro, duplicate, stale Dec 14, 2023
@droidmonkey
Copy link
Member

NSSecureTextField would require us to completely diverge from Qt which is a non starter.

@whispy
Copy link

whispy commented Dec 14, 2023

An idea (with the caveat that I am not very familiar with Qt or C++ OR the structuring of KeepassXC, and so the below could be trash):

  1. Can we create an empty Qt widget, then create an Objective-C++ wrapper class (which would allow us to mix Obj C and C++) for NSSecureTextField, and then add the wrapper to the newly created widget? Then we could use the widget like any other widget.

Since Qt does not natively support Objective-C++, we'd need to create a macOS-specific view containing the NSSecureTextField and then embed using a Qt QWidget.

Some really rough code that may be wrong:

// SecureTextFieldWrapper.mm
#import <Cocoa/Cocoa.h>

@interface SecureTextFieldWrapper : NSView
{
    NSSecureTextField* secureTextField;
}

- (instancetype)initWithFrame:(NSRect)frameRect;
- (void)setStringValue:(NSString*)value;
- (NSString*)stringValue;

@end

@implementation SecureTextFieldWrapper

- (instancetype)initWithFrame:(NSRect)frameRect
{
    self = [super initWithFrame:frameRect];
    if (self) {
        secureTextField = [[NSSecureTextField alloc] initWithFrame:frameRect];
        [self addSubview:secureTextField];
    }
    return self;
}

- (void)setStringValue:(NSString*)value
{
    [secureTextField setStringValue:value];
}

- (NSString*)stringValue
{
    return [secureTextField stringValue];
}

@end
// QtWidget.cpp
#import "SecureTextFieldWrapper.h"

// ...

SecureTextFieldWrapper* secureTextFieldWrapper = new SecureTextFieldWrapper();
QVBoxLayout* layout = new QVBoxLayout;
layout->addWidget(secureTextFieldWrapper);
setLayout(layout);

@whispy
Copy link

whispy commented Dec 14, 2023

A seprate idea — here is how Chromium handles secure inputs when a password field is focused: https://github.com/chromium/chromium/blob/3baf23064ea29859cc32fa3c750c2cfca3cf9999/ui/base/cocoa/secure_password_input.mm
and
https://github.com/chromium/chromium/blob/e304e2fb226255a159ad75931a70f6f834f70340/ui/base/cocoa/secure_password_input.h

It looks like they are manually calling EnableSecureEventInput(); and DisableSecureEventInput();. Perhaps this can be done in Qt, without actually changing the structure of password fields in KeepassXC? If it's secure enough for Chrome, after all...

@droidmonkey
Copy link
Member

Oh good, that points to where this function is located.

#import <Carbon/Carbon.h>

I'll see if we can add this to our PasswordWidget

@droidmonkey droidmonkey reopened this Dec 14, 2023
@droidmonkey droidmonkey self-assigned this Dec 14, 2023
@ShikiSuen
Copy link

ShikiSuen commented Dec 15, 2023

Just make sure DisableSecureEventInput() is called immediately as soon as any of the following conditions are met:

  1. the input field loses focus.
  2. the application loses focus.
  3. User starts interacting with another application.

Otherwise, all 3rd-party input methods installed in the system will be hindered by the SecureEventInput.

@ShikiSuen
Copy link

(Seriously, this feature needs a toggle.)

@phoerious
Copy link
Member

phoerious commented Dec 15, 2023

You should be able to just include the Cocoa headers normally without having to write anything in ObjectiveC. We have a bunch of ObjectiveC code in OSUtils, but for the most part it's not necessary. At least it wasn't for the CoreFramework headers I've been using recently.

@borowskimarcin
Copy link

What is the status of that issue? Sounds like a serious security problem?

@rshev
Copy link

rshev commented Jan 4, 2025

Same question, good exploration highlighting this has been posted at https://www.reddit.com/r/KeePass/s/9AArbpiHQS

@droidmonkey
Copy link
Member

Thank you for highlighting this issue again. I am looking into implementing the Enable/DisableSecureInput functions today. The reddit thread also highlights a great way to test it! Locking this now before we get a deluge of random comments.

@droidmonkey droidmonkey modified the milestones: v2.8.0, v2.7.10 Jan 4, 2025
@github-project-automation github-project-automation bot moved this to To triage in WIP Tracker Jan 4, 2025
@droidmonkey droidmonkey moved this from To triage to In progress in WIP Tracker Jan 4, 2025
@keepassxreboot keepassxreboot locked as too heated and limited conversation to collaborators Jan 4, 2025
@phoerious
Copy link
Member

phoerious commented Jan 4, 2025

I left the following comment on Reddit before people start fussing about it too much. Secure input is a nice enhancement, but not having it is certainly no "serious security problem". There also were some threat model misconceptions in the post, which want to rectify before readers get a wrong impression here.


The monitoring software Kidinspector required lots of permissions to be granted with an admin password, therefore such a software will not be installed by accident, but an employer or a public computer might have it installed without your knowledge.

And here's the problem: This attack requires a locally installed spyware with lots of permissions. If you are on a computer where someone managed to install such a software or where you have to expect that your employer is spying on you, your passwords aren't safe. Period. Secure input mode may offer some sort of weak protection against some very rudimentary forms of surveillance, but not by much. Certainly not enough that it was a big priority for us to implement so far. We will probably implement it at some point, but it's not nearly as much of a "security problem" as this post might make it appear.

Autofill generally does not use the clipboard and is therefore not vulnerable. But Strongbox, for example copies the TOTP code to the clipboard, which is therefore recored by *klipsustreamer (*but not by Maccy). KeePassXC uses autofill for the TOTP, which is therefore not leaked.

Auto-Type is absolutely "vulnerable", just not as trivially as the clipboard. Please do not get a false sense of security here.

Therefore KeePassXC should be used with a Key-File or a hardware key. Using the clipboard can mostly be avoided when using autofill.

This is certainly no mitigation. Especially key files, but also hardware keys, aren't any more secure than your actual password with regards to this attack vector. There is absolutely nothing that prevents a spy app from reading your key file without any particular permissions and with simple USB permissions, you can also challenge a plugged-in YubiKey. Both are there to improve the strength of your master key and perhaps they serve as an additional defence against shoulder surfing. However, they absolutely do not defend against malicious background software.

droidmonkey added a commit that referenced this issue Jan 4, 2025
* Fixes #4738
* Also fixes flaky handling of caps lock detection events
@keepassxreboot keepassxreboot unlocked this conversation Jan 4, 2025
droidmonkey added a commit that referenced this issue Jan 4, 2025
* Fixes #4738
* Also fixes flaky handling of caps lock detection events
droidmonkey added a commit that referenced this issue Jan 4, 2025
* Fixes #4738
* Also fixes flaky handling of caps lock detection events
@github-project-automation github-project-automation bot moved this from In progress to Done in WIP Tracker Jan 4, 2025
droidmonkey added a commit that referenced this issue Jan 19, 2025
* Fixes #4738
* Also fixes flaky handling of caps lock detection events
pull bot pushed a commit to Graysonbarton/keepassxc that referenced this issue Jan 26, 2025
* Fixes keepassxreboot#4738
* Also fixes flaky handling of caps lock detection events
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants