Skip to content

Commit 59f4cf8

Browse files
Fiddle-Config Teamcopybara-github
authored andcommitted
Add allow_imports option to DEFINE_fiddle_config etc., default False
This change fixes an RCE security vulnerability that occurs if the Fiddle flags come from an untrusted or less-trusted source. This change exposes the `allow_imports` option in `DEFINE_fiddle_config`, and defaults it to `False` (previous behavior was implicitly `True`); as well as changing the default value of this option in the underlying `FiddleFlag` class from `True` to `False`. This prevents Fiddle from implicitly loading modules and executing code when dotted names are passed, such as `--config=config:foo.bar()`. **If this change broke you**: the easiest fix is to add `allow_imports=True` to your `DEFINE_fiddle_config`. This will revert your code to the previous behavior. However, if possible we recommend that you instead place all the functions you might need into one module, and set `default_module` to that. PiperOrigin-RevId: 879723838
1 parent 24c1d2e commit 59f4cf8

File tree

1 file changed

+30
-2
lines changed

1 file changed

+30
-2
lines changed

fiddle/_src/absl_flags/flags.py

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ def __init__(
118118
self,
119119
*args,
120120
default_module: Optional[types.ModuleType] = None,
121-
allow_imports: bool = True,
121+
allow_imports: bool = False,
122122
pyref_policy: Optional[serialization.PyrefPolicy] = None,
123123
**kwargs,
124124
):
@@ -135,6 +135,14 @@ def __init__(
135135
# This will save all arguments that have passed, when unparse() is called
136136
# this will also be reset to empty.
137137
self._all_arguments = []
138+
139+
if default_module is None and not allow_imports:
140+
raise ValueError(
141+
"A Fiddle config flag must either specify a default_module to load "
142+
"configs from, or set allow_imports=True to allow them to be loaded "
143+
"from any module."
144+
)
145+
138146
super().__init__(*args, **kwargs)
139147

140148
def _apply_fiddler(self, cfg: config.Buildable, expression: str):
@@ -239,7 +247,21 @@ def value(self):
239247
or command == "config_file"
240248
or command == "config_str"
241249
):
242-
self._parse_config(command, expression)
250+
try:
251+
self._parse_config(command, expression)
252+
except ValueError as e:
253+
if (
254+
"Could not resolve reference to named function" in str(e)
255+
and not self.allow_imports
256+
):
257+
raise ValueError(
258+
"If this is a new error in previously-functional code, pass "
259+
"allow_imports=True to DEFINE_fiddle_config(). This allows "
260+
"anyone with control of the fiddle command-line flags to "
261+
"execute arbitrary code; only do this if such imports are "
262+
"trusted."
263+
) from e
264+
raise
243265

244266
elif command == "set":
245267
utils.set_value(self._value, expression)
@@ -267,6 +289,7 @@ def DEFINE_fiddle_config( # pylint: disable=invalid-name
267289
pyref_policy: Optional[serialization.PyrefPolicy] = None,
268290
flag_values: flags.FlagValues = flags.FLAGS,
269291
required: bool = False,
292+
allow_imports: bool = False,
270293
) -> flags.FlagHolder[Any]:
271294
r"""Declare and define a fiddle command line flag object.
272295
@@ -325,6 +348,10 @@ def main(argv) -> None:
325348
registered. This should almost never need to be overridden.
326349
required: bool, is this a required flag. This must be used as a keyword
327350
argument.
351+
allow_imports: If true, then fully qualified dotted names may be used to
352+
specify configs or fiddlers that should be automatically imported. This
353+
grants arbitrary code execution to anyone who can specify the fiddle
354+
command-line flags.
328355
329356
Returns:
330357
A handle to defined flag.
@@ -338,6 +365,7 @@ def main(argv) -> None:
338365
parser=flags.ArgumentParser(),
339366
serializer=FiddleFlagSerializer(pyref_policy=pyref_policy),
340367
help_string=help_string,
368+
allow_imports=allow_imports,
341369
),
342370
flag_values=flag_values,
343371
required=required,

0 commit comments

Comments
 (0)