Opened 4 years ago

Closed 4 years ago

#4306 closed defect (fixed)

overflow in pkt4::setFile()

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: Unclassified 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: .5 Internal?: no

Description

the call to fill at the last line of pkt4::setFile() overflows when fileLen == MAX_FILE_LEN.
Reported by Coverity so inherits from #4252.

Subtickets

Change History (15)

comment:1 Changed 4 years ago by fdupont

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

comment:2 Changed 4 years ago by fdupont

BTW there is a similar bug in pkt4::setName().

comment:3 Changed 4 years ago by fdupont

CID 1202663 and 1202665.

comment:4 Changed 4 years ago by fdupont

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

Done. Ready for review.

comment:5 Changed 4 years ago by hschempf

  • Milestone changed from Kea1.1 to Kea-proposed

This ticket needs to be reviewed by the team before it's added to 1.1, sending it to Kea-proposed.

comment:6 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

Per team meeting 3/10, accept 1.1. estimate = .5 days

comment:7 Changed 4 years ago by hschempf

This ticket fits within the effort of bug fixing

comment:8 Changed 4 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:9 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont
  • Total Hours changed from 0 to .25

Reviewed commits 898da677583b7cf878085af393a394dd89a12cca to 154f8fa1ed4aa1b37ef1bd0f4d1081c34cde5ffa

The changes are correct.

However, the original code is deficient in that there are no unit tests to check the copying of the name. We need a test that checks that copying (MAX_XXX_LEN - n) and MAX_XXX_LEN characters work successfully and that attempting to copy a string longer than MAX_XXX_LEN causes an OutOfRange exception to be thrown.

There should also be a ChangeLog entry noting that the overflow detected by Coverity has been corrected.

comment:10 in reply to: ↑ 9 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

Replying to stephen:

However, the original code is deficient in that there are no unit tests to check the copying of the name. We need a test that checks that copying (MAX_XXX_LEN - n) and MAX_XXX_LEN characters work successfully and that attempting to copy a string longer than MAX_XXX_LEN causes an OutOfRange exception to be thrown.

=> do you mean I should add these new unit tests?

comment:11 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Yes - you're adding code checking the behavior when the passed string size is equal to the buffer length, so there should be some unit tests to check that. (Even though the original code did not have those unit tests.)

comment:12 in reply to: ↑ 11 Changed 4 years ago by fdupont

Replying to stephen:

Yes - you're adding code checking the behavior when the passed string size is equal to the buffer length, so there should be some unit tests to check that. (Even though the original code did not have those unit tests.)

=> if the original wrong code had such a test IMHO a dynamic checker should have detected the error before...
So I agree a new unit test is required against regression. Doing this...

comment:13 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

Done

comment:14 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont
  • Total Hours changed from .25 to .5

Reviewed commit da131a9b8dd30bf8e8a23d4a62319066c51f4952

You might want to add a comment to the "too long argument generates exception" tests to state that the test is only checking the argument length, so the contents of the input array are not relevant (which is why the array is uninitialized). However, I don't insist on that.

All OK, please merge.

comment:15 Changed 4 years ago by fdupont

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

Merged (with the proposed comments), closing.

Note: See TracTickets for help on using tickets.