Skip to content

Conversation

@maikypedia
Copy link
Contributor

This pull request adds sinks for unsafe deserialization covering pandas, numpy and joblib.

maikypedia and others added 2 commits July 27, 2023 23:43
Co-authored-by: Jorge <[email protected]>
Co-authored-by: Jorge <[email protected]>
@maikypedia maikypedia marked this pull request as ready for review July 27, 2023 21:55
@maikypedia maikypedia requested a review from a team as a code owner July 27, 2023 21:55
@ghsecuritylab ghsecuritylab marked this pull request as draft July 28, 2023 16:02
@ghsecuritylab ghsecuritylab marked this pull request as ready for review September 5, 2023 11:04
RasmusWL
RasmusWL previously approved these changes Sep 25, 2023
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this high quality submission @maikypedia 👍

I've made a few minor changes, but overall everything looked good 🙌

(I'll just take a look at new results before merging 👍)

RasmusWL
RasmusWL previously approved these changes Sep 25, 2023
@RasmusWL
Copy link
Member

I looked over some of the new sinks we get from this (results) -- we get quite a few cases where it's clearly a path being passed to one of the new sinks, such as in the example below.

np.load(os.path.join(graph_work_path, "train_data.npy"), allow_pickle=True)

In those cases, it becomes tricky. If user can control the data being loaded, it could still lead to arbitrary code execution through deserialization. It's a little unclear whether to treat this under the path-injection query or not, since the normal rules for a safe path doesn't necessarily apply here (path restricted to specific folder) 🤔

let me be clear: It's not something I would expect you to have solved @maikypedia, just an interesting observation we (CodeQL team) can consider how to approach.

@RasmusWL RasmusWL merged commit 2d947a4 into github:main Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants