feat: Add Secure Keyboard Entry APIs in macOS #20678

mtgto · 2019-10-22T10:09:07Z

Description of Change

Add two APIs to app to control Secure Keyboard Entry.

In macOS, there is a way to protect from other processes listen keyboard input event.
https://developer.apple.com/library/archive/technotes/tn2150/_index.html

To enable it, we call EnableSecureEventInput, which available since Mac OS X 10.3.

This functions helps the apps become more security like bitwarden/desktop#298

app.setSecureKeyboardEntryEnabled(enabled) protects your application (your process) from listening keyboard input events. You have to disable before quit application.

app.isSecureKeyboardEntryEnabled() returns true if you already called app.setSecureInputEnabled(true).

To confirm your application is protected or not, below command is helpful.

ioreg -l -w 0 | grep kCGSSessionSecureInputPID

I write sample Electron app to test this PR is working. (2020-05-17 added)
https://github.com/mtgto/electron-secure-event-input-sample

I test with Swift-Keylogger whether this API protect from keylogger.
Here is my electron sample.

Password textfield is already protected by Chromium, but plain textfield is not.
I confirmed this API protect plain textfield from keylogger.

Here is the list of other open source softwares use the API.

Chromium
https://cs.chromium.org/chromium/src/ui/base/cocoa/secure_password_input.mm

iTerm2
https://github.com/gnachman/iTerm2/blob/master/sources/iTermSecureKeyboardEntryController.m

Checklist

PR description included and stakeholders cc'd
npm test passes(https://github.com/electron/electron/blob/master/docs/development/testing.md)
relevant documentation is changed or added
PR title follows semantic commit guidelines

npm test failed in my environment:

error /Users/user/work/js/electron/electron-gn/src/electron/spec/node_modules/is-valid-window: Command failed.
Exit code: 1
Command: node-gyp rebuild
Arguments:
Directory: /Users/user/work/js/electron/electron-gn/src/electron/spec/node_modules/is-valid-window
Output:
gyp: /Users/user/work/js/electron/electron-gn/src/out/Debug/gen/node_headers/common.gypi not found (cwd: /Users/user/work/js/electron/electron-gn/src/electron/spec/node_modules/is-valid-window) while reading includes of binding.gyp while trying to load binding.gyp

Release Notes

Notes: add app.isSecureInputEnabled() and app.setSecureInputEnabled(enabled) to manage Secure Keyboard Entry in macOS.

welcome · 2019-10-22T10:09:10Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

miniak

There are several incorrectly phrased sentences in the documentation.

miniak · 2019-10-22T13:26:39Z

shell/browser/browser_mac.mm

+void Browser::SetSecureInputEnabled(bool enabled) {
+  // See
+  // https://developer.apple.com/library/content/technotes/tn2150/_index.html
+  if (enabled) {


calling setSecureInputEnabled() with the same value twice should have no effect IMO.

mtgto · 2019-10-23T15:37:46Z

@miniak I apologize for wasting your time for my first PR. I almost rewrote.

There are several changes since first pull request:

Rename API
- User easily realize what the APIs are. The name is from menuitem of Terminal.app.
Use ScopedPasswordInputEnabler in Chromium instead of call EnableSecureEventInput() by API itself
- While debugging, I found app.SetSecureKeyboardEntryEnabled(true) causes crash Electron. This is because Chromium already use Secure Keyboard Entry in password textfield.
- Please grep with ScopedPasswordInputEnabler in textfield.cc
Rewrite API doc

mtgto · 2019-10-23T15:43:54Z

shell/browser/browser_mac.mm

+void Browser::SetSecureKeyboardEntryEnabled(bool enabled) {
+  if (enabled) {
+    password_input_enabler_ =
+        std::make_unique<ui::ScopedPasswordInputEnabler>();


initializer enable Secure Keyboard Entry.
https://cs.chromium.org/chromium/src/ui/base/cocoa/secure_password_input.mm

zcbenz · 2020-01-13T06:31:59Z

@miniak Can you take another look at this?

miniak · 2020-05-09T21:54:06Z

docs/api/app.md

+
+By default this API will return `false`.
+
+On _Linux_ and _Windows_, this API will return `false`.


This API should probably be only exposed on Mac.

miniak · 2020-05-09T21:54:54Z

docs/api/app.md

+
+**Note:** Enable only when it is needed and disable it when it is no longer needed.
+
+On _Linux_ and _Windows_, this API will do nothing.


The same here, the method documentation says it's only available on Mac.

miniak · 2020-05-09T21:55:10Z

shell/browser/browser.h

@@ -259,6 +263,11 @@ class Browser : public WindowListObserver {

  void RemoveObserver(BrowserObserver* obs) { observers_.RemoveObserver(obs); }

+  // Returns whether secure input is enabled
+  bool IsSecureKeyboardEntryEnabled();
+


why the space?

miniak

The implementation is good now, however these APIs should only be exposed on Mac IMO as they do nothing on other platforms.

mtgto · 2020-05-15T10:50:03Z

Thanks for your review. I'll fix this weekend.

Add methods: - app.isSecureInputEnabled() - app.setSecureInputEnabled(enabled) These enable to prevent other process listens keyboard input events.

mtgto · 2020-05-17T14:14:25Z

rebase current HEAD of master
Add example app https://github.com/mtgto/electron-secure-event-input-sample

zcbenz · 2020-05-18T01:44:19Z

Can you fix the lint error?

$ node ./script/lint.js && npm run lint:clang-format && npm run lint:docs
 
shell/browser/browser_mac.mm:440:  Add #include <memory> for make_unique<>  [build/include_what_you_use] [4]
 
Total errors found: 1

nornagon

Would it be possible to write a test for this?

In particular, I'm worried that this might interact poorly with the existing password field security, where calling setSecureKeyboardEntryEnabled(true), then focusing a password field, then focusing out would result in the secure keyboard entry being disabled.

jkleinsc · 2020-05-18T23:44:24Z

@electron/wg-api reviewed on May-18-2020.

mtgto · 2020-05-19T14:48:28Z

@nornagon

Would it be possible to write a test for this?

added. 619319f

In particular, I'm worried that this might interact poorly with the existing password field security, where calling setSecureKeyboardEntryEnabled(true), then focusing a password field, then focusing out would result in the secure keyboard entry being disabled.

I try to reproduce it by using sample app, but I can't.
https://github.com/mtgto/electron-secure-event-input-sample
Please tell me the way and your environment.

nornagon · 2020-05-19T17:45:54Z

@nornagon

Would it be possible to write a test for this?

added. 619319f

Thanks. I was hoping that it would be possible to test that the underlying feature was working—i.e. that it was protecting input from being read by other applications—but that seems hard and this test at least exercises the code path.

In particular, I'm worried that this might interact poorly with the existing password field security, where calling setSecureKeyboardEntryEnabled(true), then focusing a password field, then focusing out would result in the secure keyboard entry being disabled.

I try to reproduce it by using sample app, but I can't.
https://github.com/mtgto/electron-secure-event-input-sample
Please tell me the way and your environment.

Thanks, your test looks good. Looking at the underlying code in chromium it looks like there's a counter to track how many ScopedPasswordInputEnablers are present, so it should be fine.

welcome · 2020-05-21T15:53:47Z

Congrats on merging your first pull request! 🎉🎉🎉

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 22, 2019

mtgto added a commit to mtgto/electron that referenced this pull request Oct 22, 2019

fix: lint error in app.md for electron#20678

c388e97

miniak requested changes Oct 22, 2019

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 23, 2019

mtgto commented Oct 23, 2019

View reviewed changes

mtgto requested a review from miniak October 28, 2019 14:07

miniak reviewed May 9, 2020

View reviewed changes

miniak approved these changes May 9, 2020

View reviewed changes

nornagon added the api-review/requested 🗳 label May 11, 2020

mtgto added 4 commits May 17, 2020 10:04

feat: Add Secure Keyboard Entry APIs in macOS

842cd03

Add methods: - app.isSecureInputEnabled() - app.setSecureInputEnabled(enabled) These enable to prevent other process listens keyboard input events.

fix: lint error in app.md for electron#20678

a9bcdd3

fix: crash app.setSecureInputEnabled() in password textfield

8073ecc

fix: export Secure keyboard Entry API to only macOS

30f98fd

mtgto force-pushed the feat_darwin_secure_keyboard_entry branch from efe9c2c to 30f98fd Compare May 17, 2020 14:12

fix: lint error in browser_mac.mm for electron#20678

460f5a5

nornagon requested changes May 18, 2020

View reviewed changes

test: add test for app.setSecureKeyboardEntryEnabled in macOS

619319f

nornagon approved these changes May 19, 2020

View reviewed changes

jkleinsc added the api-review/approved ✅ label May 21, 2020

jkleinsc removed the api-review/requested 🗳 label May 21, 2020

jkleinsc merged commit 7b55a70 into electron:master May 21, 2020

arberx mentioned this pull request Feb 26, 2024

Secure Keyboard Entry for embedded terminal? microsoft/vscode#7548

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Secure Keyboard Entry APIs in macOS #20678

feat: Add Secure Keyboard Entry APIs in macOS #20678

mtgto commented Oct 22, 2019 •

edited

welcome bot commented Oct 22, 2019

miniak left a comment

miniak Oct 22, 2019

mtgto commented Oct 23, 2019

mtgto Oct 23, 2019

zcbenz commented Jan 13, 2020

miniak May 9, 2020 •

edited

miniak May 9, 2020

miniak May 9, 2020

miniak left a comment •

edited

mtgto commented May 15, 2020

mtgto commented May 17, 2020

zcbenz commented May 18, 2020

nornagon left a comment

jkleinsc commented May 18, 2020

mtgto commented May 19, 2020

nornagon commented May 19, 2020

welcome bot commented May 21, 2020


		By default this API will return `false`.

		On _Linux_ and _Windows_, this API will return `false`.


		Note: Enable only when it is needed and disable it when it is no longer needed.

		On _Linux_ and _Windows_, this API will do nothing.

feat: Add Secure Keyboard Entry APIs in macOS #20678

feat: Add Secure Keyboard Entry APIs in macOS #20678

Conversation

mtgto commented Oct 22, 2019 • edited

Description of Change

Checklist

Release Notes

welcome bot commented Oct 22, 2019

miniak left a comment

Choose a reason for hiding this comment

miniak Oct 22, 2019

Choose a reason for hiding this comment

mtgto commented Oct 23, 2019

mtgto Oct 23, 2019

Choose a reason for hiding this comment

zcbenz commented Jan 13, 2020

miniak May 9, 2020 • edited

Choose a reason for hiding this comment

miniak May 9, 2020

Choose a reason for hiding this comment

miniak May 9, 2020

Choose a reason for hiding this comment

miniak left a comment • edited

Choose a reason for hiding this comment

mtgto commented May 15, 2020

mtgto commented May 17, 2020

zcbenz commented May 18, 2020

nornagon left a comment

Choose a reason for hiding this comment

jkleinsc commented May 18, 2020

mtgto commented May 19, 2020

nornagon commented May 19, 2020

welcome bot commented May 21, 2020

mtgto commented Oct 22, 2019 •

edited

miniak May 9, 2020 •

edited

miniak left a comment •

edited