MDEV-39219: buffer overflow in PROXY protocol v1 header parsing

[uwezkhan]

This fixes a buffer overflow issue in the PROXY protocol v1 header parsing.

The loop that reads the header could fill the entire buffer and then add a null terminator past the boundary, which leads to an off-by-one overflow.

I updated the loop condition to make sure there is always space left for the null terminator.

Also added a small check to handle cases where the connection closes early, so we don’t process incomplete data.

This change does not affect normal behavior, it just makes the parsing safer.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

uwezkhan06 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[gkodinov]

Thank you for your contribution! This is a preliminary review.

First of all: sign the CLA please: either pick BSD or MariaCLA.

Secondly: Please add a commit message to your commit that's compliant with MariaDB coding standards.

Thirdly: This is a bug. Find the lowest SUPPORTED version this is in and target that instead of main. I'd say 10.11.

And, last but not least: please add a test! There are regression tests for the proxy protocol. Extend these.

[gkodinov]

this is wrong: you can get a 0 if there's no bytes to read when you call recv(). But there may be more bytes to read later. It needs to keep trying.

[vaintroub]

I think it was correct @gkodinov . 0 is returned from recv if and only if peer closes socket normally (FIN, not RESET), at least when reading more than 0 bytes.

[gkodinov]

https://man7.org/linux/man-pages/man2/recv.2.html says:

The value 0 may also be returned if the requested number of bytes
to receive from a stream socket was 0.

[vaintroub]

yes, this does not contradict to what I said previously . We are not reading 0 bytes. We're reading 1 byte.

[gkodinov]

It doesn't contradict what I said either I believe: it's wrong to treat having read 0 bytes as an error condition. If the connection is still open more might come on the next read.

[vaintroub]

Nope, this is wrong. 0 is an error condition. It means, other side did close(), closesocket(), or shutdown(SHUT_WR). Nothing more will come. Unless on some weird reason you using recv() with 3rd parameter 0. But nobody does it here, and I do not see any reason to ever call recv() with 3rd parameter 0, on Linux or elsewhere. If recv() trying to read 1 byte : if recv() is non-blocking, and there is nothing to read, it returns -1 with (WSA)EWOULDBLOCK. if recv() is blocking, and there is nothing to read it will block until it can read 1 byte, or until error, and return code 0 is this specific error (peer closed/shut down connection orderly), and -1 is for other errors (ECONNRESET, ETIMEDOUT).

[uwezkhan]

Thanks for the discussion, this is helpful.

From what I understand, since we’re reading 1 byte in blocking mode, a return value of 0 would indicate that the peer has closed the connection and no more data will arrive. In that case, continuing to read would likely result in a loop without progress.

To keep things safe and avoid partial parsing or infinite loops, I’m thinking of treating len <= 0 as a termination condition here.

That said, I want to make sure I’m aligned with the expected behavior in this part of the codebase. If there’s a preferred pattern for handling this in MariaDB networking code, I can follow that.

For the buffer issue, I’ll update the fix to increase the buffer size by one byte and keep full header parsing intact with safe null termination.

Let me know if that approach sounds good and I’ll update the patch accordingly.

[vaintroub]

I think this is pretty good, 0 is in this case definitely the "close" condition. If you continue to try to read past that, I do not know for sure, I think at some point it could return -1 with ECONNRESET, or EPIPE or something like that.

You can give it a try. The tests for proxy protocol are in tests/mysql_client_test.c (look for test_proxy in the file), you can add yours, with header that would cause buffer overflow.

A test with an incomplete header and shutdown(SHUT_WR) (half-closing connection, which I'd choose as a testable version for the "close" scenario) is trickier, as you can't use client API for that. I.e you'd need just a TCP socket that connects to the server, sends incomplete proxy header, half-closes connection with shutdown(SHUT_WR), reads server "welcome" packet, reads and checks for expected server error packet, then closesocket().

[gkodinov]

This is wrong: it MUST be able to parse the whole header. I'd increase the buffer's length and have a termination zero as an extra.