Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent long strings as int inputs #4480

Merged
merged 3 commits into from Sep 5, 2022
Merged

prevent long strings as int inputs #4480

merged 3 commits into from Sep 5, 2022

Conversation

samuelcolvin
Copy link
Member

@samuelcolvin samuelcolvin commented Sep 5, 2022

see #1477 and in turn, python/cpython#95778 this check should be unnecessary once patch releases are out for 3.7, 3.8, 3.9 and 3.10 but better to check here until then.

See this twitter thread the python patch releases should be out in the next few days, but I know how slow people are to upgrade these things, so I think better to fix here.

NOTICE:

This does not full protect user from the DOS risk since the standard library JSON implementation
(and other std lib modules like xml) use int() and are called before this, the best workaround is to

  1. update to the latest patch release of python when it's released
  2. (until then) use a different JSON library like ujson

Related issue number

fix #1477

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change

@samuelcolvin samuelcolvin mentioned this pull request Sep 5, 2022
6 tasks
@samuelcolvin
Copy link
Member Author

should now pass, please review.

@samuelcolvin
Copy link
Member Author

@pydantic/pydantic-maintainers do you think we should backport this to v1.9 and v1.8?

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I think we don't need backporting this because this is not Pydantic issue and the proper fix will be out in Python patch releases. Also, 1.10.x is mostly backward compatible release and users can have the fix by upgrading to 1.10.x

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I think we don't need backporting this because this is not Pydantic issue and the proper fix will be out in Python patch releases. Also, 1.10.x is mostly backward compatible release and users can have the fix by upgrading to 1.10.x

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I think we don't need backporting this because this is not Pydantic issue and the proper fix will be out in Python patch releases. Also, 1.10.x is mostly backward compatible release and users can have the fix by upgrading to 1.10.x

@hramezani
Copy link
Member

sorry. I think it was my internet problem. I approved the PR 3 times 🤦‍♂️

@samuelcolvin
Copy link
Member Author

No problem. Good to see you're sure 😄.

@samuelcolvin samuelcolvin merged commit eccd85e into main Sep 5, 2022
@samuelcolvin samuelcolvin deleted the long-int-fix branch September 5, 2022 11:35
# NOTICE: this does not fully protect user from the DOS risk since the standard library JSON implementation
# (and other std lib modules like xml) use `int()` and are likely called before this, the best workaround is to
# 1. update to the latest patch release of python once released, 2. use a different JSON library like ujson
if isinstance(v, (str, bytes, bytearray)) and len(v) > max_str_int:
Copy link
Contributor

@cmyui cmyui Sep 5, 2022

Choose a reason for hiding this comment

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

maybe overkill, but should this include memoryview as well?

from pydantic import BaseModel


class A(BaseModel):
    x: int

A(x=memoryview(b"1" * 5000))

this seems like a much less common use case, but perhaps could still be used as a dos in some cases

$ python -m timeit -s 'x=b"1" * 100_000' 'int(memoryview(x))'
5 loops, best of 5: 59.6 msec per loop

i can make a pr to address this

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.

potential DOS risk with pydantic - fix pending
4 participants