Opened 7 months ago

Closed 6 months ago

#5551 closed defect (complete)

Kea drops DHCPv4 packet when truncated vendor option is encountered

Reported by: tomek Owned by: tmark
Priority: high Milestone: Kea1.4
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

One user dealing is experiencing problems with cable modems. The CM sends a packet that Kea interprets as truncated vendor option and drops the packet.

We do have traffic capture and logs. Things to consider here:

  • dropping whole packet is not the way to go. the client will keep sending its broken packet over and over and the subscriber using it will be unhappy
  • if the vendor option being sent is really broken, we should try to ignore just the sub-option, or failing that, the whole vendor option.
  • we have a traffic capture and a log file. Both are big and it will take some effort to figure out which packets are causing problems. It will likely be caused either by one device over and over or perhaps several devices of the same type.

The error message is DHCP4_PACKET_DROP_0001 failed to parse packe from 1.2.3.4 to 5.6.7.8, received over interface foo0, reason: Attempt to parse truncated vendor option.

Location of the files is specified in support ticket 12392.

Subtickets

Change History (9)

comment:1 Changed 7 months ago by tmark

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

comment:2 Changed 7 months ago by tmark

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

Ticket is ready for review.

I added a new exception type that option unpack() methods can call to signify to calling layers that they should stop unpacking any remaining options. kea-dhcp4 now explicitly catches this exception type, logs it and allows the packet processing to continue.

comment:3 Changed 7 months ago by marcin

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

comment:4 Changed 7 months ago by marcin

  • Status changed from accepted to reviewing

comment:5 follow-up: Changed 7 months ago by marcin

  • Owner changed from marcin to tmark

I reviewed commit f202ec600b358897bda88f9f9c119b98c7b19a8e.

I added a missing dot in the new log message description, so please pull.

src/lib/dhcp/option.h
I think the description of the SkipRemainingOptionsError description should be more specific when it is thrown.

src/lib/dhcp/option_definition.cc
src/lib/dhcp/option_vendor.cc
Your changes are fine to me, but I wonder if they are sufficient. The vendor option contains a bunch of sub-options which are unpacked with LibDHCP::unpackVendorOptionsX(). If the suboptions are malformed (truncated) shouldn't it throw the same exception, i.e. SkipRemaininOptionsError?

src/lib/dhcp/option_vendor.h
It may be useful to slightly extend the class (or constructor) description to say that this option throws a special type of exception to allow the caller to capture the truncated packet data.

src/lib/dhcp/tests/pkt4_unittest.cc
Shouldn't we have similar test for DHCPv4 case? Maybe for the current code it is ok to just test v4 case, but if we extend this to suboptions (as suggested above), the DHCPv6 specific test will become more important.

comment:6 in reply to: ↑ 5 Changed 7 months ago by tmark

Replying to marcin:

I reviewed commit f202ec600b358897bda88f9f9c119b98c7b19a8e.

I added a missing dot in the new log message description, so please pull.

Thx

src/lib/dhcp/option.h
I think the description of the SkipRemainingOptionsError description should be more specific when it is thrown.

Done

src/lib/dhcp/option_definition.cc
src/lib/dhcp/option_vendor.cc
Your changes are fine to me, but I wonder if they are sufficient. The vendor option contains a bunch of sub-options which are unpacked with LibDHCP::unpackVendorOptionsX(). If the suboptions are malformed (truncated) shouldn't it throw the same exception, i.e. SkipRemaininOptionsError?

I changed the functions to throw SkipRemaingingOptionsError? except where they are detecting errors in the option definitions. I left those as they indicate a configuration issue.

src/lib/dhcp/option_vendor.h
It may be useful to slightly extend the class (or constructor) description to say that this option throws a special type of exception to allow the caller to capture the truncated packet data.

Added more commentary to the unpack method.


src/lib/dhcp/tests/pkt4_unittest.cc
Shouldn't we have similar test for DHCPv4 case? Maybe for the current code it is ok to just test v4 case, but if we extend this to suboptions (as suggested above), the DHCPv6 specific test will become more important.

Extended support for skip to kea-dhcp6 and added tests for it.

Please re-review

comment:7 Changed 7 months ago by tmark

  • Owner changed from tmark to marcin

comment:8 Changed 7 months ago by marcin

  • Owner changed from marcin to tmark

I reviewed commit b791aaefdf28777b64b5042609d081b05e3365d8.

Unit tests passed on macOS 10.12.6.

I fixed some little errors so please pull. This change requires a changelog entry.

comment:9 Changed 6 months ago by tmark

  • Resolution set to complete
  • Status changed from reviewing to closed

Thanks for the corrections.
Changes merged with git 59ef33ee17672c55cee4ec86ff59737b361a3c21.
Added ChangeLog? entry 1373.

ticket is complete.

Note: See TracTickets for help on using tickets.