Star-pattern-based import authorization #1180
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| @@ -68,7 +68,8 @@ We have re-built a more secure `LocalPythonExecutor` from the ground up. | |||
|
|
|||
| To be precise, this interpreter works by loading the Abstract Syntax Tree (AST) from your Code and executes it operation by operation, making sure to always follow certain rules: | |||
| - By default, imports are disallowed unless they have been explicitly added to an authorization list by the user. | |||
| - Note that some seemingly innocuous packages like `random` can give access to potentially harmful submodules, as in `random._os`. | |||
| - Furthermore, access to submodules is disabled by default, and each must be explicitly authorized in the import list as well, or you can pass for instance `numpy.*` to allow all submodules of `numpy`. | |||
| - Note that some seemingly innocuous packages like `random` can give access to potentially harmful submodules, as in `random._os`.- The total count of elementary operations processed is capped to prevent infinite loops and resource bloating. | |||
There was a problem hiding this comment.
There is a duplicate line I believe, and thanks a lot for crediting me :)
There was a problem hiding this comment.
Thanks for addressing the security issue we have after the merge of of your PR:
Just for context, a previous discussion we had on 31 March (private link): https://huggingface.slack.com/archives/D06G2QXT6LF/p1743402350227549?thread_ts=1743284185.575739&cid=D06G2QXT6LF
@albertvillanova: we could consider alternative intermediate solutions
- Maybe allowing specific patterns like:
additional_authorized_imports=["plotly.*"]?
@aymeric-roucher: Also having to precise
"plotly.*"is a good idea, but you cannot really know in advance which subpackages will be needed, so I think it's not practical for the user
Before diving into the details of the PR, I just wanted to flag something: I believe this approach doesn't fully address the underlying security concern. Specifically, even if a parent package (like plotly, as used in the example) appears safe, it may still contain submodules that are potentially dangerous but not explicitly listed in the DANGEROUS list. Also note that the DANGEROUS list can never be exhaustive.
I do think it's worth putting a helper script somewhere that does "import plotly" and returnd the list of all imported submodules. This of course is easy to game (make inner imports conditional on smolagent being imported, import at a later time, lazy imports, are nefarious examples that come to mind). And does not address packages versions (easy to add a new req to some package and update it after the initial check). But I of course understand that the threat model when using docker is okay with this. Just a reminder: my initial suggestion to use star patterns also included star patterns for the import exclusions. I do find it odd to make it easy to specify includes and harder (no globbing) to specify excludes. I'm not sure anymore if it's best to make the exclude apply before or after. Making it before would allow excluding "everything" then delectively include stuff. PS: consider adding lazy imports to the list of dangerous imports |
Co-authored-by: Albert Villanova del Moral <8515462+albertvillanova@users.noreply.github.com>
…com:huggingface/smolagents into Generalize-star-based—import-authorizations
|
Failing tests are unrelated. |
How should we define authorized imports? Exact match is too strict which is why I made this change. But @thiswillbeyourgithub proposed this good idea: just use a star-like pattern to define authorized imports. This PR implements it.