kaxil commented on code in PR #68511:
URL: https://github.com/apache/airflow/pull/68511#discussion_r3706717767
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -242,6 +242,54 @@ def _decode_priority_weight_strategy(var: str) ->
PriorityWeightStrategy:
return priority_weight_strategy_class()
+# Builtin exceptions the serializer emits as ``BASE_EXC_SER``. Only these are
ever
+# serialized (see the encode side), so deserialization resolves the stored
name against
+# this fixed map instead of importing it -- ``builtins.eval`` /
``builtins.exec`` and any
+# other name are rejected without importing anything.
+_DESERIALIZABLE_BUILTIN_EXCEPTIONS: dict[str, type[BaseException]] = {
+ "KeyError": KeyError,
+ "AttributeError": AttributeError,
+}
+
+
+def _iter_subclasses(cls: type) -> Iterator[type]:
+ """Yield every (transitive) subclass of ``cls``."""
+ for sub in cls.__subclasses__():
+ yield sub
+ yield from _iter_subclasses(sub)
+
+
+@cache
+def _serializable_airflow_exceptions() -> dict[str, type[AirflowException]]:
+ """
+ Map ``"<module>.<name>" -> AirflowException subclass``, used to resolve
``AIRFLOW_EXC_SER`` nodes.
+
+ Built once, from the in-memory ``AirflowException`` subclass tree (never
from the
+ attacker-controlled stored name), and never rebuilt -- a name absent from
it is rejected, not
+ imported. ``airflow.exceptions`` is imported by this module, so every
built-in Airflow exception
+ is registered by the time this is first called; exceptions defined later
are not added.
+ """
+ return {
+ f"{cls.__module__}.{cls.__name__}": cls
+ for cls in (AirflowException, *_iter_subclasses(AirflowException))
+ }
+
+
+def _resolve_airflow_exception(exc_cls_name: str) -> type[AirflowException]:
+ """
+ Resolve a serialized ``AirflowException`` class name to the loaded class,
without importing it.
+
+ The name is matched against the once-built ``AirflowException`` subclass
map, so deserializing a
+ stored DAG never runs the top-level code of a module named in the blob. A
name that is not a
+ registered ``AirflowException`` subclass -- e.g. an attacker's
``subprocess.check_output`` -- is
+ rejected rather than imported.
+ """
+ exc_cls = _serializable_airflow_exceptions().get(exc_cls_name)
+ if exc_cls is None:
+ raise DeserializationError(f"Refusing to deserialize unknown exception
class {exc_cls_name!r}")
Review Comment:
Separate from the staleness point on the map above: keying on
`cls.__module__` is stricter than the `import_string` it replaces, because
`import_string` followed re-export aliases and a `__module__` key can't.
Concrete case: `AirflowException`, `AirflowNotFoundException`,
`AirflowRescheduleException`, `ParamValidationError` and four others lived in
`airflow.exceptions` through 3.1.x and only moved to `airflow.sdk.exceptions`
in 3.2.0. A blob written by 3.0/3.1 therefore stores
`airflow.exceptions.AirflowException`. I checked against current main: that
name still deserializes today (the re-export keeps it importable), and with
this patch the map only holds `airflow.sdk.exceptions.AirflowException`, so it
raises. Rebuilding the cache never adds the old key, so this survives the fix
proposed above.
Would resolving against `sys.modules` work instead of a prebuilt map?
`sys.modules.get(module_part)` (never imports), `getattr`, then
`issubclass(obj, AirflowException)`. I tried it: it resolves both the
`airflow.exceptions.*` and `airflow.sdk.exceptions.*` spellings, and still
rejects `subprocess.check_output`, `builtins.eval` and `os.system` even with
`subprocess` already imported. Same no-import property, no alias problem, and
no cache to invalidate.
##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -242,6 +242,54 @@ def _decode_priority_weight_strategy(var: str) ->
PriorityWeightStrategy:
return priority_weight_strategy_class()
+# Builtin exceptions the serializer emits as ``BASE_EXC_SER``. Only these are
ever
+# serialized (see the encode side), so deserialization resolves the stored
name against
Review Comment:
This says only `KeyError` / `AttributeError` are ever serialized, but the
encode branch writes `var.__class__.__name__`, not a constrained name. With
`class MyKeyError(KeyError)`, `BaseSerialization.serialize(MyKeyError("x"))`
emits `base_exc_ser` with `exc_cls_name='MyKeyError'`, outside this map.
It failed before too (`import_string("builtins.MyKeyError")`), so nothing
regresses, but the new message calls it a "disallowed builtin exception", which
reads like a security block for what is really an unsupported type. The two
tests below hand-build the encoded blob instead of going through `serialize()`,
so nothing pins the map to the encode branch if that tuple ever grows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
