🔒 Fix unsafe JSON deserialization in Parametric models#54
🔒 Fix unsafe JSON deserialization in Parametric models#54derrynknife wants to merge 4 commits intomasterfrom
Conversation
Co-authored-by: derrynknife <18457108+derrynknife@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34c0b742c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "MixtureModel", | ||
| "NeverOccurs", | ||
| "Normal", | ||
| "Parametric", |
There was a problem hiding this comment.
Exclude non-fitters from allowed distribution whitelist
ALLOWED_DISTRIBUTIONS currently includes symbols like MixtureModel, NeverOccurs, and Parametric that are not valid Parametric fitters. In from_dict, these names pass the new whitelist check and then flow into out = cls(dist, ...), where Parametric.__init__ expects dist.k/dist.bounds/dist.param_map; this raises an internal AttributeError instead of the intended ValueError for unrecognized input. A crafted JSON payload using one of these entries can still crash deserialization, so the new validation is not fully enforced for untrusted input.
Useful? React with 👍 / 👎.
…nd fix CI Co-authored-by: derrynknife <18457108+derrynknife@users.noreply.github.com>
Co-authored-by: derrynknife <18457108+derrynknife@users.noreply.github.com>
Co-authored-by: derrynknife <18457108+derrynknife@users.noreply.github.com>
🎯 What: The
⚠️ Risk: An attacker could exploit this by providing a crafted JSON file containing an unexpected "distribution" value (e.g.,
from_jsonandfrom_dictmethods insurpyval/univariate/parametric/parametric.pyallowed instantiating arbitrary classes and functions from thesurpyvalmodule viagetattrbased on user-provided JSON/dict input.__builtins__or other internal modules/functions). This unsafe deserialization could lead to unexpected behavior, denial of service, or potentially arbitrary code execution depending on the available targets.🛡️ Solution: Added a hardcoded whitelist (
ALLOWED_DISTRIBUTIONS) containing only valid parametric distributions. Thefrom_dictmethod now strictly checks the requested distribution against this whitelist and raises aValueErrorif the distribution is not recognized, explicitly blocking unsafe deserialization attempts.PR created automatically by Jules for task 6319190020759174654 started by @derrynknife