`pickle.Unpickler`: add hook to intercept `REDUCE` calls

[mandolaerik]

Feature or enhancement

Proposal:

I want to pickle and unpickle data that may validly contain types.FunctionType instances, but I want to prevent them from being used in REDUCE opcodes, to protect me from RCEs like REDUCE os.system('firefox https://www.youtube.com/watch?v=dQw4w9WgXcQ').

The traditional conservative approach is to use an allowlist, e.g.:

class SafeUnpickler(pickle.Unpickler):
    def find_class(self, module, name):
        obj = super().find_class(module, name)
        if not known_to_be_safe(obj):
            raise pickle.UnpicklingError(f"unsupported symbol '{module}.{name}'")
        return obj

However, find_class is applied when a GLOBAL/STACK_GLOBAL opcode is executed, before the interpreter knows whether the symbol will be used as a value or passed to REDUCE. I want to permit pickling of any function, including os.system; I just don't want to allow passing it to REDUCE.

So, what I really want is some sort of hook called at REDUCE time. One concrete proposal:

Ts = TypeVarTuple('Ts')
R  = TypeVar('R')

def reconstruct(self, constructor: Callable[[*Ts], R], args: tuple[*Ts]) -> R:
    # default impl; overrides may raise UnpicklingError
    return constructor(*args)

The safe idiom would be:

known_safe_classes = (SomeClass, OtherClass)
def reconstruct(self, constructor: Callable[[*Ts], R], args: tuple[*Ts]) -> R:
    if not isinstance(constructor, type):
        # prevent e.g. os.system RCE
        raise UnpicklingError(...)
    if not issubclass(constructor, known_safe_classes):
        # prevent e.g. subprocess.Popen RCE
        raise UnpicklingError(...)
    return constructor(*args)

Dismissed alternatives:

• I can solve my problem by prohibiting functions in find_class, and instead providing a custom serialization for FunctionType through persistent_id/persistent_load, but this is unfortunate since I need to re-implement serialization from scratch of a type that is already supported natively.
• I can solve my problem by subclassing the undocumented pure-python pickle._Unpickler instead of pickle.Unpickler, and overriding the undocumented load_reduce (which cPickle's Unpickler does not honor). This is undocumented and slow.

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

[picnixz]

pickle is not meant to be secure (i.e. only unpickle data you trust) and this is documented, and highlighted. If you need to validate the pickled code (more precisely), you should rewrite your own unpickler. This feature exceeds the scope of the module.

cc @serhiy-storchaka

[picnixz]

Note that while we provide some examples of "secure" unpickling, this is a "all-or-nothing" kind of switch and we say:

Therefore if security is a concern, you may want to consider alternatives such as the marshalling API in xmlrpc.client or third-party solutions.

[serhiy-storchaka]

Pickle is not safe by default, but there are ways to restrict it and make more secure. @mandolaerik talk about them. We recently discovered that this was not enough, and some manufactured pickles can be unsafe even if you override find_class. That was fixed, but we still do not rush to declare pickle safe.

Overriding find_class and white-listing what classes and functions can be loaded is the way to restrict what code can be executed. The drawback is that it blocks also classes and functions which are not called. There is perhaps workaround with persistent_id/persistent_load, I did not try it. So, the idea of adding yet one hook to control what can be called has a basis. I will investigate what was done in this direction in recent years.