DreymaR / BigBagKbdTrixPKL

"DreymaR's Big Bag of Keyboard Tricks" for Windows with EPKL
Other
326 stars 32 forks source link

KeyHistory should be disabled by default #63

Closed dfrvfmst closed 1 year ago

dfrvfmst commented 1 year ago

Currently EPKL silently records all the inputs in plain text with the KeyHistory by default, which is not good from a security point of view. KeyHistory should be strictly a debugging tool.

Hiding the KeyHistory menu (with advancedMode = no) or disabling the KeyHistory shortcut (with epklDebugHotkey =) doesn't really stop EPKL from recording it. The user just have no way to access it interactively, while the recorded data still sits in memory.

Consider adding #KeyHistory 0 by default, with a user overridable option to reenable it on demand.

DreymaR commented 1 year ago

Thank you for your post! Your points are valid, however:

I don't think that users running a keyboard program themselves on their own computer consider the AutoHotkey default KeyHistory a big security risk.

Note that the Key History buffer only holds the last 40 key presses, not a significant log. So it'd take some pretty specific breach to use it for anything devious.

It's easy for any concerned users to add the directive you mention and compile. You are the first to mention it. I believe that the majority of users would rather, like me, have the KeyHistory available for debugging – which surprisingly often becomes useful when setting up advanced options.

To address anyone feeling this way, I will add a #KeyHistory 0 line to _PKL_Main.ahk. It will however be disabled by default.

dfrvfmst commented 1 year ago

Thanks for you replying.

I'm not very familiar with AutoHotKey 1. Is the #KeyHistory impossible to be overridden in runtime once it's compiled? Because as a debugging tool, it should only be enabled when needed, and it's what most other software do.

Say if you want to tweak layouts, but how many time would you actually tweak it? It should be tweaked once and forget it (Do you change Group Policy all the time?). So I would image most users messing around the program for the first week and then being done with it for years. KeyHistory at this point will serve no use, except adding security risk. Even if you ignore the security risk, continuously logging (as plain text) in the background also adds more CPU usage that are mostly wasted, and EPKL is a latency-oriented program running under realtime priority, so any CPU cycle should matter.


If AutoHotKey doesn't really allow #KeyHistory to be overridden in runtime (So you can't make it user-overridable in config):

Does AutoHotKey allow executing external programs? If you compile a "standalone KeyHistory" program which the sole purpose is to log inputs, and then add a entry to start that KeyHistory program in the main program, it would:

DreymaR commented 1 year ago

No, to change the #KeyHistory directive you have to recompile. That's how compiler directives work, I'm afraid. At least ttbomk.

I don't know what this "most other software" is. I don't know of other keyboard programs like (E)PKL for Windows, come to think of it. As for "most other software" in the AHK universe, I think you're wrong: Many scripts just leave it on and never worry about it. As I said above, you're the first I've heard of (since 2009).

The way I'll provide lets users remove one semicolon in the main file and then compile, to shut off KeyHistory completely as they wish. I think that should suffice. Less advanced users are more likely to need it than be afraid of it.

dfrvfmst commented 1 year ago

As I said above, you're the first I've heard of (since 2009).

Being the very first person to talk about it doesn't mean it's a edge-case or it should be ignored. It could also mean that there was no one who cares about both efficient keyboard layout and security and daily-drive Windows and bothered to open an issue before me.

Like, try to note about this behavior right in a conspicuous place of your readme.md: "⚠this program will record your last 40 key presses in plain text, including passwords. if you want to disable it then recompile yourself", how do you think users will react to that? Or try posting this exact issue in any security or privacy-oriented forums or subreddits, what responds do you think you would get? And I'm not even exaggerating it, this is basically a low-hanging fruit from a security PoV.


I don't know what this "most other software" is.

Too many to even give examples. Firefox to name one. With the log level option it can happily spit 1000 lines every seconds if user really wants it to, but they're all disabled by default. Debugging tools are strictly debugging tools for debugging purposes.

I don't know of other keyboard programs like (E)PKL for Windows, come to think of it

ReWASD to name one. A proprietary software that are also capable of handling multiple layers of modifiers. It doesn't really have to use logging to help remapping anything because everything is configured via GUI so you just press your key combos to set up.


The way I'll provide lets users remove one semicolon in the main file and then compile, to shut off KeyHistory completely as they wish.

It's OK to me. I can recompile it on every program update just fine (that's what i did on Linux desktop for the past decades and I will even write a script for it) or even further, resort to ReWASD and recreate the BigBag layouts myself.

However, I created this issue in a hope to make casual users benefit from the added security, as they may not realize the potential consequences of leaving KeyHistory on. If I only care about my own usage this issue wouldn't have existed at all because appending a single line and recompiling is not a hard thing.


Many scripts just leave it on and never worry about it

They are, because they are scripts, not a full-fledged software with GUI configurable settings. Scripts are more likely to be modified (Open up text editor and add a line and you're done) and be used by power users. Also, quoting the official documentation:

If the script does not have the keyboard hook installed, the KeyHistory window will display only the keyboard events generated by the script itself (i.e. not the user's)

Which means the logging scope is very likely much smaller than what EPKL does.


Also, what's stopping you from doing this:

If you compile a "standalone KeyHistory" program which the sole purpose is to log inputs, and then add a entry to start that KeyHistory program in the main program, it would:

  • Allow users to start debugging key presses anytime they want
  • Keep the KeyHistory disabled by default when users are not actively debugging it

This would make EPKL to be just like what most other software do: making debugging features optional.

If you simply don't want to take the time to do that because it's not important to you, then that is also OK to me. You're maintaining the software for free anyway, so you get to choose what sits in your own project (I may take my time and send a PR if I get the mood but I'm not familiar with AutoHotkey nor this project so poor code quality is expected). Just remind you that this project being the very most popular BigBag implementation on Windows.

DreymaR commented 1 year ago

I may indeed add this piece of information to the README. Just, I don't think most users read it through. There's already a lot of info to take in there. And a lot of things I already want to be in "conspicuous view".

If you can make such a standalone program work, then please make a PR for it! It'd be a useful thing. However: I have my doubts that tack will work. The main program needs to have the keyboard hook to make the hotkeys that remap keys etc, and the hook is what KeyHistory uses. Having a separate hook in the standalone program will likely lead to disaster as you'll incur "hook competition" and both scripts may become unreliable. Maybe if you make sure the standalone gets priority and has zealous pass-through... but I won't bet on it at this point. At any rate, by itself it'd be a hobbled and less useful KeyHistory as it wouldn't see the remapped key presses from the main program. Not sure how you'd make that work?

dfrvfmst commented 1 year ago

Doing some very basic researches I believe that #KeyHistory is a compile time parameter and it's hard-coded into the resulting binary. There is no way to override it in runtime.

The very first thing I came into mind is creating two different entrypoint programs with different KeyHistory parameter.

This would be:

;;EPKL.ahk
#KeyHistory 0
DebugMode = 0
#Include %A_ScriptDir%\EPKL_Source
...
;;EPKL_debug.ahk
#KeyHistory 500
DebugMode = 1
#Include %A_ScriptDir%\EPKL_Source
...

Then make changes to the _PKL_main.ahk to add an entry "Restart in debug mode" (taking over advancedMode), and show/hide related entries based on the entrypoint (The DebugMode variable should be enough to tell the difference)

Pros:

Cons:


The next thing I thought of is KeyboardStateView. Experiment shows that EPKL always takes the higher priority over KeyboardStateView, no matter the starting order, with the exception of when EPKL is running under standard privilege and KeyboardStateView is running under administrator privilege. However, this shouldn't be concerning because KeyboardStateView should be started by EPKL and it will follow whatever the privilege EPKL has.

KeyboardStateView license allows redistributing the program as long as no changes are made to its program.

The plan would be to:

  1. Distribute KeyboardStateView with EPKL
  2. #KeyHistory 0
  3. Make changes to _PKL_main.ahk so the calling to KeyHistory would instead open this external program
  4. Optionally show a info message box that KeyboardStateView only displays the remapped input when user clicks on the Key History entry

Pros:


Either would be trivial to implement with minimal changes to the project (The latter seems to be easier). Which one do you think would be easier on casual users?

I personally feel like picking the latter one as it makes less changes to the project, it's easy enough to be implemented in like 5 minutes (And I'm not familiar with this project so less changes implies less potential breakages).

DreymaR commented 1 year ago

Neither tack looks practical to me. Most users don't want that kind of complexity for a concern that most users don't even care about. And god knows setting up EPKL is already too complex for a lot of new users!

If you're advanced enough to navigate something like that, you're also advanced enough to uncomment a line and recompile.

More people do need KeyHistory than you seem to realize. Only yesterday I asked someone at the Colemak Discord to provide KeyHistory info to help solve their problem. It happens with some frequency.

I have made a commit in which I added info to the README as discussed (under Known Issues), and a line in _PKL_Main.ahk .

dfrvfmst commented 1 year ago

You still missed my point. The point is:

I would image most users messing around the program for the first week and then being done with it for years

From your PoV KeyHistory is frequently used by your users. This happens only because those who contacted you were often having issues with EPKL and those who didn't have an issue have no reason to contact you, also known as survivorship bias. If you ask each users you've supported in one year whether they still actively utilize KeyHistory even after their issues were fixed, the answer would probably be a boldly no.

But it's OK. At least now that you've addressed the issue in README so it's now up to the users to decide whether to keep the KeyHistory on (they don't read manual you say? ~then tell them to RTFM~).