Harden protocol against pickle RCE with a restricted unpickler#63
Open
LeonarddeR wants to merge 4 commits into
Open
Harden protocol against pickle RCE with a restricted unpickler#63LeonarddeR wants to merge 4 commits into
LeonarddeR wants to merge 4 commits into
Conversation
pickle.loads on payloads received over the DVC named pipe is an arbitrary code execution primitive: a compromised or malicious peer can smuggle a __reduce__ referencing os.system, builtins.eval, subprocess.*, etc. Replace the naked pickle.loads in RemoteProtocolHandler._unpickle with a pickle.Unpickler subclass whose find_class only resolves an allowlist of classes that legitimately cross the wire (driver settings, voice/parameter info, gesture maps, RDAccess's own BrailleInputGesture, and NVDA's speech.commands hierarchy via a dynamic subclass check), rejecting and logging everything else. This is the short-term mitigation ahead of the planned pickle removal (nvaccess/nvda#19745). Also update tests/_stubs.py with stub modules for the newly allowlisted classes, and rework test_misc.py's invalidate-cache test to probe with an allowlisted class instead of an arbitrary AutoPropertyObject subclass, which the new allowlist now correctly rejects. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The test now verifies that error-level log records not only exist but also contain both the module and function name of the refused global, making rejections diagnosable. Uses platform-aware module name lookup since os.system resolves to different modules on different platforms (nt on Windows, posix on Unix). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extract RestrictedUnpickler and its lazily-built allowlist out of the protocol package __init__ into addon/lib/protocol/_restrictedUnpickling.py, exposed via restrictedLoads(). Keeps the security-sensitive deserialization logic self-contained and easier to audit; behaviour is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
xgettext refused to scan the file due to a raw ellipsis char in the module docstring, breaking the pot build in CI. Replace with ASCII "etc.". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
RDAccess uses
pickleas the wire format on the DVC named pipe (_pickle/_unpickleinaddon/lib/protocol/__init__.py).pickle.loadson peer-controlled bytes is an arbitrary-code-execution primitive, and the pipe crosses the RDP trust boundary in both directions (client NVDA ↔ server session). Any local process able to create anRdPipe_NVDA-*pipe can also feed the client crafted pickles. This is the same concern that motivates NVDA core deprecating pickle (nvaccess/nvda#19745, milestone 2027.1) — but network-facing here, so it bites harder.Change
Short-term mitigation that keeps the wire format and stays compatible with existing peers:
_unpicklenow routes through aRestrictedUnpicklerwhosefind_classresolves only an allowlist._pickleand the wire framing are unchanged.Allowlisted globals (keyed by the imported class object's runtime
(__module__, __qualname__), not hardcoded strings):autoSettingsUtils.driverSetting.{DriverSetting, BooleanDriverSetting, NumericDriverSetting}autoSettingsUtils.utils.StringParameterInfo(+ compat alias for the olddriverHandler.StringParameterInfomodule)synthDriverHandler.VoiceInfoinputCore.GlobalGestureMapcollections.OrderedDictlib.protocol.braille.BrailleInputGesturespeech.commands: allowed iff the resolved attribute is a class defined in that module andissubclass(obj, SpeechCommand)— so future NVDA command classes work without an add-on update, while anything else in the module is refused.Every rejection logs the offending
(module, name)and raisespickle.UnpicklingError(rejections run inside swallowed_bgExecutorfutures, so silent failures would be undiagnosable).This blocks the generic RCE gadgets (
os.system,builtins.eval, arbitrary-class__reduce__). Residual risk (accepted): pickle can still instantiate allowlisted classes with attacker state and nest builtins (memory-amplification DoS is inherent to pickle). Long-term fix — removing pickle from the wire entirely (JSON + type registry, negotiated via the existingRD_ACCESS_VERSIONhandshake) — is left for a future major version.Tests
New
tests/test_restrictedUnpickling.py:os.system/builtins.eval__reduce__gadgets, a non-SpeechCommandname inspeech.commands, an arbitrary unknown class; plus a test that the rejection is logged and names the offending global.frozensetof command classes,DriverSettinglist,OrderedDictofVoiceInfo/StringParameterInfo,GlobalGestureMap,BrailleInputGesture, builtins dict.Full suite 102/102; ruff + ty clean.
Not covered here
Manual end-to-end over a live RDP/Citrix session (speech, settings sync, braille I/O, beeps, wave files) — every unpickle site is unit-tested for (de)serialization, but the live pipe path should be validated on a real session before release.
🤖 Generated with Claude Code