Skip to content

Disable debug console by default and enable it with a new module #160

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

Closed
wants to merge 1 commit into from
Closed

Disable debug console by default and enable it with a new module #160

wants to merge 1 commit into from

Conversation

ntietz
Copy link
Contributor

@ntietz ntietz commented Oct 12, 2016

As I wrote on #159 , having this on by default is a security risk, especially for OS X users. This fix works on my machine and appears to disable it. I'm not familiar enough with the firmware to know if there is more risk lurking elsewhere in the firmware, but this at least removes one obvious attack users could suffer from.

To enable the console, one simply changes their build script to have the following variable: DebugModule="debugconsole"

With DebugModule="full", the debug console will be disabled.

…ondary module users can use (called debugconsole) which enables it.
@haata
Copy link
Member

haata commented Oct 12, 2016

Hmm. I'm not sure what the best course of action is for this.

But I don't really want to flat out disable the console (which has a lot of valid uses, and is one of the only ways right now I can collect support data).

There are two ways keys can be logged:

  1. Enabled outputDebug. This will enable the USB output bitmask, which must be compared with the USB HID descriptor and then cross-referenced with the OS keyboard layout in order to determine which keys were pressed. The HID descriptor is generally rather tricky to get at (requires some sort of root access on Linux and Windows).
    This is likely the easiest way to write a key-logger. Perhaps a better way would be to disable usb keyboard packet output when outputDebug is enabled by default. Then have a flag to enable it for development/testing like I do for the reload command.
  2. Reading the output of matrixDebug. This is a bit trickier, it requires some of the same work of the previous, but less work on the HID descriptor. First you have to read in the scancode pressed using matrixDebug. Then you have to lookup the scancode using macroList. The attacker would have to use macroShow T on each of the possible triggers, then build a table for all possible actions with that scancode. Next with the possible trigger actions, lookup with macroShow R which will give you the USB Code before packetizing (this means the attacker wouldn't have to read the USB HID descriptors to understand the meaning).
    Also possible, but a huge pain as it's keyboard + layout dependent. Even the same layout may be ordered differently, so nothing can be assumed.
    Again, I'd deal with this the same way and disable USB output when enabled by default. And leave a flag for this in development mode.

This is where I'd put the code to drop the packets.
https://github.com/kiibohd/controller/blob/master/Output/pjrcUSB/arm/usb_keyboard.c#L217
https://github.com/kiibohd/controller/blob/master/Output/pjrcUSB/arm/usb_keyboard.c#L256

Have a define like this one:
https://github.com/kiibohd/controller/blob/master/Output/pjrcUSB/capabilities.kll#L30

Then add some sort of variable, like this one to check/set the mode:
https://github.com/kiibohd/controller/blob/master/Output/pjrcUSB/output_com.h#L96

Thoughts?

@ntietz
Copy link
Contributor Author

ntietz commented Oct 12, 2016

I see where you're coming from (and thank you for everything you do for the community and all the support you provide).

I like the approach you suggest here, I'll hopefully update the PR in the next couple of days. It's a good start.

In general, I think having the console enabled by default (or having user-input to it read by default) is always going to pose a security risk. On OS X it's exposed to all users, and on Linux the suggested udev rules (as far as I read them) would also expose them to all users. Having any user input read in the console opens us to the possibility of a buffer overflow, or other sneaky attacks that someone more clever than I may create. You're right that it's a balancing act, so I'm going to go with this suggested resolution for now and maybe eventually I can think of something that addresses both sides. Something that would be pretty ideal, in my mind, would be a setting you can change without recompiling that would just fully enable/disable the console -- is there an approach for that that you know of?

@haata
Copy link
Member

haata commented Oct 12, 2016

:D

As far as runtime switches. This gets dangerous, because the attacker can do the same thing.

What I've done for flash mode is hardened the code to a point where it's very difficult to make it happen unless it's a physical keypress (the general assumption, is physical access means all security goes out the window).

Now, there is one possible way. Is to use USB configuration switches. These require root access on all platforms iirc.
http://www.beyondlogic.org/usbnutshell/usb5.shtml
The two possibilities I see are multiple usb configurations or bAlternateSetting
I'm not sure how compatible all the OSs would be with this, and testing would be a nightmare imo. Because you also need to worry about BIOSs and bootloaders (they all handle USB slightly differently).

I am working on an alternate API interface to the keyboard (to work with the upcoming client-side configurator/driver). One wrinkle is that I need the CLI in order to run manufacturing tests, so I don't think I could disable it for shipping keyboards.

Perhaps, in addition to my earlier suggested changes, we could offer a "security hardened" mode in the configurator/build option. Which disables the CLI USB interface (or just makes it output only).
Going a step further, it could also disable all reflash modes except pressing the physical reset button on the back of the keyboard.

Now I'm rambling, but perhaps a security access key might be a better way "authenticate" access on the keyboard. i.e. generate some sort of security key that has to be input before you get access that's generated when building the firmware image. Would be tricky for keyboards shipped in manufacturing, but is easy once the layout is flashed.

@ntietz
Copy link
Contributor Author

ntietz commented Oct 14, 2016

Great points all around. I like the idea of making a "security hardened" mode, as well as having authentication as an option on the debug console in general.

I'll work on the security hardening first, so I'm going to close this PR and open a new one once I have a first drop of that. I want to be careful in my approach, so it might take me some time to get something out.

@haasn
Copy link

haasn commented May 21, 2017

A possible solution to the security issues that doesn't require tricky authentication code (which could itself be exploited) is to make it toggleable via hardware (e.g. dip switch, or special keybinding on the keyboard layout itself).

Presumably, somebody with physical access to the keyboard who is able to press buttons on it will also be capable of installing a hardware keylogger, reflashing your keyboard, or other nasty stuff; so I think this assumption of physical access = trusted is fair.

@haata
Copy link
Member

haata commented May 21, 2017

Yeah, a toggle mechanism might be cool. Unfortunately, you'd lose the benefit of the debug console, if for example your keyboard firmware stops accepting keypresses. There are some NKRO/6KRO bugs right now that require you to either unplug/replug or send the restart command to the cli (they are OS/BIOS hand-off related, so they are tricky to reproduce).

Dip switches are a pain because they require GPIOs and physical space somewhere on the keyboard.

@TheOtherDave
Copy link

TheOtherDave commented May 21, 2017 via email

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

Successfully merging this pull request may close these issues.

4 participants