gh-136063: fix various quadratic worst-time complexities in _header_value_parser.py [WIP]

[picnixz]

This is a work in progress (I need to go now) but I'll continue tomorrow. I want to add tests, and some other places are still not fixed because I didn't find a straightforward fix.

• Issue: A Series of Simple DOS Vulnerabilities #134873

• Issue: Potential Quadratic Complexity Vulnerabilities in the email Module #136063

[picnixz]

Urgh, so there is still a quadratic complexity that I need to think about, it's when we alternate if-branches. For instance in get_phrase:

    try:
        token, value = get_word(value)
        phrase.append(token)
    except errors.HeaderParseError:
        phrase.defects.append(errors.InvalidHeaderDefect(
            "phrase does not start with word"))
    while value and value[0] not in PHRASE_ENDS:
        if value[0]=='.':
            phrase.append(DOT)
            phrase.defects.append(errors.ObsoleteHeaderDefect(
                "period in 'phrase'"))
            value = value[1:]
        else:
            try:
                token, value = get_word(value)
            except errors.HeaderParseError:
                if value[0] in CFWS_LEADER:
                    token, value = get_cfws(value)
                    phrase.defects.append(errors.ObsoleteHeaderDefect(
                        "comment found without atom"))
                else:
                    raise
            phrase.append(token)
    return phrase, value

The

if value[0]=='.':
    phrase.append(DOT)
    phrase.defects.append(errors.ObsoleteHeaderDefect(
        "period in 'phrase'"))
    value = value[1:]

is quadratic if we're processing multiple times .. However, if I have something like 'a + '.a' * 100, then the if branch still requires a copy every two iterations, whatever I put inside. Even if the length reduces at each iteration, it doesn't sufficiently reduce. I'll need to think a bit more.

One idea would have been to use a deque to prevent a copy when stripping parts, but then this requires to reconstruct a deque everytime.

Maybe switch to a stateful parser? That way, we shouldn't have high complexity and we should be fine. But this requires a complete rewrite of this module.

[picnixz]

Ok, this patch still fixes some cases but not all. Cases where two branches alternate would still be subject to O(n²) complexities (unless we avoid the copy in .lstrip() or in [1:], it's not possible to avoid this with .lstrip() or slices since they still need O(n) to copy the rest of the string).

The advantage of .lstrip() over slices is essentially when the if branch is executed more than once before going to the else branch (namely, we can batch-process some characters). For instance, "a" + "." * 50000 is parsed using lstrip() in O(n) instead of O(n²). However, "a" + ".a" * 50000 is still subject to O(n²) parsing.

[picnixz]

@serhiy-storchaka I'm a bit stuck here. I don't really have a better idea than to rewrite the module where we would use a deque to hold the current value. That way, I can call .popleft() to drop prefixed chars. Unfortunately, this also means that cannot really return a string anymore as the module is used and signatures are actually in _typeshed: https://github.com/python/typeshed/blob/main/stdlib/email/_header_value_parser.pyi.

So, to ensure backward compatibility, I think I'll need a new module... I can't think of another solution instead of entirely rewriting the logic so that we don't have un-necessary slice so your help would be appreciated, TiA!

---

I thought about holding the current "index" where the parser stopped but again, I don't think it'll be sufficient as I'll still need to make slices at some point to extract some values to hold (OTOH, using a deque allows me to move some characters to elsewhere without having to copy the string twice, though I'll still need a ''.join() on the part that is being stored).

def get_something(value):
	storage = Storage()
	head = []
	while cond(value):
	    head.append(value.popleft())
	storage.value = ''.join(head)
	return storage, value

[serhiy-storchaka]

This approach doesn't work, because the code has inherently quadratic complexity. Every function splits the input string on two parts -- the parsed part and the rest. This operation (creating a new still unparsed string) has linear complexity. Repeated, the total complexity is quadratic.

The only way to avoid quadratic complexity is to change interface of all functions. To rewrite the code in a style similar to the re parser or HTMLParser. I'll take care of this when I'm done with HTMLParser.

[picnixz]

The only way to avoid quadratic complexity is to change interface of all functions. To rewrite the code in a style similar to the re parser or HTMLParser. I'll take care of this when I'm done with HTMLParser.

Yeah that's what I feared and thus I didn't start this work until you confirmed it. Thanks for taking care of this as it would take me too much time.