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

Add API Keys for fzf --listen #3374

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Conversation

boazy
Copy link
Contributor

@boazy boazy commented Jul 17, 2023

Summary

Provide optional hardened security for the FZF server. If the FZF_API_KEY environment variable is set, FZF would refuse any request that does not contain the specified API key. The API key can be any string, and should be the specified using the x-api-key header.

Background

fzf --listen currently accepts all requests, without any kind of authentication. This exposes the user to several threats:

  1. A malicious script on the internet can bypass CORS and execute arbitrary commands on a running fzf instance by employing a DNS rebinding attack.
  2. Non-browser applications have no concept of CORS, but often allow arbitrary POST requests to arbitrary URLs which can be manipulated by remote code.
  3. Any non-containerized process running on the same machine, can force the fzf process to execute arbitrary shell commands, even if the other process is running as a different, non-privileged, user. This creates an opportunity for privilege escalation on multi-user machines.

See #3368 for more details.

Alternative Approaches

  1. We can protect against the DNS rebinding vulnerability by blocking any request that doesn't specify localhost in the Host header. But this approach would only protect fzf against DNS rebinding, and it could also interfering with legitimate use cases where you want to bind another domain to the loopback interface.

  2. The API key could be made mandatory, and randomly auto-generated if not explicitly specified. This approach would break all current usage of the --listen flag.

  3. The API key could be specified with a new command line argument (e.g. --listen-key or --listen-api-key). I believe using an environment variable is cleaner, since it naturally propagates to child processes.

  4. A different header name could be used. There are no standard headers for API authorization, but x-api-key is the closest thing we have to a popular choice for API keys (i.e. single-credential, directly-supplied secret keys for calling an API).

  5. We could use the standard Authorization header, but it offers no standard scheme for API keys. The bearer scheme is meant for tokens (especially OAuth 2 tokens), while the Basic scheme requires two values (username and password) which makes it more complicated to use.

Points of note

  1. Sending an invalid API key results in a 401 error.
  2. Comparing API keys is done using subtle.ConstantTimeCompare() to prevent timing attacks. This requires converting the API key to a byte array, but it's well worth the price.
  3. fzf is not returning any Content-Type header for error messages. I think it should return Content-Type: text/plain, but this is probably not this pull request's job to do that.

@junegunn
Copy link
Owner

Thanks! We need to mention this variable on the man page. Also can you add a test case (in test_go.rb)? Let me know if you need any assistance.

@boazy
Copy link
Contributor Author

boazy commented Jul 19, 2023

Thanks! We need to mention this variable on the man page. Also can you add a test case (in test_go.rb)? Let me know if you need any assistance.

Added both. One test in test_go.rb fails on my computer, but it also fails on master branch, so I don't think that's related:

Finished in 366.023389s, 0.5956 runs/s, 6.0870 assertions/s.

  1) Failure:
TestGoFZF#test_preview_window_follow [/fzf/test/test_go.rb:2093]:
Expected "  24                                                       │ 990                                                   ⠇ │" to include "/1003".

Additionally, I'm not very familiar with the test harness, so I don't know what's the best way to test that sending a command with the wrong API key did not affect fzf without introducing flakiness. For now I'm only checking the HTTP response header with wrong API keys.

@junegunn
Copy link
Owner

Thanks, let me finish this up.

@junegunn junegunn merged commit c0435fd into junegunn:master Jul 20, 2023
5 checks passed
@junegunn
Copy link
Owner

Merged, thanks!

@boazy
Copy link
Contributor Author

boazy commented Jul 21, 2023

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants