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

Security considerations #1361

Open
lmeyerov opened this issue Mar 5, 2017 · 10 comments
Open

Security considerations #1361

lmeyerov opened this issue Mar 5, 2017 · 10 comments
Labels

Comments

@lmeyerov
Copy link

lmeyerov commented Mar 5, 2017

We're considering allowing user-defined transforms specified in jq, but it has been difficult for us to ascertain how safe it is from code injection & privilege escalation, by design & in practice. As far as I can tell, while jq supports some user-defined functions, they are largely sandboxed. The exception is the module imports, which would need some thing for restricting.

Can anyone clarify the security model? May help to link the answer as part of the README.

@nicowilliams
Copy link
Contributor

Excellent question. So far we've held back from adding the sorts of builtins that might make it easier to break out of the jq sandbox.

Real or potential issues to be concerned with:

  • CPU consumption -- jq is Turing complete (but single-threaded)
  • Row-hammer attacks -- there's no reason to think that they are not possible to code in jq
  • Potential security bugs in jq -- we've not yet applied AFL, but others have and we know that there exist JSON parser bugs
  • Potential security bugs in bison where the jq program could source itself can exploit them
  • It is possible to read arbitrary files down from the current directory using include and import directives -- this is a big deal if you don't trust the jq code you're running
  • We do want to add more I/O builtins in the future -- you can't count on jq being sandboxed forever

Now, it is my intention to require the use of a command-line switch to enable future I/O builtins that could allow reading or writing files, or executing external programs. But still, there's no way to guarantee that's so.

I think it's probably desirable to add a command-line option to disallow the main program from including files or modules, or more generally a sandbox option.

@nicowilliams
Copy link
Contributor

nicowilliams commented Mar 5, 2017

See also #1215, #660, and others.

@lmeyerov
Copy link
Author

lmeyerov commented Mar 6, 2017

Ah, excellent. Thanks!

Out of those, the include and import statements seem concerning under most threat models (including ours.) +1 for sandboxed mode flag for this and future features :) Items like DOS via resource abuse are real concerns, and I agree with others that we'd normally go to to the OS etc. for handling that.

For the interim, would it be sufficient for us to do a blacklist for "include" and "import", and freeze to the current version?

@nicowilliams
Copy link
Contributor

@lmeyerov Yes. You might even edit src/parser.y and remove the include/import functionality.

Thinking about how to disable include/import... we could just shove that into struct locfile, or add another parse param for this, and the in the rules for include/import simply error out if they are disabled.

@nicowilliams
Copy link
Contributor

There's a lot of potential sandboxing options:

  • allow importing modules, but not data
  • allow importing modules, but not data, and don't allow modules to import data either
  • don't allow importing anything

Eventually we'll have fairly capable I/O builtins. We might have a need for many sandboxing options.

Obviously, @lmeyerov, the simplest sandbox will be to chroot jq...

@nicowilliams
Copy link
Contributor

Actually, that may well be the best thing we could do: have a "chroot" option that disallows I/O to/from files outside the sandbox. On Unix-like systems jq would then literally call chroot(2) and be done, but on Windows it might attempt to implement a chroot-like sandbox for the jq program in user-land.

@lmeyerov
Copy link
Author

lmeyerov commented Mar 12, 2017

It may help, as simple policies, to consider two of the most basic security properties: data confidentiality & integrity. I'd expect "--sandbox" to provide both. Integrity doesn't seem to be an issue, but confidentiality would seem to require disallowing both module & data import, as who knows what's on the system. (I'm assuming the attacker can control the data & transform parameters, but not the rest, and there is a chance of dangerous files already on the system for use as part of an escalation.)

Making "--sandbox" target a model like that, and supporting it going forwards, would provide a simple policy for both use & development.

FWIW, we're using JQ via https://github.com/sanack/node-jq, so patching it with --sandbox would be easy. Rather not have them fork which jq..

@raijinsetsu
Copy link

This thread is now almost 4 years old, but seems like it should be raised again.

We are very much interested in a "--sandbox" option. Our use case is that a client can supply a JQ filter allow with an RestAPI call so that the server only returns a subset of the data. Note: GraphQL and similar are not a good fit for us at this time.

I am loath to re-implement the syntax, create a library to sanitize the input (which would be just as difficult as reimplementing jq), or use a custom built version which does not contain the insecure builtins and variables. Is there anything we can do to contribute to a "--sandbox" option or an alternative build with those insecurities taken out, suitable for embedding?

@jemc
Copy link

jemc commented Apr 12, 2024

If a PR was submitted to add a --sandbox option that prevented the use of include/import (and would be expected to also prevent any future I/O primitives that could break the sandbox), would that PR be accepted (assuming any review comments about the details of the implementation were properly addressed)?

Any notes from the maintainers about acceptance criteria for somebody who wants to do that PR?

jemc added a commit to jemc/jq that referenced this issue Apr 12, 2024
Some use cases for `jq` may involve accepting untrusted input.
See discussion in jqlang#1361 for some security considerations that
may be relevant for those use cases.

This commit adds a `--sandbox` flag which is meant to mitigate
one category of security issue with untrusted input:
features of jq which are meant to let the jq filter access
files/data other than the direct input data given to the CLI.

Specifically, the new `--sandbox` flag blocks the implicit
loading of `$HOME/.jq`, and also blocks the use of
`import` and `include` for loading other `jq` files.

If other features are added to `jq` in the future which allow
for reading files/data as part of the filter syntax, it is
intended that the `--sandbox` flag would also gate access to those.
@jemc
Copy link

jemc commented Apr 12, 2024

Okay, this was a simple enough change that I decided to go ahead and create a pull request to add the --sandbox flag.

Hopefully that can mitigate at least this category of risk (including/importing arbitrary files/data from the file system), if the PR is accepted.

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

No branches or pull requests

5 participants