Opened 5 years ago

Closed 5 years ago

#3504 closed defect (fixed)

Kea4 does not check if RELEASE was sent from proper location

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea0.9.2-beta
Component: dhcp4 Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Kea4 does not check if the RELEASE was sent from a proper
location. It just checks if there is a lease for a given
client and then accepts it.

This allows for ugly tricks. Spoofing client's release from
any place, not where the client is connected.

There's also one company that is interested in getting
subnet information in lease4_release hook.

The easiest way to implement this is to call selectSubnet()
in Dhcpv4Srv::processRelease() and do checks on the lease.

Subtickets

Change History (25)

comment:1 Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0

comment:2 Changed 5 years ago by tomek

  • Milestone changed from Kea1.0 to Kea0.9.1
  • Version set to git

comment:3 Changed 5 years ago by stephen

  • Component changed from Unclassified to dhcp4

comment:4 Changed 5 years ago by hschempf

  • Milestone changed from Kea0.9.1 to Kea0.9.2

comment:5 Changed 5 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to assigned

interpreted the comment as:

  • call selectSubnet() soon
  • return if !subnet

soon because selectSubnet() should not raise an exception. return without a message because a failed selectSubnet() already did it.
Is it more to do?

Last edited 5 years ago by fdupont (previous) (diff)

comment:6 Changed 5 years ago by fdupont

I move this to the review queue to get an answer to the previous comment.

comment:7 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from assigned to reviewing

comment:8 Changed 5 years ago by fdupont

Ping?

comment:9 Changed 5 years ago by fdupont

Tomek, is my interpretation correct? If it is you can assign the ticket to me?

comment:10 Changed 5 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:11 Changed 5 years ago by tmark

  • Owner changed from tmark to UnAssigned

comment:12 Changed 5 years ago by fdupont

  • Owner changed from UnAssigned to fdupont
  • Status changed from reviewing to accepted

comment:13 Changed 5 years ago by fdupont

Done but keeping the ticket for a possible new unit test.

comment:14 Changed 5 years ago by fdupont

Branch trac3504a (please notice the a) ready for review.

Last edited 5 years ago by fdupont (previous) (diff)

comment:15 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

comment:16 Changed 5 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:17 follow-ups: Changed 5 years ago by marcin

  • Owner changed from marcin to fdupont

Reviewed commit 6aeb50518027b94f1822764c1dd9663e6e8896d5

The commit message:

commit 6aeb50518027b94f1822764c1dd9663e6e8896d5
Author: Francis Dupont <fdupont@isc.org>
Date:   Fri Jan 16 14:32:41 2015 +0100

    [trac3616] proposal

is poor as it says nothing about the actual change. Also, the ticket number is wrong. It may be worth to correct it before pushing to master.

In general, the idea is good and this is how we should handle it. However, this is still some work we should do here.

I have recently implemented some tests for DHCPRELEASE message which are available in the src/bin/dhcp4/tests/release_unittest.cc. They use the Dhcpv6Client class and supersede all old tests for DHCPRELEASE which didn't use the Dhcpv6Client. Your branch was created long time ago and it doesn't include the new tests for release. I suggest that you branch off the latest master, cherry-pick your change and extend the release_unittest.cc with a test that validate your change.

What we also need is a debug message (using the bad_packet logger) which would log that the packet is dropped because it was sent from the wrong location.

Also, ChangeLog entry is required.

comment:18 in reply to: ↑ 17 Changed 5 years ago by fdupont

Lets go for a trac3504b branch (which will solve the commit message issue as a nice side effect).

comment:19 in reply to: ↑ 17 Changed 5 years ago by fdupont

Replying to marcin:

I have recently implemented some tests for DHCPRELEASE message which are available in the src/bin/dhcp4/tests/release_unittest.cc. They use the Dhcpv6Client class and supersede all old tests for DHCPRELEASE which didn't use the Dhcpv6Client. Your branch was created long time ago and it doesn't include the new tests for release. I suggest that you branch off the latest master, cherry-pick your change and extend the release_unittest.cc with a test that validate your change.

=> I disagree because the release_unittests.cc is fully about acquireAndRelease, i.e., the merge of previous ReleaseBasic? (the successful case) and ReleaseReject? (the various (other than from bad location) cases to reject a Release). My concern is the bad location doesn't fit well into this, for instance it doesn't reuse the configuration. Now this doesn't mean the test should not go into this file...

What we also need is a debug message (using the bad_packet logger) which would log that the packet is dropped because it was sent from the wrong location.

=> I am doing this.

Also, ChangeLog entry is required.

=> I am finishing the code first (:-)...

comment:20 Changed 5 years ago by fdupont

Almost done. 3 points:

  • where to put the unit test? (old dhcp4_src_unittest.cc or new release_unittest.cc)
  • I understand why bad_packet_logger is better but there should be a doc explaining what logger to use (here the argument is the lease stuff is not yet involved and the packet is rejected before).
  • any idea for the ChangeLog? Perhaps RELEASE sent from a bad location (i.e., no subnet can be selected)' are dropped.

As soon as the first point is decided I put the test in the file and prepare the trac3504b branch for review.

comment:21 Changed 5 years ago by fdupont

Branch trac3504b ready for re-review. Proposed ChangeLog in my previous comment.
BTW it will be fine when the stat stuff will be merged to check Releases are dropped for the expected reason.

comment:22 Changed 5 years ago by fdupont

  • Owner changed from fdupont to marcin

comment:23 Changed 5 years ago by marcin

  • Owner changed from marcin to fdupont

Reviewed commit 5d4fa4ed17f54c5a34fecd5c09dbf4f4f9dc0c4e

Thanks for addressing review comments.

The outstanding issues (see below) are sufficiently minor that I don't need to see this ticket again if you decide to fix them as I suggest. Otherwise, let's dicuss.

As for the ChangeLog entry, I propose: "DHCPv4 server drops DHCPRELEASE messages sent from a bad location (i.e., no subnet can be selected)."

src/bin/dhcp4/dhcp4_messages.mes
You can't wrap the line of the message to be logged because the text beyond the first new line character is treated as a log message description but is not logged to the console or log file. The message should look like this in the message file:

% DHCP4_RELEASE_FAIL_NO_SUBNET %1: client is trying to release a lease %2 from a subnet which cannot be selected.
This warning message indicates that client tried to release an address
from an improper location. The first argument contains the client and 
transaction identification information. The second argument contains
the address which the client is trying to release.


src/bin/dhcp4/dhcp4_srv.cc
These changes are OK!

src/bin/dhcp4/tests/release_unittest.cc
The test name should be "releaseNoSubnet", rather than "ReleaseNoSubnet"

comment:24 Changed 5 years ago by fdupont

Fixed and merged. Closing...

comment:25 Changed 5 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.