fix: harden syft-restrict against verifier and obfuscator bypasses by pjwerneck · Pull Request #9436 · OpenMined/PySyft

pjwerneck · 2026-07-06T20:58:57Z

Summary

Closes a series of bypasses in syft-restrict's static verifier and obfuscator, found through
iterative adversarial review. Update documentation.

Changes

Ban additional dynamic-escape/reflection builtins: type, __build_class__, print, and the
dunder-proxy builtins repr/str/ascii/format/bytes.
Fix multiple aliasing bypasses: bare names imported via from X import name, homoglyph identifiers, and
common identifier-aliasing patterns that evaded the call-target checks.
Fix f-string handling: conversion flags (!r/!s/!a), the {x=} debug form, and plain
interpolation (f"{x}") all invoke __format__/__repr__/__str__ with no Call node — all
are now rejected instead of only the conversion-flag forms.
Harden self/cls parameter vetting: reassigning self/cls, reusing them as an unrelated
parameter name, and non-first-parameter/nested-function/lambda cases no longer grant the
self.<name> trust exemption.
Close self.<attr> trust bypasses: tuple-unpack and for-loop assignment targets are now tracked
by the safety table, and local-variable aliasing (including multi-hop copies) is tracked so
tmp = self.fn; tmp(x) is checked the same way self.fn(x) is.
Ban def/class statements whose name shadows a trusted import alias or visible wrapper name
(previously only rebinding via assignment was checked).
Fix the obfuscator to correctly blank f-string literal text under Python 3.12+'s PEP 701
tokenizer (previously left un-obfuscated on 3.12+).
Rewrite docs/disallowed-ast-examples.md as per-construct subheadings instead of a wide table;
add docs/code-layout.md; simplify README.md.
Extract shared AST helpers into astutil.py and refactor verifier.py/obfuscator.py for
readability (central violation-code registry, _SelfAttrTrust helper, split obfuscator passes),
with no behavior change.
Raise on a malformed/inverted private-line range instead of silently verifying nothing in it.

Testing

Added tests/verify/test_bypasses.py with a regression test per bypass class
Added tests/verify/test_ranges.py for the range-validation fix.

Asana task

https://app.asana.com/1/1185126988600652/project/1216249688888494/task/1216266561077080?focus=true

…ases

…or f-string conversions

… scope checks Fix ast.comprehension check relying on unavailable lineno, therefore unreachable code.

…handling and range errors

Copilot

Pull request overview

This PR strengthens syft-restrict’s static verification and obfuscation layers to close multiple identified bypass techniques, adds regression tests for each bypass class, and refreshes the documentation to match the evolved threat model and implementation.

Changes:

Harden verifier rules around call-target resolution, aliasing, self/cls trust, and f-string interpolation to prevent reflection/dynamic-escape bypasses.
Update obfuscation to correctly blank f-string literal text under Python 3.12+ (PEP 701 tokenization), plus refactor shared AST/range utilities into astutil.py.
Add targeted regression tests (bypasses + range validation) and restructure docs/README for clarity.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/syft-restrict/tests/verify/test_whitelist.py	Updates whitelist expectations for f-strings to reflect new interpolation rejection behavior.
packages/syft-restrict/tests/verify/test_ranges.py	Adds regression test ensuring malformed private ranges raise instead of silently passing.
packages/syft-restrict/tests/verify/test_bypasses.py	Adds comprehensive regression coverage for verifier bypass classes (aliasing, reflection, f-strings, `self`/`cls` trust, etc.).
packages/syft-restrict/tests/obfuscate/test_obfuscate.py	Adds a test ensuring f-string literal text is blanked across tokenizer variants (incl. Python 3.12+).
packages/syft-restrict/src/syft_restrict/verifier.py	Major verifier hardening + refactor: central violation codes, stronger call/name checks, self-attr trust tracking, and f-string interpolation rejection.
packages/syft-restrict/src/syft_restrict/runner.py	Switches to shared `astutil` helpers for range normalization and scanning.
packages/syft-restrict/src/syft_restrict/policy.py	Expands banned builtins and improves documentation/comments; minor cleanup.
packages/syft-restrict/src/syft_restrict/obfuscator.py	Refactors passes; adds PEP 701 f-string token handling; uses shared `astutil`.
packages/syft-restrict/src/syft_restrict/astutil.py	New shared helper module for AST and range utilities (scan, dotted paths, range normalization).
packages/syft-restrict/src/syft_restrict/init.py	Updates package-level docs/pointers to new docs structure.
packages/syft-restrict/README.md	Simplifies and reorients README toward usage + docs pointers.
packages/syft-restrict/docs/verify.md	New/expanded explanation of verifier behavior, edge cases, and limitations.
packages/syft-restrict/docs/code-layout.md	New short guide to module responsibilities.
packages/syft-restrict/docs/blacklist.md	New consolidated “what is rejected” doc with violation codes.
packages/syft-restrict/disallowed-ast-examples.md	Removes legacy table-style doc (replaced by new docs structure).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

koenvanderveen · 2026-07-15T10:36:04Z

+| ----------------- | ---------------------------------------------------------- |
+| Definitions       | `def`, `class`, `lambda`, `return`                         |
+| Names & constants | variables, numbers, strings (not f-strings)                |
+| Assignment        | `=`, `+=`, annotated assigns                               |


is this always allowed? also reassigning self for instance?

Assignment is always allowed as a construct, it's not banned in itself, but name and target are checked separately from the assignment. The name must not be reserved, the target must be allowed, and the assignment resets the later call-trust of the target.

I will add a line clarifying that too.

koenvanderveen · 2026-07-15T10:36:35Z

+| Definitions       | `def`, `class`, `lambda`, `return`                         |
+| Names & constants | variables, numbers, strings (not f-strings)                |
+| Assignment        | `=`, `+=`, annotated assigns                               |
+| Containers        | `list`, `tuple`, `dict`, `set`, comprehensions             |


I guess the note here is that they can only nest allowed syntax?

Yes. An allowed outer construct does not grant permission to its inner pieces. Each node is verified independently. I will make that clearer in the document.

koenvanderveen · 2026-07-15T10:38:08Z

+
+---
+
+## Per-file knobs


maybe its me, but I am not familiar with the term "knob"

Good point. I'll change the section title to "Per-file policy configuration", matching the terms used in the rest of the project.

koenvanderveen · 2026-07-15T10:42:05Z

+| Private defs/classes | `Attention`, `helper`                        |
+| Safe builtins        | `list`, `range`                              |
+| `self` / `cls`       | only as the real first parameter of a method |
+


how about the result of x =transpose(y), are we allowed to reassign x?

Yes. Ordinary locals can be reassigned at will. Only reserved names are protected.

However, the verifier still tracks the source to attempt to resolve and reject if the name is used as a callable.

I'll add a line to clarify that too.

I think reassignment is tricky though. Because of control flow we never know whether something actually was reassignment. So instead of tracking the "current" value of something (which we cant), we should track all assignments and check if they are all safe.

Yes, we can't track dynamic branching, but we still block everything at the source. It gets rejected in some way.

For example:

if c: g = safe else: g = unsafe g()

unsafe is visited last, so in theory this case gets rejected with call-unresolved.

But if we flip it:

if c: g = unsafe else: g = safe g()

In this safe is visited last, so in theory it's not rejected, even though it should have.

However, the false negative is not exploitable anyway because defining unsafe with something like a banned name or other unsafe construct would have triggered another error at the origin, so while the assignment tracking theoretically fails in this case, it's inconsequential in practice.

koenvanderveen · 2026-07-15T10:51:55Z

+
+```python
+class Net(nn.Module):              # base must be allow-listed (e.g. flax.linen.Module)
+    def setup(self):                 # often public (data owner can read wiring)


Now that I read this, perhaps this is the weakest part of our setup. We assume that setup acts like a init function, something a variable is assigned here and never reassigned. But that is very jax specific, and how do we even know that setup is always called (if it wouldnt be jax)

That's a matter of correctness to be resolved at runtime, not security. In the static analysis, the self.attr() calls in __call__ only pass if they were assigned a safe target somewhere in the class, or if they were not assigned anywhere and therefore presumed to come from the base class.

Order doesn't matter. The verifier collects all assignments first and requires all of them to be vetted. If another method reassigns them between setup and __call__, that reassignment needs to be vetted too. If setup doesn't run before __call__, it's a runtime AttributeError, not a bypass. It's not the verifier's job to check for that.

We can flip the rule to default-deny any self.attr that wasn't explicitly assigned in the class, so the assumption that undefined attributes come from the base class no longer holds. It's a stricter rule, but breaks access to base class attributes.

yes I was thinking about something similar, or we could let the user pass in certain fields that can be used but not reassigned or something like that. Perhaps out of scope for this PR

koenvanderveen · 2026-07-15T10:57:47Z

+| ---------------------------------------------------------------------------- | ----------------------------------------------------- |
+| Bases that resolve to an allow-listed path (e.g. `nn.Module`)                | `object`, random private bases, non-allow-listed libs |
+| Decorators: `nn.compact`, `jax.jit`, `jax.named_scope`, `flax.linen.compact` | `@property`, `@staticmethod`, arbitrary functions     |
+| Defining `setup`, `__call__`, `__post_init__`                                | `__getattr__`, `__reduce__`, other magic methods      |


do we need post_init?

We already block __init__. If we block __post_init__ too, there's no hook for initializing dataclasses at construction time. I think there's a case for leaving it in, as convenience.

koenvanderveen · 2026-07-15T10:58:17Z

+| Allowed                                                                      | Not allowed                                           |
+| ---------------------------------------------------------------------------- | ----------------------------------------------------- |
+| Bases that resolve to an allow-listed path (e.g. `nn.Module`)                | `object`, random private bases, non-allow-listed libs |
+| Decorators: `nn.compact`, `jax.jit`, `jax.named_scope`, `flax.linen.compact` | `@property`, `@staticmethod`, arbitrary functions     |


I think in the end we are not using any of these anymore. perhaps we should just disallow them?

Yes. I'll just ban the decorator node entirely, since those were the only decorators allowed.

Anyone who wants decorator behavior should make an explicit call instead. I'll document that.

koenvanderveen · 2026-07-15T10:59:46Z

+
+```python
+# library path (imports public, use private)
+import jax.numpy as jnp


only if allowlisted

koenvanderveen · 2026-07-15T11:00:29Z

@@ -0,0 +1,352 @@
+# How verification works


this is high quality now! It helps me reason over the logic and find edge cases we may not have covered. Great job!

koenvanderveen · 2026-07-15T12:32:55Z

+
+
+class Block(nn.Module):
+    cfg: dict


I didnt think about this yet, is this allowed? is it secure?

It's secure, keeping the documented limitations in mind. Malicious public wrappers still depend on human review.

However, it's still a gap because the verifier doesn't catch potential abuse of a declared field like that. It's not a security issue, since you still need malicious public code to abuse it, but it's inconsistent when compared to how we handle similar implicit assignments.

I will close this gap too. self.cfg() in that case shouldn't be trusted.

koenvanderveen · 2026-07-15T12:42:54Z

+_MARKER_RE = re.compile(r"^#\s*syft-restrict:\s*(obfuscate|hide)(?:-(start|end))?\s*$")
+
+
+def parse_markers(source: str) -> tuple[list[tuple[int, int]], list[tuple[int, int]]]:


some comments here would help me or otherwise split it up in functions

I modified the structure to use a stack to handle the start/end and nesting and simplified the implementation. I think it's easier to understand now.

Rename the method to a clearer enforcement verb and extract named booleans (name_reserved/name_rebound) in both branches for readability.

koenvanderveen · 2026-07-15T14:57:19Z

+            return
+
+        # methods are not bare-call targets and therefore not shadowed; shared names like `setup`` are fine
+        if isinstance(node, ast.FunctionDef) and id(node) in self.scan.method_ids:


perhaps nitpick and not dangerous, but it looks like you might be able to define method x on class y twice because we are not checking that

Yes. It's not exploitable in any way that matters for the security model, but they might be confusing in obfuscated code. I'll fix it.

koenvanderveen · 2026-07-15T15:03:47Z


-# The only dunder/hook methods a model class may *define* (approach-B §3.1 #6).
+# The only dunder/hook methods a model class may *define* (docs/verify.md#allow_functions--paths-callable-by-name).
 ALLOWED_DUNDER_DEFS: frozenset[str] = frozenset({"__call__", "setup", "__post_init__"})


I think we can remove post init

koenvanderveen · 2026-07-15T15:04:22Z

-# access (`block.w`), the same attribute-access-hook class banned in §3.1 #6, so default-deny
+# access (`block.w`), the same attribute-access-hook class banned for dunder defs, so default-deny
 # rejects it. Pure inference needs only setup/__call__.
 ALLOWED_DECORATORS: frozenset[str] = frozenset(


we are not using these, remove

…d add corresponding test

koenvanderveen · 2026-07-16T12:34:25Z

+            root = path.split(".")[0]
            if root in ("self", "cls"):
+                # a single self.<name>/cls.<name> read is fine; a deeper chain is not
+                if self_attr_name(node) is not None:


I think we may want to consider making this more strict. I think its only fine to call that if we defined it ourselves, so we know the getattr doesnt create side effects

That will prevent using attrs inherited from the base class. It's the same question I asked above.

#9436 (comment)

koenvanderveen · 2026-07-16T13:18:35Z

+            attr = self_attr_name(value.value)
+        if attr is not None:
+            return self._self_attr.is_safe(attr, self._enclosing_class())
+        return _all_leaves_safe(value, self._is_safe_local_leaf)


I dont have an answer to this question yet, but do we need any asignment that not an attribute?

No, we don't strictly need it. It's a trade-off between convenience and attack surface. If we remove that, we lose any legitimate alias-then-call pattern, like this:

block = self.layers[0] return block(x)

Because block(x) is trusted only because block was tracked as safe.

Without local assignments, you'd have to do:

return self.layers[0](x)

I don't think the trade-off is worth it. The reduced attack surface here is minimal. The local aliased call must still resolve to a safe callable.

koenvanderveen · 2026-07-16T13:24:19Z

+            else:
+                safe.pop(t.id, None)
+
+    def _is_safe_local_source(self, value: ast.AST) -> bool:


we are calling this function from _track_safe_local after we already skipped anything that is not an ast.Name, like ast.Attribute, so in that context value here can only be an ast.Name. However, we are calling functions that sound like they should be applied to an attribute. (self_attr_name), this is confusing me a bit

No, there's target and value. The Name filter is for the target name, the variable being assigned to. The value is the one being assigned and need to be tracked as safe.

I think it could be confusing because we have to iterate possible chained assignments, like x = y = value. I will add some comments clarifying that.

github-actions Bot added the bugfix label Jul 6, 2026

pjwerneck added 10 commits July 6, 2026 18:00

Fix common aliased identifier bypass case

2d2f548

Fix self.* call vetting.

1268fbe

Improve call vetting to include checks for bare names imported as ali…

4daaa7f

…ases

Add banned names to denylist: type, __build_class__, and print

2cfe2cf

Enhance policy to include additional banned names and better checks f…

1502f4a

…or f-string conversions

Enhance self/cls parameter vetting and add tests for reassignment and…

888f9b3

… scope checks Fix ast.comprehension check relying on unavailable lineno, therefore unreachable code.

Add regression tests for bypasses and update f-string handling.

4eeca96

Simplify README.md and update docs/

c69587a

Enhance range normalization and verification; add tests for f-string …

5cc5267

…handling and range errors

Add alias tracking for self attributes to prevent trust bypasses

fc54f9d

pjwerneck force-pushed the pjwerneck/fix-restrict-bypasses branch from 0213862 to fc54f9d Compare July 6, 2026 21:01

pre-commit fixes

085f0a0

pjwerneck requested review from Copilot and koenvanderveen July 6, 2026 21:08

Copilot started reviewing on behalf of pjwerneck July 6, 2026 21:29 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

Comment thread packages/syft-restrict/src/syft_restrict/verifier.py Outdated

Comment thread packages/syft-restrict/docs/verify.md Outdated

Comment thread packages/syft-restrict/docs/blacklist.md Outdated

Apply suggestions from code review

8c99d39

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>