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

Overflow memset in Dhcp.cpp #45

Open
Rotzbua opened this issue Feb 8, 2017 · 2 comments · May be fixed by #216
Open

Overflow memset in Dhcp.cpp #45

Rotzbua opened this issue Feb 8, 2017 · 2 comments · May be fixed by #216
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@Rotzbua
Copy link
Contributor

Rotzbua commented Feb 8, 2017

Overflow risk in Dhcp.cpp.
_dhcpLocalIp has only 4 byte.
Every variable should be filled separate with zeros.

void DhcpClass::reset_DHCP_lease(){
    // zero out _dhcpSubnetMask, _dhcpGatewayIp, _dhcpLocalIp, _dhcpDhcpServerIp, _dhcpDnsServerIp
    memset(_dhcpLocalIp, 0, 20);
}
@gudnimg
Copy link
Contributor

gudnimg commented Jul 2, 2020

This out of bounds access of the buffer is still in the code. The error goes away with this code below, but I haven't tested.

void DhcpClass::reset_DHCP_lease()
{
	// zero out _dhcpSubnetMask, _dhcpGatewayIp, _dhcpLocalIp, _dhcpDhcpServerIp, _dhcpDnsServerIp
	memset(_dhcpSubnetMask, 0, 4);
	memset(_dhcpGatewayIp, 0, 4);
	memset(_dhcpLocalIp, 0, 4);
	memset(_dhcpDhcpServerIp, 0, 4);
	memset(_dhcpDnsServerIp, 0, 4);
}

@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Feb 22, 2022
@technyon technyon linked a pull request Jan 31, 2023 that will close this issue
@technyon
Copy link

Just noticed this when running cppcheck over the code. This code is very dangerous:

  • There's no guarantee that the compiler puts these arrays into sequential memory locations, so this could potentially overwrite unrelated memory locations
  • The compiler is free to change the alignment. So even if the arrays are put into sequential memory locations, there could be added bytes for padding, depending on architecture. That would result in not all bytes being zeroed
  • If someone changes the code unaware of that memset (e. g. adding / removing / moving around some of the arrays), a lot bad stuff happen :)

I've created a PR to fix this.

@per1234 per1234 linked a pull request Jan 31, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants