Skip to content

Conversation

@paul-gerste-sonarsource
Copy link
Contributor

@paul-gerste-sonarsource paul-gerste-sonarsource commented Jun 3, 2024

Problem

The Postgres wire protocol has certain fields that contain fixed-size integers, such as the 32-bit message length (see here). This library does not check if values fit into these fixed-size fields before writing them to the buffer. This can lead to the truncation of the values being written, causing corrupted messages.

Example: If the message to be sent is $4^{32} + 4$ bytes long, the length cannot fit into the 4-byte length field. The value written to the buffer will be \x00\x00\x00\x04, which gets decoded to $4$ by the Postgres database when it receives such a message. Therefore, the Postgres database will think the message has a length of 4 and will try to read the next message after those 4 bytes.

After such a malformed message, the client and the database have different understandings of where messages start and end. In the best case, this causes a connection abort due to a parsing error. In the worst case, this leads to the execution of malicious SQL statements that an attacker has injected into a large payload that ends up in an SQL query.

Solution

I added size checks that error if the value being written is too large for its field. Since the functions that I changed don't return errors, I had to panic() in the error case. I opted for this route because I noticed that other functions do the same (example 1, example 2). Please let me know if there's a different error-handling mechanism I should use; then I'll amend the PR.

Copy link

@hmh hmh left a comment

Choose a reason for hiding this comment

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

LGTM

@arp242 arp242 added the needs-test Needs a test before it can be merged label Dec 31, 2025
@arp242 arp242 closed this in #1209 Dec 31, 2025
@arp242
Copy link
Collaborator

arp242 commented Dec 31, 2025

Thanks; merged via #1209; I wanted to update the commit message and tweak the errors, but I can't update this PR since it's created from the fork's master branch (you're still recorded as the author in the git commit).

arp242 added a commit that referenced this pull request Jan 13, 2026
Remove most panics. The only ones remaining are in writeBuf when the
messages are too long (added in #1161). I think that's probably okay:
normal operations should never hit this.
arp242 added a commit that referenced this pull request Jan 13, 2026
Remove most panics. The only ones remaining are in writeBuf when the
messages are too long (added in #1161). I think that's probably okay:
normal operations should never hit this.
arp242 added a commit that referenced this pull request Jan 13, 2026
Remove most panics. The only ones remaining are in writeBuf when the
messages are too long (added in #1161). I think that's probably okay:
normal operations should never hit this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-test Needs a test before it can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants