Harden the FFI parser cache against code execution (restrict unpickling) - #562
Open
Nexory wants to merge 2 commits into
Open
Harden the FFI parser cache against code execution (restrict unpickling)#562Nexory wants to merge 2 commits into
Nexory wants to merge 2 commits into
Conversation
import pyvex reads a cffi FFI parser cache from a world-predictable path in the shared temp directory (gettempdir()/pyvex_ffi_parser_cache.<user>.<hash>, where the username is public and the hash is a fixed per-version constant) and deserializes it with a bare pickle.loads. On a shared multi-user host a co-tenant can plant a poisoned cache, and pickle.loads then executes arbitrary code as the victim on the next import (CWE-502 / CWE-377). This is local, defense-in-depth hardening, the same class recently addressed in keras and pdfminer.six. The cache legitimately contains only cffi.model type classes, so this replaces pickle.loads with a RestrictedUnpickler that permits only those classes and rejects everything else. No behaviour change on a legitimate cache. Adds tests/test_ffi_cache_filter.py: an arbitrary-callable gadget and a cffi.model helper function are both rejected, and a legitimate cache still loads.
for more information, see https://pre-commit.ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
import pyvex(and thereforeimport angr) reads a cffi FFI parser cache from a world-predictable path in the shared temp directory and deserializes it with a barepickle.loads(pyvex/native.py):The path is
gettempdir()/pyvex_ffi_parser_cache.<getuser()>.<md5(ffi_str)>: the username is public and the hash is a fixed per-version constant. On a shared multi-user host a co-tenant can plant a poisoned cache file, and becausepickle.loadsexecutes arbitrary reduce callables, the nextimport pyvexruns attacker code as the victim (CWE-502 / CWE-377).This is local, defense-in-depth hardening rather than a remote vulnerability, and it is the same deserialization-cache class recently hardened in keras and pdfminer.six.
Fix
The cache legitimately contains only
cffi.modeltype classes, so a bare unpickle is unnecessary. This replacespickle.loadswith aRestrictedUnpicklerwhosefind_classallows onlycffi.modelclasses and rejects everything else, so a poisoned cache raisesUnpicklingErrorinstead of executing. No behaviour change on a legitimate cache.Test
tests/test_ffi_cache_filter.py: an arbitrary-callable gadget is rejected; acffi.modelhelper function is also rejected (the allow-list is class-only, so it cannot be used as a gadget); and a legitimatecffi.modelcache still loads. The full test suite passes (67).