Re: [PR] validate exception type when deserializing serialized exceptions [airflow]

["naruto-lgtm (via GitHub)" <gi...@apache.org>]

naruto-lgtm commented on code in PR #68662:
URL: https://github.com/apache/airflow/pull/68662#discussion_r3523396518


##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -662,6 +662,12 @@ def deserialize(cls, encoded_var: Any) -> Any:
                 exc_cls = import_string(exc_cls_name)
             else:
                 exc_cls = import_string(f"builtins.{exc_cls_name}")
+            # ``exc_cls_name`` comes from the serialized payload. A tampered blob can name any
+            # importable callable (``os.system``, ``builtins.exec``, ...) which would then be
+            # invoked with attacker-supplied args. Only accept genuine exception types, matching
+            # the import-path guards the timetable/window/wait-policy decoders already apply.
+            if not (isinstance(exc_cls, type) and issubclass(exc_cls, BaseException)):

Review Comment:
   Moot now. Pushed eef021eb, which drops the whole exception branch from BaseSerialization, so there is no import-then-validate ordering left to worry about and nothing legitimate serializes an exception through the general framework anymore.



##########
airflow-core/src/airflow/serialization/serialized_objects.py:
##########
@@ -658,14 +659,18 @@ def deserialize(cls, encoded_var: Any) -> Any:
             args = deser["args"]
             kwargs = deser["kwargs"]
             del deser
+            # ``exc_cls_name`` comes from the serialized payload, so resolve it without handing an
+            # attacker-controlled string to ``import_string`` -- that would execute the named
+            # module's top-level code before any validation. The encode side only ever emits
+            # ``airflow.*`` AirflowException subclasses for AIRFLOW_EXC_SER and builtin
+            # KeyError/AttributeError for BASE_EXC_SER, so reject anything outside those up front,
+            # mirroring the import-path guards in the timetable/window/wait-policy decoders.
             if type_ == DAT.AIRFLOW_EXC_SER:
+                if not exc_cls_name.startswith("airflow."):

Review Comment:
   Gone in eef021eb. Rather than trying to make the prefix safe, I removed the encode and decode branches entirely, so there is no import_string call on a payload name to defend anymore.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org