gh-136061: IDLE - update code in editor.Editor.load_extension

[johnzhou721]

A DOS by Quadratic complexity issue is fixed in idlelib. Part of (but does not fix) #134873.

• Issue: A Series of Simple DOS Vulnerabilities #134873

• Issue: IDLE: Performance Issue with Long Lines Due to Quadratic Complexity #136061

[terryjreedy]

I believe that the 6 lines from 1205 to 1210 can be replaced by 2 lines -- an re.match and an f-string. I will submit an alternate proposal later. I believe that the input vevent name should have either no <>s or 2 of each, with maybe the latter for back compatibility (I will test). But I will may stick with the more general code to not break buggy extensions.

[ZeroIntensity]

Assuming this is the fix that we go with, let's add a test case.

[johnzhou721]

@terryjreedy so should I leave the code for now, or should I go ahead and replace with the re.match thing you are going to propose? @ZeroIntensity so do you mean that active voice is preferred in release notes? I can replace this specific case with the change that you are suggesting, but I'm asking for advice in this aspect for future News.

[johnzhou721]

Yes, I think it can be. Will fix.

…

Message ID: ***@***.***>

[kexinoh]

@johnzhou721
I would greatly appreciate it if you could kindly address the issue located at

cpython/Lib/idlelib/editor.py

Lines 1373 to 1378 in 5ab66a8

	while True:
	chars = chars[:-1]
	ncharsdeleted = ncharsdeleted + 1
	have = len(chars.expandtabs(tabwidth))
	if have <= want or chars[-1] not in " \t":
	break

. I sincerely apologize for overlooking this in my previous message.

As an example, I successfully utilized Gemini 2.5 Pro to generate a reasonable fix for this problem. Could you give it a try?

[johnzhou721]

@kexinoh Yes, I would give it a try once I have time; however, I am working on something else right now -- is it acceptable if I delay this by about a day or so?

(if anyone else has a fix ready before I get to this, feel free to make a pr onto the branch of my pr and I'll merge it into my PR)

[johnzhou721]

@kexinoh I have a small amount of time not enough to work on anything else before I end my day so I attempted the issue you pointed out -- but can't test though.

[johnzhou721]

Assuming this is the fix that we go with, let's add a test case.

Where? How? For what? Thanks! @ZeroIntensity

[ZeroIntensity]

Where? How? For what?

We need a test case in test_idlelib that results in DOS/extreme slowness off main. Basically, just do something to prove that this PR fixes it (probably just testing with large amounts of data).

[zware]

Please don't keep merging main like that, it consumes CI resources unnecessarily. A merge is only really necessary to resolve conflicts or as a final check before merge, which is not the stage we're at here yet.

[johnzhou721]

Please don't keep merging main like that, it consumes CI resources unnecessarily. A merge is only really necessary to resolve conflicts or as a final check before merge, which is not the stage we're at here yet.

Noted.

[johnzhou721]

@picnixz Have you gotten time yet?

[picnixz]

I don't use a lot idlelib so I don't know if there is a way to inject very long strings into it to trigger the bug. The rstrip/lstrip improvement can be isolated in a separate PR though, but for the bug in itself, we can just come up with a comment saying that it's unrealistic to trigger it.

However, I either want a proof that we can't trigger the DOS (or it's very hard to do so) or a PoC demonstrating that we can trigger it (not just some "we could do ...")

[johnzhou721]

@picnixz In fact, it might be possible to prove that this is untriggerable -- does IDLE allow you to load any kind of settings file that allows specifying huge indent widths? If not -- this is probably unrealistic to do it -- because the want values I tested in this PR are extremely unrealistic unless you have huge indentwidths, and some of them are flat-out impossible edge cases I made up just to make sure the new implementation is equivalent to the old one.

It's trivial to prove that the old loop is of complexity (EDIT: of AT MOST) O(self.indentwidth*length) and the new one is O(length)... I suspect I might be able to just reduce this to O(self.indentwidth) if I work backwards in delete_trail_char_and_space and be careful but chars.expandtabs is already >= O(n) so this is a moot point.

In other words: This is exploitable only if (but NOT if) you can set huge indent widths, AND the line width has to be reasonably large in order to have any possibility to do this.

However, I still can't figure out how to get to this function at all -- I don't really understand the surrounding code well.

I will be happy if I understand this code enough, to write a "virus" that changes config-main.cfg and immediately open a file with a bunch of spaces and tabs and press backspace. However we need to show that it's non-exploitable if we don't press backspace (EDIT -- I mean that it's going to be more exploitable / freeze longer using this than simply, say, loading a file with a bunch of spaces.).

I really apologize for the weird discussion.

[johnzhou721]

That said: someone could put an even huger indentwidth into the config file, and when the user presses tab, freezes the IDLE (will this happen?) If this will happen, my suggestion is to ensure that when you load the cfg file that you validate the values are below 10 or something, which would make this pretty much not more exploitable over just having a huge length

@picnixz Are you okay with this?

[johnzhou721]

@kexinoh Since you reported this bug, I would appreciate if you could read this patch and see if my analysis above is correct. Thank you for making CPython a safer product.

[kexinoh]

Whether there is a concrete issue depends on CPython's threat model. Regarding the specific problem in the idle library, I agree that its harm is not as severe as vulnerabilities in email or path parsing, since it is difficult for remote users to directly control or exploit it. However, considering that a DoS vulnerability, as a time-complexity issue, may not cause visibly noticeable downtime, fixing it could still improve CPython's runtime performance. Such a fix would be entirely harmless (as it wouldn’t alter any existing functionality) and beneficial to CPython. In that case, there seems to be no reason not to accept it.

I believe this fix for the idle flaw could be treated as part of an improvement rather than as a security patch.

[johnzhou721]

But

• It makes the code a bit less cleaner
• IDLE is not used in production, so it's sort of isolated and does nothing to overall performance
• It's only a 4x speedup at most (indentwidth) with usual settings -- and that's excluding all the other UI handling going on.

I'll leave it to @terryjreedy to decide on this.

[terryjreedy]

When I last looked at this, the issue was only about modernizing load_extension(). The 4 lines replaced were part of the original merge in August 2000. The non-whitespace 'char' parameter for the lstrip and rstrip replacements was introduced about 2 years later, in 2002. I will accept this change. (Accepting obviously correct modernizations has been my general policy.)

I see no DOS or even 'quadratic time' issue here. This code is only executed once, at IDLE startup. As I said when this was opened, there should be at most 2 pairs of '<>'s. The possibility of there being 0 or 2 pairs in 3rd party additions is due to some ambiguity in the extension instructions, and others numbers could result from typos. If someone installs an extension from a malicious author, they have more of a problem than IDLE hanging on startup because the author put an event name with 1000s of pairs of '<>'s in config-extensions.def.

I am rejecting the refactoring of the handing of space and tab in smart_backspace_event(). Any such project must begin with a separate issue and an detailed analysis of current versus desired behavior, and it must consider the entire block of relevant code. This is lines 1365-1385, which begin with the comment

It may require inserting spaces if we back up over a tab character! This is written to be clear, not fast.

Currently, spaces and tabs are treated the same anywhere in the line, while only deletion of an indent space should possibly result in the deletion of multiple spaces. In current default IDLE, tab characters cannot be put in a file; they are intercepted or converted to spaces according to indentwidth. (Changing this requires editing editor.py.) Even if a file from elsewhere is loaded, a space after a tab should just be deleted. The current function is icky partly to handle mixed space and tab indents; python now rejects such, and IDLE runs editor files through tabnanny to check.

Someone asked about self.tabwidth. It is currently set to 8 because it cannot be changed, AFAIK, in tk Texts. Comments in editor.py imply that this was a bug to be fixed. 2 decades later, maybe we should delete code that anticipates this changing. In any case, I would like to consider deprecating having IDLE handle .py files with tabs, or at least better define and document what it will and will not do.

Again, I do not consider this a DOS problem. IDLE is not a 'service' and does not connect to the internet. Nor is 'quadratic time' a real issue. The looping in the edited lines is limited to indentwith, which defaults to 4 and is limited to 10 in the Settings dialog. Again, anyone who can edit files or get code files loaded can do much worse than than make IDLE hang on hitting [<--].

I will edit the title and blurb and delete the refactor additions.

[terryjreedy]

Still need to delete Security blurb and add IDLE blurb and news items. However

f:\dev\3x>git push https://github.com/johnzhou721/cpython pr_134874:idledos

remote: Permission to johnzhou721/cpython.git denied to terryjreedy.
fatal: unable to access 'https://github.com/johnzhou721/cpython/': The requested URL returned error: 403

This is my standard pr revision push template. with pr#, johnzhou721 idledos taken from top of this page.

@zware Do you have any idea what is wrong? Is the fact that johnzhou forked from somewhere else than python/cpython relevant?
@johnzhou721 Did you uncheck [] allow others to edit pr? The fact that I edited online suggests not.

Edit: deleted blurb on View files page. Unfortunately, that triggers retest, which will fail because no blurb.

Blurb-it failed also due to lack of access to PR.

[johnzhou721]

@terryjreedy Thanks for looking into this issue and resolving my suspicions. I will be blurbing it.