20 ms·
Prevent DoS by large int-str conversions
- machina_ex_deus 4y agoThis is way too low, I've used RSA keys in base 10 with half the size of this string. It corresponds to only 14,000 bit numbers, there are 8192 bit keys. I'm pretty sure this will break some CTF challenges. The limit should be in the millions at the very least.
- munch117 4y agoIt does seem very low. However, you shouldn't be passing million-digit numbers around as (decimal) text. Even if you're not at risk of DOS attacks, there's still the issue that it's very, very slow: $ python3 -m timeit -s "s='1'*1000000" "i=int(s)" 1 loop, best of 5: 5.77 sec per loop A ValueError alerting you to that fact could be considered a service. Contrast and compare: $ python3 -m timeit -s "s='1'*1000000" "i=int(s,16)" 200 loops, best of 5: 1.45 msec per loop
- adgjlsfhk1 4y agopython being slow isn't news. that's not a reason for an error.
- nomel 4y ago> However, you shouldn't be passing million-digit numbers around as (decimal) text This is about numbers that are thousands of digits, not millions. Regardless, why not? What's the alternative that supports easy exchange? If you stick it in some hexified representation, you still have to parse text, and put it into some non-machine-native number container. It's going to be slow no matter what.
- munch117 4y agoNo, it's not going to be slow no matter what. Didn't you see my example? The hexadecimal non-machine-native textual representation was 4000 times faster than the decimal ditto. On a number that was much larger, I might add. Hex number parsing is linear time.
- blibble 4y agoyou can convert hex into binary directly without any multiplications
- sidewndr46 4y agoIf you're doing cryptography in Python, performance is most definitely not a concern.
- munch117 4y agoSure it is. It's a concern that's addressed by using efficient libraries written in Rust or C. Mostly. One time I implemented RSA in Python. I needed it to read some legacy data with the wrong padding, something which the libraries couldn't (wouldn't) do. It performed just fine. The ternary pow() that does the heavy lifting is not quite as fast as the dedicated crypto libraries, but it was close enough.
- sidewndr46 4y agoIn that case, you aren't doing cryptography in Python. You're using cryptography in Python. I'm not just saying that to be pedantic, I've seen some crypto implemented in Python and it is awfully slow. This is the case of most Python code & is why many attempts to compile Python to native code results in slower overall performance.
- munch117 4y ago> In that case, you aren't doing cryptography in Python. You're using cryptography in Python. One of the great things about the Python community is that we don't have these kinds of artificial hangups. You just solve the problem, and if part of the solution involves something that someone wrote in a different programming language, that's fine. Earlier this year, I implemented ECIES in Python. Not a very complicated algorithm, but it still needed to be done. So there is C involved in the underlying EC and AES implementations, big deal, I don't see how that should disqualify my ECIES work from being "doing cryptography".
- googlryas 4y agoFor anyone wondering, '1' * 4301 creates a string of '11111....' 4301 characters long. It doesn't result in an integer value of 4301 like in some other languages. I find this a strange modification to the language, though probably not a particularly painful one. Has python saved you from yourself when dealing with non-linear built-in algorithms before? IIRC it is also possible to have the regex engine take an inordinate amount of time for certain matching concepts(I think stackoverflow was affected by this?), but the engine wasn't hobbled to throw in those cases, it is merely up to the user to write efficient regex that aren't subject to those problems.
- hyperpape 4y agoBacktracking regular expressions as an intentional or accidental DOS vector are a moderately well-known issue, and while I prefer that a standard library implementation be robust against them, I can see the POV that it's buyer beware. Converting a string to an integer is somewhat less well known as a DOS vector, more painful to avoid as an application creator, and easier to fix in code. So there's a cost-benefit argument that you should just do this before you rewrite your regex engine.
- masklinn 4y ago> I can see the POV that it's buyer beware. On the other hands, lots of buyers are not aware that it's an issue, and more frustratingly there are regex engines which are very resilient to it... but are not widely used. Python's stdlib will fall over on any exponential backtracking pattern, but last time I tried to make postgres fall over I didn't succeed. Even though it does have lookahead, lookbehind, and backrefs, so should be sensible to the issue (aka it's not a pure DFA).
- bo1024 4y agoThis does seem like a strange level of handholding, even if the motivation makes lots of sense. If you start going down the road of protecting people who don't sanitize user input, you may have quite a long journey ahead...
- mjevans 4y ago
- wmichelin 4y agoCan anyone TL;DR why? Why wouldn't it just return that long integer of all 1s?
- sp332 4y agoYeah it's right at the top of the linked page?
- schoen 4y agoIt's stated to be CVE-2020-10735, which is apparently about a denial of service by forcing Python to inefficiently convert a very large string to an integer, using a potentially ridiculous amount of CPU time. The CVE hasn't been published, but for example there's an explanation at https://bugzilla.redhat.com/show_bug.cgi?id=1834423 https://bugzilla.redhat.com/show_bug.cgi?id=1834423
- adgjlsfhk1 4y agothis seems like a dumb fix to the cve to me. why not just use a faster algorithm?
- lifthrasiir 4y agoBecause there is no linear-time algorithm for decimal-to-binary conversion. If we are to expose the bignum-aware `int` function to untrusted input there should be some limit anyway. I do think the current limit of 4301 digits seem too low though---if it were something like 1 million digits I would be okay.
- schoen 4y agoIt looks like there is some discussion of the algorithmic options at https://github.com/python/cpython/issues/95778 https://github.com/python/cpython/issues/95778 https://github.com/python/cpython/issues/90716 https://github.com/python/cpython/issues/90716 Is there something bad going on with Python's internal representation of big integers, too? I thought I might have understood Tim Peters to be saying that in the latter thread. It does look like gmpy2.mpz() is like 100 times faster than int() or something. Is this just because it's doing it all in assembly rather than in Python bytecodes, or are the Python data structures here also not so hot?
- schoen 4y agoIf you need to make integers this big from decimal representations, I guess you could still use gmpy2.mpz(), and then either leave the result as an mpz object (which is generally drop-in compatible with Python's int type, with the addition of some optimized assembly implementations of arithmetic operations and some additional methods), or convert it to a Python int by calling int() on it.
- jwilk 4y agohttps://github.com/python/cpython/issues/95778 https://github.com/python/cpython/issues/95778 has more information.
- dang 4y agoOk, we'll change to that from https://pythoninsider.blogspot.com/2022/09/python-releases-3107-3914-3814-and-3714.html https://pythoninsider.blogspot.com/2022/09/python-releases-3.... Thanks! All: submitted title was "`int('1' * 4301)` will raise ValueError starting with Python 3.10.7" and comments reference that, so you might want to take a look at both URLs.
- eugenekolo 4y agoCould they not have modified the `int` function to `int(thingy, i_really_want_to_do_this=false)`? Edit: Looks like they added a python argument to increase the limit. So if you really need this, I suppose you can search around until you figure out why it's not working and pass the correct argument to the python bin.
- justinsaccount 4y agoFrom the linked bug.. > It takes about 50ms to parse an int string with 100,000 digits and about 5sec for 1,000,000 digits. The float type, decimal type, int.from_bytes(), and int() for binary bases 2, 4, 8, 16, and 32 are not affected. Sure seems strange to set the limit to 4300. 50ms is not a DoS.
- grnmamba 4y agoIt might be a DoS for a webserver that's supposed to be IO-bound. Turning on that behavior for all python code in a patch release with only a global override is sloppy.
- xani_ 4y agobalooning 2ms request to 50ms is absolutely a DoS that's only 20req/sec to fill a core of execution
- LudwigNagasena 4y agoJust rate-limit anyone who does 20req/sec.
- gpshead 4y agoThis is easy for huge corporations who live and breathe automated-DDoS protection without blinking an eye, but a major challenge for all of the little applications and small hosts.
- bo1024 4y agoFrom the link: > Everyone auditing all existing code for this, adding length guards, and maintaining that practice everywhere is not feasible nor is it what we deem the vast majority of our users want to do. It's hard not to read this as "we want to use untrusted input everywhere with no consequences". Seems like we'll be kicking as many issues under the rug as we're fixing with this change, right?
- Dylan16807 4y agoIt's easy for me not to read it that way! Converting to an integer is a very good start for validating many kinds of input.
- bostik 4y agoI read it the other way round - untrusted input is used in various places where doing such inline checks is prohibitively tricky. The examples given are quite telling: json, xmlrpc, logging. First two are everywhere in APIs. The third is just ... everywhere. Are you really going to use a JSON or XML stream parser first before feeding it to the stdlib module? And one that does not try to expand the read values to native types? As for logging, that is certainly the place where you are not only expected, but often required to use untrusted input. The fix feels like a heuristic and a compromise. None of the [easily available] solutions are robust, solid or performant, so someone picked an arbitrary threshold that should never be hit in sane code. The linked issue mentions that GMP remains fast even in face of absurdly big numbers. No surprise, the library is literally designed for it: MP stands for multi-precision (ie. big int and friends).
- adgjlsfhk1 4y agothis would all make more sense if python was using a reasonably fast string to int routine, but the one they are using is asymptotically bad, and the limit they chose is roughly a million times lower than it should have been.
- rwmj 4y agoDid they consider doing tainting (like Perl)? Input strings are marked as tainted and anything derived from them, except for some specific operations that untaint strings. If you use a tainted string for a security-sensitive operation then it fails. http://perlmeme.org/howtos/secure_code/taint.html http://perlmeme.org/howtos/secure_code/taint.html
- im3w1l 4y agoThis will break correct code for a fairly small benefit. I don't think they should do this in a patch release.
- deleted 4y ago[deleted]
- deleted 4y ago[deleted]
- gfd 4y agoWhy did they close the discussion due to code of conduct? I didn't see anything wrong with the previous comments before that point.
- klodolph 4y ago> As a reminder to everybody the Python Community Code Of Conduct applies here. > Closing. This is fixed. We'll open new issues for any follow up work necessary. The issue was marked closed, because the associated work was completed and the PR was merged. The same comment happened to mention the code of conduct, but the code of conduct wasn't why the issue was closed--it was just because the work was done. I think the comment mentioned the CoC because the previous comment, "This is appalling" was a bit rude.
- Delk 4y ago> I think the comment mentioned the CoC because the previous comment, "This is appalling" was a bit rude. The previous comment was indeed a bit rude. I personally wouldn't think it was rude enough to invoke a code of conduct. Even just referring to a code of conduct has, IMO, a rather strong vibe of policing and perhaps even an implication of wrongdoing, more so than merely a suggestion to keep it calm. I don't know the culture or context of Python development (either the language or CPython), but I'm inclined to agree with gdf that it's a bit weird to start reminding people of a CoC because of a slightly rude sentence or two, especially since the rest of the comment was reasonable technical argumentation even if unapologetic. Even if closing the issue were entirely because of other reasons and benign (someone did still reference the issue in a commit later, though), it's all too easy to see the issue-closing comment as shutting out dissenting opinions, either because of a somewhat unpleasantly expressed argument or simply because "this is fixed, no further discussion needed". The "this is appalling" comment may have been a bit rude but the closing one wasn't exactly a triumph in communication either.
- Guthur 4y ago"This is appalling" is not even remotely rude, honestly are we all children now?
- mywittyname 4y agoWhat should you use instead if you want the original functionality?
- Veedrac 4y agohttps://docs.python.org/3/library/stdtypes.html#configuring-the-limit https://docs.python.org/3/library/stdtypes.html#configuring-...
- mywittyname 4y agoIf I'm understanding this correctly: the only way to convert an extremely large base10 string to an integer using the standard library is to muck with global interpreter settings? It seems short sighted to not provide some function that mimics legacy functionality exactly. Even if it is something like int.parse_string_unlimited(). Especially since a random library can just set the cap to 0 and side-step the problem entirely.
- Someone 4y ago> Especially since a random library can just set the cap to 0 and side-step the problem entirely. Until another random library sets it to its preferred value (see https://news.ycombinator.com/item?id=32738206 https://news.ycombinator.com/item?id=32738206 for a similar issue with a CPU flag for supporting IEEE subnormals) We might end up with libraries that keep setting that global to the value they need on every call into them.
- mywittyname 4y agoOh fun. Just what Python needs more of, this... try: value = int(value_to_parse) except ValueError: import sys __old_int_max_str_digits = sys.get_int_max_str_digits() sys.set_int_max_str_digits(0) value = int(value_to_parse) sys.set_int_max_str_digits(__old_int_max_str_digits) Or maybe just this: class UnboundedIntParsing: def __enter__(self): self.__old_int_max_str_digits = sys.get_int_max_str_digits() return self def __exit__(self, *args): sys.set_int_max_str_digits(self.__old_int_max_str_digits) with UnboundedIntParsing as uip: value = int(str_value)
- js2 4y ago4300 digits? > Chosen such that this isn't wildly slow on modern hardware and so that everyone's existing deployed numpy test suite passes before https://github.com/numpy/numpy/issues/22098 https://github.com/numpy/numpy/issues/22098 is widely available. https://github.com/python/cpython/blob/511ca9452033ef95bc7d7fc404b8161068226002/Include/internal/pycore_long.h#L14-L28 https://github.com/python/cpython/blob/511ca9452033ef95bc7d7...
- svet_0 4y agoSo now an unreasonable user input will crash my server instead of slowing it down by 50ms. Great DoS mitigation!
- omnicognate 4y agoYour server crashes if a request fails?
- xani_ 4y agoit does with this change where it didn't before. At the very best you're still restarting the whole process instead of just wasting a bit of time
- aYsY4dDQ2NrcNzA 4y agoThen don’t upgrade Python in your container?
- progval 4y agoYou should always catch ValueError when using int() on user input, because that input may not be a valid number.
- deleted 4y ago[deleted]
- mattnewton 4y agoI should also check to see if the length is reasonable, no? But the whole point of the issue is that nobody finds that practical.
- fuckstick 4y agoWho uses a process per request for serving Python apps? That must be very uncommon. Even if you use a worker pool that isn’t going to restart a whole process just because of an errant exception in a request handler. Also as noted if your whole process crashes because of errant input to int() you are beyond fucked in other ways.
- Phil_Latio 4y agoWhat's next? A default socket timeout of X seconds for security reasons? What a joke and rather scary that apparently everyone or the majority on the internal side agrees with this change.
- linspace 4y agoI find it completely unpythonic. Python has become too important to do the right thing, there is money on the table.
- LtWorf 4y agoI think python is now completely owned by a couple big companies that decide everything. By this logic they should also block me from running benchmarks on too big lists, because I'm dossing myself.
- krick 4y agoThis. I don't really understand CPython decision-making process, but it just seems like a common sense that anybody who would find this a good idea surely must be a very junior developer who shouldn't be allowed to commit directly to the master branch of your local corporate project just yet… But basically breaking a perfectly logical behaviour just like that in a language used by millions of people… To me it's absolutely shocking.
- sidewndr46 4y agoYou would also need a limit on how large of a file can be written by python. Otherwise you could have a web server that takes an upload and stores it on disk which could fill the disk of the host machine. We can't expect developers to check for this, so Python must be patched to not write files larger than 2 kilobytes.
- qbane 4y agoYeah, we must prevent DoS at all costs. It seems that Python should not have integers at arbitrary size for "performance" reason in the beginning. Aren't int32/int64/int128 nice? Number of operations are all bounded. We should stick to them.
- kragen 4y agoThis was Python's behavior until Python 2; `long`, the arbitrary-precision integer, was a separate type, and `int` arithmetic overflow caused a ValueError. One of the big changes in Python 2 was to imitate the behavior of Smalltalk and (most) Lisp by transparently overflowing `int` arithmetic to `long` instead of requiring an explicit `long()` cast. Python 3 eliminated the separate `long` type altogether. Having been bitten by the Smalltalk behavior, I am skeptical that the Python 2 change was a good idea.
- gpshead 4y agoI keep wondering if it was as well given code I've had to wrangle that _wants_ twos compliment fixed size math in Python. Both signed and unsigned. But our language tries not to have a bazillion different basic types and the ill-defined Python <= 2 `int` being whatever the platforms `C long` could hold was not great so simplifying to a single integer type in 3 was still a net win AFAICT.
- kragen 4y agoIt'd be nice to have a twos-complement fixed-size type too, but I think it's probably better that Python 2's int isn't that. The problem with transparently overflowing to Python `long` is that, most of the time, the overflow is unintended, and the resulting performance collapse is a bug that's harder to track down than a ValueError.
- loeg 4y agoWill Python's relentless campaign to break backwards compatibility never end? (80% sarcastic.)
- klyrs 4y agoDon't worry, it's a minor release. (110% sarcastic)
- tremon 4y agoIt's a patch release, not even minor (100% serious).
- chmod775 4y agoEvery morning while enjoying my breakfast cereal I read about what python broke today (95% whole grain).
- speedgoose 4y agoDoes CPython follow semantic versioning ?
- klyrs 4y agoThat's what we're complaining about.
- ridiculous_fish 4y agoWhy is base 10 string -> int a quadratic algorithm? Are there no faster ones that could be implemented?
- blahedo 4y agoNo, because 10 is not a power of 2, so any digit in the source (base 10) can affect any digit in the result (base 2). Converting from e.g. base 16 to base 2 is linear, because 16 is a power of 2.
- krick 4y agoThis simply isn't true. Faster algorithm is even linked in comments to this particular issue: https://members.loria.fr/PZimmermann/mca/mca-cup-0.5.9.pdf https://members.loria.fr/PZimmermann/mca/mca-cup-0.5.9.pdf (p.38)
- blahedo 4y agoMy mistake! I knew it couldn't be linear and got sloppy. (For those checking that link, see p38, section 1.7.2)
- adgjlsfhk1 4y agoThe best algorithm isn't quadratic. It's M(n)log(n) where M(n) is the cost of your integer multiply (M(n) theoretically be as low as nlog(n), but in practice the best algorithms used are nlog(n)log(log(n))). Python just didn't bother to implement them.
- blibble 4y agonew interpreter argument: -X int_max_str_digits=number limit the size of int<->str conversions. This helps avoid denial of service attacks when parsing untrusted data. The default is sys.int_info.default_max_str_digits. 0 disables. this should not be a runtime configuration setting, fix the sodding algorithm to not be quadratic will we be getting PHP style magic quotes soon? that also protects developers against untrusted input (bonus! this could be configured too!) or an inability to pass strings into the regular expression module? that can also cause DoS (what happened to Python?)
- simonw 4y agoMy understanding is that there is no algorithm for this that isn't quadratic. Update: I may have understood incorrectly, see https://github.com/python/cpython/issues/90716 https://github.com/python/cpython/issues/90716
- blibble 4y ago> My understanding is that there is no algorithm for this that isn't quadratic. > If you know of one, the Python core development team would love to hear about it! it's mentioned on the issue page that makes up the article... (before they closed it due to the "code of conduct")
- deleted 4y ago[deleted]
- saghm 4y agoI was surprised to see this in a bugfix release since it seems like a breaking change, but from reading, it seems that this was considered a security vulnerability (specifically a DOS opportunity) given the CVE status, so I imagine that compatibility concerns were secondary here. This seems in line with how other languages seem to do things from what I've seen; semver is important, but in a sense not every change is equally "breaking" to users, and breaking code that's unlikely to be common and potentially is not behaving correctly in the first place is not going to cause as much friction as most other types of breaking changes. Put another way, if there's a valid security concern, breaking things loudly for users forces them to double check their usage of this sort of code and ensure that nothing risky is going on. (I don't personally have enough domain knowledge here to know if the security concern is actually valid or not, but the decision to make this change in a patch release seems like a reasonable conclusion to come to for people who determine that it is a security concern).
- jhugo 4y agoIs this GitHub issue typical of the Python community nowadays? A two-year old CVE is picked up by gpshead as suddenly urgent enough to justify a fairly significant breaking change in a patch release, despite another issue (#90716) already tracking better solutions to the same problem. gpshead submits a PR which doesn't solve the issue (the DoS still happens, and then a ValueError comes afterwards!), so mdickinson submits a PR which works around the problem in the first PR. Nobody else is apparently involved in discussing whether this change should even be made, and whether the fixed fix genuinely eliminates the DoS. mdickinson then has to fight for several comments to convince gpshead to make an obviously-correct and zero-cost change to the integer type, with gpshead dropping a passive-aggressive "pedantically correct" when they finally fix it. Then when other people finally notice the issue and start to question the need for the change, especially in the context of another issue discussing a better solution, they are ignored and the discussion is shut down with a reference to the Code of Conduct. Why would anyone want to participate in a community this toxic?
- nas 4y agoI'm pretty sure the decision on how to address the bug (and the determination of even if it's a bug) was not done by one dev. Other devs were involved and the determination was made as a team to make the change. Having a better fix, e.g. something like what is suggested in #90716 is not precluded. As yet, no one has actually stepped up with a better int-to-str implementation. I.e. something that can be reviewed, tested and then maintained in the long term As discussed in #90716, there is not much point in trying to do something similar to what GMP does. People can just install that library and use it. I'm not sure why people are so excited about this issue. It's not much different than sys.setrecursionlimit(). We know how to implement tail recursion. Python doesn't do it though and so there is a limit, set high enough that most people don't care. It seems a perfectly practical approach to me.
- adgjlsfhk1 4y agoIt's a breaking change in a patch release that fixes the "Security Vulnerability" that python is sometimes slow.
- amluto 4y ago> A huge integer will always consume a near-quadratic amount of CPU time in conversion to or from a base 10 (decimal) string with a large number of digits. No efficient algorithm exists to do otherwise. I don’t believe that. I did a quick search and didn’t find much, but: Let d_0, d_1, etc be decimal digits, little endian (so the number is d_0 + 10d_1, etc). The goal is to compute that quantity in binary. The naive algorithm is to convert d_0 to binary. Then compute 10 in binary, multiply by d_1, and add it. Then multiply to compute 10^2 in binary, and accumulate 10^2 · d_2, and repeat. For n digits, there are n steps, and each step involves two multiplications by small factors and an addition. The overall time is O(n^2). But I would try it differently. To convert 2n-digit number, first convert the first n digits to binary (recursively) and convert the second n digits to binary. Then multiple the second result by 10^n and add to the first result. Let’s simplify this analysis by making n be a power of 2, so 2n = 2^k. Then the big powers of 10 are all 10 to a power of 2, so 10 needs to be squared k times. Additionally, there is one 2^k-digit multiplication, two 2^(k-1)-digit multiplications, four 2^(k-2)-digit multiplications, etc. With FFT multiplication, multiplication is O(digits * poly(log(digits)), as is squaring. This will all add up to k levels of recursion, each acting on a total of 2^k or so bits, taking time O(2^k) times some log factors. This comes out to O(n · poly(log(n)). I have not implemented this or done the math rigorously :) edit: this is also buried in the issue. There’s also: https://github.com/python/cpython/issues/90716 https://github.com/python/cpython/issues/90716 I don’t get it.
- userbinator 4y agoIndeed, the naive algorithm is quadratic due to the multiplications. Yet everyone familiar with even a bit of crypto and/or otherwise working with huge numbers knows that multiplication can be subquadratic. ...and in doing a bit of research on how Python does multiplication, I found some... rather odd opinions, considering it's one of the few languages with arbitrary precision integers: https://discuss.python.org/t/faster-large-integer-multiplication/13300 https://discuss.python.org/t/faster-large-integer-multiplica...
- stingraycharles 4y agoIntegers / floating point / bignum is such a problem in almost any language, with multiple competing implementations in most languages. I’d argue it’s probably a harder problem than Unicode support, although Python also managed to make that difficult with multiple implementations in the wild (both UCS-2 and UCS-4 are commonly used, depending on how it was compiled).
- Sophistifunk 4y agoI clicked thinking this was some sort of client-proof-of-work access protocol, boy was I wrong!
- nine_k 4y agoIt took me some time to parse this as (Prevent) (DoS by large int-str conversions) and not (Prevent DoS) by (large int-str conversions).
- rurban 4y agoFor the curious. I've checked the corresponding perl5 code, and it is not affected by such json/yaml DOS attacks. It bails out early on overlarge bignums already. Ruby, no idea.
- fulafel 4y ago> A huge integer will always consume a near-quadratic amount of CPU time in conversion to or from a base 10 (decimal) string with a large number of digits. No efficient algorithm exists to do otherwise. This is pretty interestign in itself. Are there other sw compoenents that have flagged & fixed this vulnerability? Seems like there should be many.
- williamstein 4y agoIt is an incorrect assertion. See elsewhere in this thread and later on the linked ticket.
- williamstein 4y agoA well reasoned argument that this change was a bad idea: https://discuss.python.org/t/int-str-conversions-broken-in-latest-python-bugfix-releases/18889 https://discuss.python.org/t/int-str-conversions-broken-in-l...