diff mbox

dhcp: check if the DHCPOFFER matches our MAC

Message ID 1469591389-9033-1-git-send-email-nikunj@linux.vnet.ibm.com
State Rejected
Headers show

Commit Message

Nikunj A Dadhania July 27, 2016, 3:49 a.m. UTC
Add missing check to see that the IP offered is for this mac address.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 clients/net-snk/app/netlib/dhcp.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Thomas Huth July 31, 2016, 4:41 p.m. UTC | #1
On 27.07.2016 05:49, Nikunj A Dadhania wrote:
> Add missing check to see that the IP offered is for this mac address.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  clients/net-snk/app/netlib/dhcp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/clients/net-snk/app/netlib/dhcp.c b/clients/net-snk/app/netlib/dhcp.c
> index 7e2e88c..3f45633 100644
> --- a/clients/net-snk/app/netlib/dhcp.c
> +++ b/clients/net-snk/app/netlib/dhcp.c

Please note that this file has recently been moved to lib/libnet/ instead.

> @@ -865,6 +865,8 @@ int8_t handle_dhcp(int fd, uint8_t * packet, int32_t packetsize)
>  		switch (dhcp_state) {
>  		case DHCP_STATE_SELECT :
>  			if (opt.msg_type == DHCPOFFER) {
> +				if(memcmp(btph->chaddr, get_mac_address(), 6))
> +					break;
>  				dhcp_own_ip = htonl(btph -> yiaddr);
>  				dhcp_server_ip = opt.server_ID;
>  				dhcp_send_request(fd);
> 

Checking the MAC here should be fine, I think. I'm just wondering: Did
you encounter a real world problem here, or did you just find this by
reading the sources? The SLOF code already checks the XID for received
packets, so that should already give a basic protection against wrongly
received broadcast DHCPOFFER messages, shouldn't it? Anyway, let's be
better safe than sorry, so including this additional check is certainly
a good idea!

 Thomas
Nikunj A Dadhania July 31, 2016, 5:13 p.m. UTC | #2
Thomas Huth <thuth@redhat.com> writes:

> On 27.07.2016 05:49, Nikunj A Dadhania wrote:
>> Add missing check to see that the IP offered is for this mac address.
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  clients/net-snk/app/netlib/dhcp.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/clients/net-snk/app/netlib/dhcp.c b/clients/net-snk/app/netlib/dhcp.c
>> index 7e2e88c..3f45633 100644
>> --- a/clients/net-snk/app/netlib/dhcp.c
>> +++ b/clients/net-snk/app/netlib/dhcp.c
>
> Please note that this file has recently been moved to lib/libnet/ instead.

Sure, will update.

>> @@ -865,6 +865,8 @@ int8_t handle_dhcp(int fd, uint8_t * packet, int32_t packetsize)
>>  		switch (dhcp_state) {
>>  		case DHCP_STATE_SELECT :
>>  			if (opt.msg_type == DHCPOFFER) {
>> +				if(memcmp(btph->chaddr, get_mac_address(), 6))
>> +					break;
>>  				dhcp_own_ip = htonl(btph -> yiaddr);
>>  				dhcp_server_ip = opt.server_ID;
>>  				dhcp_send_request(fd);
>> 
>
> Checking the MAC here should be fine, I think. I'm just wondering: Did
> you encounter a real world problem here, or did you just find this by
> reading the sources?

An issue was reported to me, though the tester hasnt been able to
recreate the issue. I had this patch queued for him to test.

> The SLOF code already checks the XID for received packets, so that
> should already give a basic protection against wrongly received
> broadcast DHCPOFFER messages, shouldn't it?

Oh right and I have verified that this patch is missing in that tree.

> Anyway, let's be better safe than sorry, so including this additional
> check is certainly a good idea!

Was there with me for few weeks until I could get a confirmation that
this indeed is the fix. Then thought of sending it on the list for
review !

Regards
Nikunj
diff mbox

Patch

diff --git a/clients/net-snk/app/netlib/dhcp.c b/clients/net-snk/app/netlib/dhcp.c
index 7e2e88c..3f45633 100644
--- a/clients/net-snk/app/netlib/dhcp.c
+++ b/clients/net-snk/app/netlib/dhcp.c
@@ -865,6 +865,8 @@  int8_t handle_dhcp(int fd, uint8_t * packet, int32_t packetsize)
 		switch (dhcp_state) {
 		case DHCP_STATE_SELECT :
 			if (opt.msg_type == DHCPOFFER) {
+				if(memcmp(btph->chaddr, get_mac_address(), 6))
+					break;
 				dhcp_own_ip = htonl(btph -> yiaddr);
 				dhcp_server_ip = opt.server_ID;
 				dhcp_send_request(fd);