search

pqrs-org / Karabiner-Elements

Karabiner-Elements is a powerful utility for keyboard customization on macOS Sierra (10.12) or later.
https://pqrs.org/osx/karabiner/
The Unlicense
18.74k stars 835 forks source link

Big security issue - binary being shipped inside this repo, it is not built from source, aka keylogger #1684

Closed kzaher closed 5 years ago

kzaher commented 5 years ago

Please help me to confirm is this true.

  1. You are shipping a binary inside this repo that is the kernel extension being installed to intercept the keyboard pressed keys (and this logs all keys somebody is pressing).

https://github.com/tekezo/Karabiner-Elements/blob/master/src/vendor/Karabiner-VirtualHIDDevice/dist/org.pqrs.driver.Karabiner.VirtualHIDDevice.v061000.kext/Contents/MacOS/VirtualHIDDevice

  1. The default installation instructions will make it appear like the user is building the kernel extension when in fact it will fail with signing error and your binary extension will be used as a fallback.
bash ./scripts/codesign.sh dist
code sign org.pqrs.driver.Karabiner.VirtualHIDDevice.v060800.kext
8ECD43BA902B40380BD84C4512385E6C5EB3F160: no identity found
org.pqrs.driver.Karabiner.VirtualHIDDevice.v060800.kext: code object is not signed at all
In architecture: x86_64
bash ./scripts/setpermissions.sh dist
  1. When user is installing the result binary Karabiner-Elements-VERSION.dmg by default your attached binary (not build from the source will be used), because only you have the sigining key!!!
stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kristjan72 commented 5 years ago

How is an issue like this closed without being addressed?

kzaher commented 5 years ago

Wtf?

kzaher commented 5 years ago

How is this closed?

nspassov commented 5 years ago

The binary is probably from this project? https://github.com/tekezo/Karabiner-VirtualHIDDevice

How did you determine that keys are logged? Were you able to detect anything being sent over the internet? I have Little Snitch installed on my machine.

tekezo commented 5 years ago

Of course, you can build Karabiner-VirtualHIDDevice from source code with replacing CODESIGN_IDENTITY with yours.

However, macOS requires a special signing key for the kernel extensions. Without it, you cannot load the kernel extension with SIP. https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/KernelExtensions/KernelExtensions.html

Thus, normally you have to upgrade the Developer ID Certificate to be able to sign the kext. This is why Karabiner-Elements embed the signed kernel extension binary.

kzaher commented 5 years ago

Hi @tekezo ,

First of all I would say that I appreciate all of your giant effort to open source this tool. I really loved to use it, and it was almost indispensable to me.

This is also the reason why this is so frustrating. Because of this I can't use this project anymore. The risk is simply too high.

There are two ways of distributing the code:

What went wrong: When a user is trying to build the binary from source files, because they care about security, you are presenting the built from sources progress to the user. Even though the process fails because of signing issues, you ignore the entire build process and just use your home built and signed kext from somewhere and build the result dmg file successfully. This gives the user an impression that everything went smooth, when the expected action actually didn't happen.

Why is this problematic:

However, macOS requires a special signing key for the kernel extensions. Without it, you cannot load the kernel extension with SIP.

I understand why you need to sign the kext. That doesn't mean that you should attach a signed kext binary and commit it as a part of source code and use it as a fallback.

Thus, normally you have to upgrade the Developer ID Certificate to be able to sign the kext. This is why Karabiner-Elements embed the signed kernel extension binary.

Like I said, I think that you are aware that Apple won't be giving those kext signing keys to developers for personal use, the risk for Apple would be too high.

Why am I so frustrated with this:

How to remedy this so that everyone is happy:

What not to do:

tekezo commented 5 years ago

@kzaher I have the same opinion of yours.

But is your proposal possible?

Create reproducible kext build.

You can do use this repository. https://github.com/tekezo/Karabiner-VirtualHIDDevice

Create a kext signature for every release of Xcode that is supported (just supporting the last version of Xcode is more then sufficient for probably 99% of people).

How we can do it? Generally, the file contents (== sha1 hash) of Contents/MacOS/VirtualHIDDevice is differ by each build.

kzaher commented 5 years ago

@tekezo

But is your proposal possible?

I'm not a compiler expert but it seems like it should be possible. Debian is trying to make their builds reproducible and they have way more code than this project.

I've tried to investigate a bit what is happening and there is a couple of sources of non determinism in your setup.

.o files are deterministic as far as I can tell. The problem is in the linker step. If you play around with linker you can get builds with stable shasum on a single machine. I can not vouch that you would get the same result on multiple machines with these changes, or that you would need to standardize something additional, but I think I got stable linker step also on my computer. (As far as I can tell generating .o files is already stable).

I was able to get the reproducible builds on the same machine in this way. Just run your normal build and then extract all o files:

into a separate directory that all users have write access to. Like /var/tmp/Repro. Changing the path will change the shasum. Generate a VirtualHIDDevice.LinkFileList and put it in the /var/tmp/Repro directory with the following content:

/var/tmp/Repro/VirtualHIDKeyboard.o
/var/tmp/Repro/VirtualHIDPointing.o
/var/tmp/Repro/VirtualHIDRoot.o
/var/tmp/Repro/UserClient.o
/var/tmp/Repro/VirtualHIDDevice_info.o

Xcode path is /Application/Xcode.app.

Running this command:

rm /var/tmp/Result/VirtualHIDDevice || true && touch /var/tmp/Result/VirtualHIDDevice && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -filelist /var/tmp/Repro/VirtualHIDDevice.LinkFileList  -mmacosx-version-min=10.12  -stdlib=libc++ -Xlinker -kext -nostdlib -lkmodc++ -lkmod -lcc_kext  -Xlinker -o /var/tmp/Result/VirtualHIDDevice && shasum /var/tmp/Result/VirtualHIDDevice

gives me a constant shasum for /var/tmp/Result/VirtualHIDDevice on a single machine.

How did I discover this? You can modify src/Makefile in the following way:

all:
    ../scripts/setversion.sh
    rm -rf build_now
    xcodebuild -scheme VirtualHIDDevice -configuration $(CONFIGURATION) -derivedDataPath build_locally_here build

to get a local derived data inside the git repo. Remove all of the .gitignore files you have. You have at least two. One in the root and in src, and use git diff to investigate the differences between builds. Read the full linker command and work backwards from there. Like I said, I think you only need to fix the linker step, and I've already helped you as much as my time allowed me to.

I hope this information will get you closer to some reasonable solution, but I'm not a compiler expert and you'll need to probably find one.

I would really want this to work and for you to find some solution because I really love your product, but even if there isn't an obvious solution I think you should stop doing what you are doing now and stop doing fallback on compiled signed binary.

tekezo commented 5 years ago

@kzaher Thank you for investigation! I think shasum /var/tmp/Result/VirtualHIDDevice guarantees the not signed binary was produced the source code. But is it ensure the distributed binary is safe? (Could you tell me the detail of relation?)

kzaher commented 5 years ago

I think that kext contains two files. Binary and plist. I was trying to explain how it might be possible to create a signed kext without you needing to distribute a binary.

If the identical binary can be built by the user (per build environment), then you can just attach a correct signature you've made when you have been signing the binary locally.

As far as this link explains, seems like only content matters.

https://developer.apple.com/library/archive/documentation/Security/Conceptual/CodeSigningGuide/AboutCS/AboutCS.html

Wait, are you using Karabiner-VirtualHIDDevice just to emit the keystrokes? It seems like you can detect then through IOHIDDeviceRegisterInputReportCallback.

tekezo commented 5 years ago

Wait, are you using Karabiner-VirtualHIDDevice just to emit the keystrokes?

Yes. Please read the documentation about the data flow. https://github.com/tekezo/Karabiner-Elements/blob/master/docs/DEVELOPMENT.md#core-processes

Thus, Karabiner-VirtualHIDDevice does not observe any input events. But, as you know, if the kernel extension is malformed, the kext do various attacks. So we should prove the distributed kernel extension is clean.

About proving the distributed binary

As my investigation, codesign modifies the binary and there are no reliable way to strip the sign. (== We cannot get the original unsigned binary from signed binary.)

There are some tools that strips the signature, but these tools are not enough to revert the codesign modification. (The file hash is not same signed binary and signature stripped binary.) https://stackoverflow.com/questions/7500381/bug-in-codesign-remove-signature-feature

So, we have to take a different approach. I guess we can do by the following steps.

  1. Build binary and do codesign in Travis CI. (I'll pass my certificate to Travis CI in order to be able to sign with valid signature.)
  2. Dump the sha values of dist files in order to everyone confirm the valid sha value in the job log. https://travis-ci.org/tekezo/Karabiner-VirtualHIDDevice

The build process is complete automated and there is not a chance to interfere by malicious human. (There is an assumption that we can trust Travis CI.)

kzaher commented 5 years ago

@tekezo

About proving the distributed binary

Again, you shouldn't be distributing binary in the source code and fallback on it when the user fails to codesign:

Again, why:

The build process is complete automated and there is not a chance to interfere by malicious human.

I think you are missing the point completely. It doesn't matter how to binary is generated. I was never saying how to prove your binary is correct, but how to let users build their own binary (reproducible build) for which you can just provide the signature).

As my investigation, codesign modifies the binary and there are no reliable way to strip the sign. (== We cannot get the original unsigned binary from signed binary.)

I have no idea how did you investigate this. The goal isn't stripping the signature, but replacing the signature with your own for the same binary. Seems there are two parts of mach-o you need to change https://github.com/steakknife/unsign/blob/master/unsign.c

I'm really not convinced at the moment that you actually need a kext to send keystrokes. https://stackoverflow.com/questions/8027146/sending-keystroke-events-to-osx

tekezo commented 5 years ago

how to let users build their own binary (reproducible build) for which you can just provide the signature).

How do I prove the signature which I provided is correct?

I think it's a macOS SIP issue and third party including me cannot provide any way to avoid the distributing signed kext binary (== including signed kext binary into the source code).

Disabling SIP (enable --without kext) is a solution, but it is highly not recommended.

https://apple.stackexchange.com/questions/208478/how-do-i-disable-system-integrity-protection-sip-aka-rootless-on-macos-os-x

I'm really not convinced at the moment that you actually need a kext to send keystrokes.

macOS provides several way to post input events,but it is not enough except virtual input device.

https://github.com/tekezo/Karabiner-Elements/blob/master/docs/DEVELOPMENT.md#the-difference-of-event-posting-methods

tekezo commented 5 years ago

How do I prove the signature which I provided is correct?

Additional description:

If I have malice, I'll put the right build signature but replace the signed binary with malformed binary as you said.

like somebody joining your github organization and publishing

Karabiner-Elements requires the root privillege to install and run. So, if the distributed binary is malformed, the attacker can anything without the malformed kext. To avoid the possibility, I'm restricting Karabiner-Elements core repository collaborator (current, only me)

kzaher commented 5 years ago

There shouldn’t be any binary inside the repo, so you or anyone else can’t put a malformed binary.

Even if you think that only you have access, that is only your opinion. One can only prove something has been compromised after the fact.

Why are you using the root priviledge kext anyway? You have the accessibility API to post keyboard events. I’ve tested it and it seems to work. https://stackoverflow.com/questions/8027146/sending-keystroke-events-to-osx

tekezo commented 5 years ago

Hmm, how do you believe the distributed binary is built from the source code in other projects such as VirtualBox? (If you also doubt other projects, it is consistent..., but if there are some difference, please tell me.)

Why are you using the root priviledge kext anyway?

CGEventPost does not support some keys as far as I investigated. https://github.com/tekezo/Karabiner-Elements/blob/master/docs/DEVELOPMENT.md#cgeventpost

kzaher commented 5 years ago

Hmm, how do you believe the distributed binary is built from the source code in other projects such as VirtualBox? (If you also doubt other projects, it is consistent..., but if there are some difference, please tell me.)

Well I guess it's game theory. I think you are already well aware of these reasons.

CGEventPost does not support some keys as far as I investigated.

Why can't you just fallback to CGEventPost if the kext is not present?

Why do you insist on distributing privately built binary in your source code? This is a huge red flag.

If the user can't sign the build, it should fail and give the user message he needs to get a signing certificate or use a prebuilt binary from some location.

tekezo commented 5 years ago

Why do you insist on distributing privately built binary in your source code?

It's for users who want to modify Karabiner-Elements.

For me, the built binary is not reasonable since I can sign with valid certificate. As you said, the binary should be removed from repository.

But I assume other people do not get kext certificate and there is not a guarantee user can get the certificate from Apple. Other people can build Karabiner-Elements from source code without any certification except kext. So, I'm adopting the current source tree to keep being able to modify Karabiner-Elements by anyone.

Why can't you just fallback to CGEventPost if the kext is not present?

We can do it but I don't have enough time to maintain the fallback code.

kzaher commented 5 years ago

Other people can build Karabiner-Elements from source code without any certification except kext.

If you want to be able to allow maintainers to use your built kext, just remove the kext binary from the source code and attach it to release page as binary. They can just copy the binary themselves and put it to correct location if they want to do it.

This is transparent and right way IMHO with no nasty surprises like I've had.

We can do it but I don't have enough time to maintain the fallback code.

Is it possible for you to maybe just hack some code that does uses CGEventPost and post a commit or point me where I can add CGEventPost calls as a fallback so I don't need the kext extension? I really love this project and I can maintain my own version of Karabiner-Elements without the kext. This is necessary for me to be able to use this project in commercial environment. At the end of the day it doesn't really matter what I think or who I trust, but my employers security team also.

tekezo commented 5 years ago

Is it possible for you to maybe just hack some code that does uses CGEventPost and post a commit or point me

I think you can do it by modify key_event_dispatcher.hpp.

https://github.com/tekezo/Karabiner-Elements/blob/bd8c7af4f078746581d30a534fee1c8db581871a/src/share/manipulator/manipulators/post_event_to_virtual_devices/key_event_dispatcher.hpp#L11-L104

In the above code, convert usage_page to cg_key_code and post it.

Old Karabiner-Elements has CGEvent support. You can copy the code which convert key_code to cg_key_code.

https://github.com/tekezo/Karabiner-Elements/blob/ca1ff1eb4a3ce4b856d63677f517c3cdfc4c6b50/src/share/types.hpp#L404-L597

You can convert usage_page to key_code by make_key_code. https://github.com/tekezo/Karabiner-Elements/blob/bd8c7af4f078746581d30a534fee1c8db581871a/src/share/types.hpp#L424

And there are some kext checks which you disable them.

https://github.com/tekezo/Karabiner-Elements/blob/bd8c7af4f078746581d30a534fee1c8db581871a/src/core/grabber/src/main.cpp#L50-L72

https://github.com/tekezo/Karabiner-Elements/blob/bd8c7af4f078746581d30a534fee1c8db581871a/src/core/grabber/include/device_grabber.hpp#L542-L546

tekezo commented 5 years ago

They can just copy the binary themselves and put it to correct location if they want to do it.

This way is not strongify security because the distributed binary has not any proof as we discussed in above. I think your surprises was caused by the poor documentation of Karabiner-Elements about the build process with pre-built binary. So, I should write a clear explanation about it.

I'll update the how to build the article soon. https://github.com/tekezo/Karabiner-Elements/blob/master/README.md#how-to-build

kzaher commented 5 years ago

I think you can do it by modify key_event_dispatcher.hpp.

Thanks a lot. I'll try to make it work without the kext.

This way is not strongify security because the distributed binary has not any proof as we discussed in above.

I was surprised because I was not building it from source which exists in the repository, but a different binary was slipped in. If you don't delete the binary from the repository you are still doing the wrong thing IMHO because users might run make without reading your instructions.

I'm not sure why do you persist on distributing a built binary in your repository. The correct action is to download the binary from some other prebuilt place. In this way there can be no misunderstanding.

You have ~30ish contributors and probably 20-30k users. IMHO it would be more important to inform the users what they are actually doing because that might be not allowed in a lot of companies and they could get in a lot of trouble with the current build process.

tekezo commented 5 years ago

I updated the document. https://github.com/tekezo/Karabiner-Elements/#how-to-build

I also updated Makefile to avoid building a package without the document. https://github.com/tekezo/Karabiner-Elements/blob/master/Makefile

it would be more important to inform the users what they are actually doing

I think so too. I maintain the code and the docs as necessary.

The correct action is to download the binary from some other prebuilt place.

In that case, Karabiner-VirtualHIDDevice binary will be downloaded from https://pqrs.org or https://github.com, I think it's no essential difference maintain the binary into the source tree. (And manual download causes other problems such as version mismatch, wrong file placing, etc.)