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

Audit usage of server-side code generation from strings #84816

Open
legrego opened this issue Dec 2, 2020 · 5 comments
Open

Audit usage of server-side code generation from strings #84816

legrego opened this issue Dec 2, 2020 · 5 comments
Labels
enhancement New value added to drive a business result Feature:Hardening Harding of Kibana from a security perspective Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@legrego
Copy link
Member

legrego commented Dec 2, 2020

This is essentially a server-side version of #36311.

Code generation from strings is a powerful tool, but with great power comes...a significant increase in attack surface.

Node.js has a --disallow-code-generation-from-strings command line argument that allows us to block this feature altogether. Before we can even consider such a drastic measure, we should audit all places that Kibana directly and indirectly requires this functionality.

Ideally, this would run as part of CI for the functional UI tests and fail CI if we find new usages which aren't already known. This will let us work toward removing all existing usages without continuing to add new usages which later have to be addressed. We could also potentially use this same approach as part of dev mode to catch violations which aren't covered by the functional ui tests.

@legrego legrego added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! enhancement New value added to drive a business result Feature:Hardening Harding of Kibana from a security perspective labels Dec 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Aug 5, 2021
@legrego legrego removed EnableJiraSync loe:small Small Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Aug 18, 2022
@legrego
Copy link
Member Author

legrego commented Mar 28, 2023

@watson I know you had made some progress on this a while back. If you still have the branch available, would you be able to reference it here so we don't lose track of your work?

@watson
Copy link
Contributor

watson commented May 16, 2023

@legrego 👍 Will pack together what I have and push it to a branch ASAP

@watson
Copy link
Contributor

watson commented May 25, 2023

This is the current status:

Initially I thought it was possible to simply overwrite the global eval / Function with a custom eval that would allow hooks to be added. I developed an npm package for that called codegen-audit. However, in the initial testing it turned out that this approach didn't work as overwriting eval had side effects. So this package was never published to npm, and still just lives in my repo: https://github.com/watson/codegen-audit (there's also a wip branch in the codegen-audit repo, which contain code that I was in the middle of writing when the project was paused: https://github.com/watson/codegen-audit/tree/wip)

Instead we need to add official hooks into the eval / Function code inside Node.js core. I collaborated on a PR to do that here: nodejs/node#35157

However, that PR doesn't work as is (which is also why it was never merged) because it requires some changes to V8 itself. I never got around to making these changes in V8, which is where the work is currently stranded. I believe the details of what needs to be done is documented in the above PR.

The TL;DR is that the V8 hooks into eval / Function doesn't expect JavaScript code to be called from within the hooks - only C/C++ code. So when that happens the memory is in an unknown state and the Node.js processes crashes. All that's needed is for the V8 code to be augmented with a few lines of code so it correctly handles the memory issue. So this should be fairly simple to do.

Finally I have a WIP commit/branch of Kibana which I pushed to my fork of Kibana which ties all this together: You can see the source here: main...watson:kibana:audit-code-gen

@watson
Copy link
Contributor

watson commented Jun 2, 2023

I took a stab at recreating the original Node.js PR and to work on where in V8 the mentioned changes needed to go. I've created a new draft PR for the purpose of getting help with the V8 changes as I'm not that familiar with the V8 codebase: nodejs/node#48295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Hardening Harding of Kibana from a security perspective Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

No branches or pull requests

3 participants