Skip to content

Conversation

@Shivam7-1
Copy link

In this PR addresses there could be potential code injection vulnerability in current file in PR changes ensuring proper handling of invalid UTF-8 sequences.

@ghost
Copy link

ghost commented Dec 30, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Dec 30, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@rruuaanng
Copy link
Contributor

Usually a PR should correspond to an open issue (by the way, your title should be prefixed with gh-<issue>, and add a NEWS.).

Copy link
Contributor

Choose a reason for hiding this comment

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

Since as the PR body says, we should add a test to trigger it and ensure that the change fixes this issue.

@rruuaanng
Copy link
Contributor

cc @picnixz

@picnixz
Copy link
Member

picnixz commented Jan 1, 2025

Fuzzing is not my area of expertise but here are some thoughts:

We are fuzzing PyLong_FromUnicodeObject so if our input is incorrect (namely it is not UTF-8 decodable), we just bail (what matters is that we properly detected the error). Note that PyUnicode_FromStringAndSize uses a "strict" error handler and not a "replace" one. We only want to detect decoding errors before passing it to PyLong_FromUnicodeObject because that's what we're interested in.

What I'm actually wondering is whether we should call PyErr_Clear() if we're not matching a ValueError.

cc @gpshead @alex

@alex
Copy link
Member

alex commented Jan 1, 2025

I don't understand what the motivation for this PR is. Can you please try explaining it in more detail? What code injection vulnerability do you believe exists?

@picnixz picnixz added the pending The issue will be closed if no feedback is provided label Jan 1, 2025
@gpshead
Copy link
Member

gpshead commented Jan 1, 2025

Closing as there doesn't seem to be any point to this PR? We actually want the unicode error to be caught in order to skip the int(s) fuzz testing of the API using that value. There is no code injection.

This "module" is for fuzz testing purposes only.

It is expected to only be used within a contained environment being fed procedurally generated arbitrary inputs to uncover issues.

If you believe that PyUnicode_FromStringAndSize or PyLong_FromUnicodeObject allows for actual code injection via untrusted inputs, please use https://github.com/python/cpython/security to discuss details of that.

@gpshead gpshead closed this Jan 1, 2025
@AA-Turner AA-Turner removed the pending The issue will be closed if no feedback is provided label Apr 6, 2025
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.

6 participants