fix: buffer overflow in multipart body proc

[airween]

what

This PR fixes a possible heap buffer overflow in multipart body processor.

why

There is a bug report, received in email from @fumfel and his team. Also they provided this fix.

references

The original report:

Root Cause
----------

Bug A: The condition checks *(p+1) and *(p+2) for hex digits. When
*(p+1) is the last byte before the null terminator, the short-circuit
evaluation of the || chain does NOT prevent *(p+2) from being read.
The condition (!isxdigit(*(p+1))) is false when *(p+1) is a hex digit,
so evaluation continues to (!isxdigit(*(p+2))) which reads past the
allocation.

Bug B: After the while loop that scans a quoted value, the code
unconditionally does p++ to skip the closing quote. But if the string
ended without a closing quote (the loop exited on *p == '\0'), this
advances the pointer past the null terminator, causing subsequent
reads to access out-of-bounds memory.

other notes

After a review and a short discussion, @fumfel and his team confirmed that this issue can occur if the source code was built with ASAN flags.

[Copilot]

Pull request overview

Fixes a potential out-of-bounds read/heap buffer overflow in multipart Content-Disposition parsing, based on an external bug report.

Changes:

• Adds an explicit *(p+2) == '\0' guard before validating the second hex digit in filename* percent-escapes.
• Avoids incrementing p past the string terminator when a quoted parameter value is missing a closing quote.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The PR addresses out-of-bounds reads in Content-Disposition parsing; adding regression cases for (1) filename* values ending with an incomplete percent-escape (e.g. trailing %A) and (2) quoted values missing a terminating quote would help ensure this remains fixed (especially under ASAN/UBSAN).

Suggested change

	if (*p == quote) {
	p++; /* go over the quote at the end */
	}
	if (*p == '\0') {
	/* missing terminating quote */
	return -7;
	}
	p++; /* go over the quote at the end */

[airween]

I'm not sure this is a valid case, multipart header can contain urlencoded text, but the parser's task process it as is.

This suggestion removes that very important condition, I'm afraid that could make the parser wrong.

[theseion]

You only get here if a opening quote was observed, so a closing quote is expected. If the closing quote is URL encoded, then that looks like an evasion attempt. If the opening quote is also URL encoded, we won't end up here anyway (I think), so I think it's valid to say that the quote is missing.

I don't think we should set invalid_quoting in this case, the semantics are different.

I don't understand what difference it makes for you whether the return value is -7 or -15. The previous suggestion would behave the same, just with a different return code.

[airween]

I think here we should split up the suggestions into two parts:

• handling url-encoded characters
• handling missing quote at the end

First item: not relevant, the parser does not handle the url encoded quotes at all, this is why I wrote that the suggestion from Copilot is not a valid case.

Second: the missing quote is handled correctly with this, I added a new test case; I checked this test without this modification, which was failed.

I don't understand what difference it makes for you whether the return value is -7 or -15. The previous suggestion would behave the same, just with a different return code.

I think this is why we shouldn't change anything here 😃.

[theseion]

You only get here if a opening quote was observed, so a closing quote is expected. If the closing quote is URL encoded, then that looks like an evasion attempt. If the opening quote is also URL encoded, we won't end up here anyway (I think), so I think it's valid to say that the quote is missing.

I don't think we should set invalid_quoting in this case, the semantics are different.

I don't understand what difference it makes for you whether the return value is -7 or -15. The previous suggestion would behave the same, just with a different return code.

[theseion]

Why is the cast necessary here? And why isn't it necessary for p+1?

[airween]

Why is the cast necessary here?

see Copilot's suggestion

And why isn't it necessary for p+1?

you're right, I should cast p+1 too.

[airween]

Done, see 9b33b2e.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud