Skip to content
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.

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

Jump to bottom

Incorrect length checks and comments in content protocol implementation #6449

Closed
DorpsGek opened this issue Apr 19, 2016 · 4 comments
Closed

Incorrect length checks and comments in content protocol implementation #6449

DorpsGek opened this issue Apr 19, 2016 · 4 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

joepie91 opened the ticket and wrote:

Exact OpenTTD revision I've checked this against: r27546

While reverse-engineering the content server protocol from the OpenTTD source code, I ran across two likely bugs in the code, both in src/network/network_content.cpp:

  1. void ClientNetworkContentSocketHandler::RequestContentList(ContentVector *cv, bool send_md5sum)

The second length assertion here assumes that a single item is either 20 or 4 bytes depending on whether an MD5 checksum is included, but this is incorrect; it's 21 or 5 bytes instead, due to the additional uint8 being written for the ContentType.

  1. void ClientNetworkContentSocketHandler::RequestContentList(uint count, const ContentID *content_ids)

The comments claim the following:

A packet begins with the packet size and a byte for the type. Then this packet adds a byte for the content type and a uint16 for the count in this packet.

This is incorrect; there's no byte for the ContentType, just a uint16 for the item count. The `p_count` calculation also appears to incorrectly assume the same non-existent byte to exist.

---

I don't speak C++ myself, so I'm unfortunately unable to contribute a patch.

Reported version: trunk
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/6449
@DorpsGek
Copy link
Member Author

joepie91 wrote:

Not sure it's worth opening a new bug for this, so... another potential bug I ran across:

---
bool ClientNetworkContentSocketHandler::Receive_SERVER_INFO(Packet *p)
{
[...]
p->Recv_string(ci->version, lengthof(ci->name));
---

That should probably be lengthof(ci->version) instead.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6449#comment14164

@DorpsGek
Copy link
Member Author

frosch wrote:

Does the attached match your analysis?

About the fixes themself:
* The ci->name -> ci->version mixup is non-critical. Within ContextInfo "version" is followed by "url", which is the read just after. So the buffer overrun cannot cause any harm.
* The p_count miscomputation only affects performance.
* The assertion about cv->Length would have been backed up by later assertions in Packet methods.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/6449#comment14165

@DorpsGek
Copy link
Member Author

joepie91 wrote:

Apologies for the slow response. From a quick glance, those changes look correct to me.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6449#comment14168

@DorpsGek
Copy link
Member Author

frosch closed the ticket.

Reason for closing: Fixed

in r27570

This comment was imported from FlySpray: https://bugs.openttd.org/task/6449

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) Network labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek