Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
uwezkhan wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −2
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sql/proxy_protocol.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,10 @@ int parse_proxy_protocol_header(NET *net, proxy_peer_info *peer_info)
if (have_v1_header)
{
/* Read until end of header (newline character)*/
while(pos < sizeof(hdr))
while(pos < sizeof(hdr) - 1)
Copy link
Copy Markdown
Member

@gkodinov gkodinov Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
long len= (long)vio_read(vio, hdr + pos, 1);
if (len < 0)
if (len <= 0)
Copy link
Copy Markdown
Member

@gkodinov gkodinov Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@vaintroub vaintroub Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@gkodinov gkodinov Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@vaintroub vaintroub Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@gkodinov gkodinov Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@vaintroub vaintroub Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Author

@uwezkhan uwezkhan Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@vaintroub vaintroub Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

return -1;
pos++;
if (hdr[pos-1] == '\n')
Expand Down