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

[FR] Add a way to enforce static evaluation of attr: #2901

Open
1 task done
KOLANICH opened this issue Nov 22, 2021 · 7 comments
Open
1 task done

[FR] Add a way to enforce static evaluation of attr: #2901

KOLANICH opened this issue Nov 22, 2021 · 7 comments

Comments

@KOLANICH
Copy link
Contributor

KOLANICH commented Nov 22, 2021

What's the problem this feature will solve?

It is possible to cause code execution in pure setup.cfg + pyproject.toml workflows at wheel build stage.

Describe the solution you'd like

  1. Add an attr options.no_exec = true into setup.cfg which semantics is to disable as many mechanisms of execution of code supplied by package author at the build stage as possible.
  2. Disable importing of modules mentioned in attr for now when it is enabled. Other hardenings can be done in other PRs.

Alternative Solutions

Do it by default for setup.cfg + pyproject.toml packages, it's the main feature of such a way to build packages anyway.

Code of Conduct

  • I agree to follow the PSF Code of Conduct
@KOLANICH KOLANICH added enhancement Needs Triage Issues that need to be evaluated for severity and status. labels Nov 22, 2021
@abravalheri
Copy link
Contributor

abravalheri commented Nov 23, 2021

Isn't it always possible to cause code execution during builds according to PEP 517 by setting backend-path and using an in-tree backend (and possibly wrapping the backend of your choice)?

I suppose the only safe way of not having code execution is to install the pre-compiled wheel...

@jaraco
Copy link
Member

jaraco commented Nov 23, 2021

To clarify, the default behavior of attrs is to load statically, and it's only the fallback behavior that would enable code execution. Acknowledged, a maliciously-crafted module could easily trigger the arbitrary code execution.

Privately, someone asked if this issue was a security vulnerability. Here's my response.

At first blush, I don’t consider this issue a vulnerability. The feature is designed to import the code indicated by the package author, accepting any code execution that happens as a consequence. Setuptools and distutils have had arbitrary code execution as a basic expectation since their inception.

The reporter’s expectation that there should not be any code execution during the build phase has never been guaranteed, so he’s being overly presumptive about the goals of the project.

Reporting this vulnerability would be comparable to reporting a vulnerability against macOS that it’s possible to install applications not from the App Store. Although there are examples of platforms where apps can only be installed from the trusted store (iOS), it’s always been the case in macOS that apps could be side loaded and historically that was the default behavior.

Instead of focusing on a specific missed expectation (attrs), this project would first need to step back and decide if build/install without code execution general is a desirable and possible goal to work toward. Would you agree there's little value in plugging this hole if there are many other larger holes that cannot be plugged?

Avenues of code execution

While not exhaustive, here are some current avenues of code execution available when building a Setuptools-based project:

  • Add a setup.py, which is unconditionally executed if present.
  • (As Anderson suggested) Alter the pyproject.toml to reference local behavior instead of Setuptools.
  • Alter the pyproject.toml to reference a different backend that executes arbitrary code.
  • Include a build dependency that exposes of Setuptools' hooks as entry points and execute the code there (egg_info.writers, distutils.setup_keywords, setuptools.finalize_distribution_options, distutils.commands).
  • Add a build dependency that causes behavior on interpreter startup (e.g. future-fstrings).

If the goal is to prevent arbitrary code execution during build time, limiting the attrs directive is probably the least of these challenges. Some of these are Setuptools-specific and others are inherent in the language or the packaging ecosystem.

Design

What is the broader problem @KOLANICH wishes to solve? Under what conditions is it necessary (and not) to prevent code execution?

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Nov 23, 2021

If the goal is to prevent arbitrary code execution during build time, limiting the attrs directive is probably the least of these challenges.

Yeah, a low-hanging fruit.

What is the broader problem @KOLANICH wishes to solve? Under what conditions is it necessary (and not) to prevent code execution?

  1. I beleive that execution of arbitrary code must be minimized. Execution of arbitrary code is a privilege. Principle of least privilege says that if a privilege is not needed, it should not be granted. It reduces attack surface.

  2. There is nothing in usual pure python (not compiling cexts, not transpiling sources, not preprocessing data, not pickling stuff, not training machine learning models) packages that requires arbitrary code execution. setup.cfg + pyproject.toml only configuration can be utilized for the most of them. If we can harden it enough, we can get packages that can be built without arbitrary code execution. And principle of least privilege says we should do it.

  3. What are the benefits? Lot of them.
    a. We don't trust package author at the stage of building of a package. We can be sure in some threat model that the software involved has no vulnrs he cannot for example take our confidential info like ssh keys and send them to own machine. Or do anything nasty, like removing all the files in home dir. Or use ast.parse + ast.unparse to patch our projects the way to achieve an effect like this: .
    b. building wheels from such packages can be outsourced to third parties;
    c. some people (including me) have habit to install packages from root. It has some benefits. For example I can use my packages to do privileged operations, like modifying package manager config. Also my software becomes awailable systemwide. In default configuration (I may be wrong, I don't use the default configuration for a long time) pip downloads packages from pypi and if no wheels are available it builds them. If I understand right, no measures to deelevate and to sandbox the building process (in security sense) are taken (while they definitely should be taken).
    d. Turing-complete code cannot be reliably sandboxed.

So the roadmap IMHO should be the following:

  1. harden building process using declarative config to exclude uncontrolled package-author-supplied code execution. There must be finite amount of well-known ways to cause it and there must be a reliable and non-resource-consuming way to proove that a package is not capable to trigger code execution while it is being built.
  2. deprecate these ways. Not only in setuptools, but in the all the well-known build backends and frontends and plugins. Always show a warning. While the tools are called interactively, require a user to explicitly aggree to arbitrary code execution before it happens. When are run non-interactively, just assumme that a user has aggreed. In the case of disaggreement the following is to be done:
    a. Alter the pyproject.toml to reference local behavior instead of Setuptools. - stop with an error
    b. Add a setup.py, which is unconditionally executed if present. - whitelist trivial setup.pys doing nothing. In other cases setup.py must be executed explicitly.
    c. d. e. Alter the pyproject.toml to reference a different backend that executes arbitrary code., Include a build dependency that exposes of Setuptools' hooks as entry points and execute the code there (egg_info.writers, distutils.setup_keywords, setuptools.finalize_distribution_options, distutils.commands)., Add a build dependency that causes behavior on interpreter startup (e.g. future-fstrings). - require all the build dependencies to be preinstalled.
    f. introduce an option to ensure sanitization of PYTHONPATH (ignoring setuptools in a local dir, ignoring pth files) for the platform-specific "binary stubs" (I mean ones generated with console_script entry point).
  3. Harden pip. Disable downloading of sdists by default.
  4. Start assumming a user hasn't aggreed in the tools. It is a breaking change, everyone would have to fix either scripts or packages. Fixing packages is preferred.

Would you agree there's little value in plugging this hole if there are many other larger holes that cannot be plugged?

No. A journey of a thousand miles begins with a single step.

@jaraco
Copy link
Member

jaraco commented Nov 23, 2021

You make a compelling argument, which I will summarize as: arbitrary code executing during building is worth solving and Setuptools should aim to limit whatever avenues to arbitrary execution that are inherent in Setuptools' own logic.

If Setuptools wants to take this stance, I'd like to critique the proposed solution. In particular, it creates this scenario:

  • Preferred, safest usage requires specifying two options to achieve one thing (attr: + options.no_exec = true).
  • The discouraged usage is simpler and cleaner (and thus preferable from a UX perspective).
  • The proposed naming presents a double-negative (no_exec = false). Better to express as a positive (e.g. allow_exec).
  • Possibly, the static analysis already serves 99% of use-cases. Perhaps the last 1% can be encouraged to migrate to a usage that supports the static evaluation such that arbitrary code execution can be eliminated altogether.

@KOLANICH My preference would be to deprecate the arbitrary code execution fallback and rely on the behavior introduced in #1753. Would you be willing to take on the implementation?

@jaraco jaraco added help wanted and removed Needs Triage Issues that need to be evaluated for severity and status. labels Nov 23, 2021
@KOLANICH
Copy link
Contributor Author

KOLANICH commented Nov 24, 2021

My preference would be to deprecate the arbitrary code execution fallback and rely on the behavior introduced in #1753.

Would forward compatibility be a problem? I mean would older versions of setuptools throw any error if they meet an option unknown to them (that yousers of newer versions would have to insert in order to avoid breakage), forcing upgrade of setuptools version, which is not always feasible (i.e. a newer version of setuptools can be incompatible with the version of python used and it may be problematic to upgrade python version)?

The discouraged usage is simpler and cleaner (and thus preferable from a UX perspective).

Untill the defaults are swapped. My idea was the following

  1. setuptools analyses configuration. If it is compatible to the recommended workflow, it prints nothing and activates the best settings automatically. The package is already compatible, no action is required.
  2. If it sees incompatibility, it uses the defaults. And prints a warning saying
    • "Hello, <package developer>, your package is incompatible to the great future in which we gonna make package building more secure."
    • "your package relies on the feature that may mean execution of arbitrary code during package building. It is in <line №> line of your setup.cfg and <line №> line of your __init__.py, which is not an expression that can be evaluated statically (ast.literal_eval)"
    • "It is recommended to fix the expression to allow it be evaluated statically"
    • "In future the defaults will be changed. Explicitly set options.allow_exec = true if you have to rely on actual code execution".
    • "In a bit more distant future the possibilities to execute code during package building will require user's explicit consent. It means your package won't be installable via pip from an sdist for the users who haven't opted in. It is recommended to modify your package to avoid mandatory code execution at package build time. It is also recommended to upload wheels to pypi."
  3. When the security check fails, it should print something like "Cannot statically extract an attr. .... You can set allow_exec = true in options section in order to build the package, but it is recommended to fix it. "
  4. After a default pref has been flipped and the check has failed, an error message should also mention thhis fact.

The proposed naming presents a double-negative (no_exec = false). Better to express as a positive (e.g. allow_exec).

I aggree, this is better.

Would you be willing to take on the implementation?

I have no ETA on that. I would be glad if someone else did it.

@kamikredstone
Copy link

Hey! Just wondering if anyone is working on this issue?

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Feb 1, 2022

IDK about other people, but I am currently NOT.

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

No branches or pull requests

4 participants