Opened 7 years ago

Closed 7 years ago

#2786 closed defect (fixed)

DHCPv4: domain-name option must be encoded as text and NOT as FQDN

Reported by: marcin Owned by: marcin
Priority: high Milestone: Sprint-DHCP-20130523
Component: dhcp4 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The DHCP standard options are created using option definitions defined in the std_option_defs.h. The definition for domain-name V4 option is invalid as it specifies that the option carries the FQDN value while it actually carries a string value (RFC2132, section 3.17).

As a result, the server will encode the domain name as FQDN using technique described in section 3.1 of RFC 1035. The client will assume that the domain name value was sent as a pure string and will decode it that way. This will lead to the invalid domain name value on the client's side.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by marcin

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130509

comment:2 Changed 7 years ago by marcin

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

comment:3 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20130509 to Sprint-DHCP-20130523

comment:4 Changed 7 years ago by marcin

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

The domain-name option is now encoded using simple string. Since, there are quite a few options which have similar structure, I also derived new class ''OptionString'' which represents an option carrying a single string value with the length of at least 1 character.

The proposed ChangeLog entry is:

6XX.	[bug]		marcin
	b10-dhcp4: Fixed a bug whereby the domain-name option was encoded
	as FQDN (using technique described in RFC1035) instead of a string.
	Also, created new class which represents an option carrying a single
	string value. This class is now used for all standard options of
	this kind.
	(Trac #2786, git abc)

Please review.

comment:5 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit commit b3c562f2bd85fa9f825d59f8601ba99e04e661f3

src/lib/dhcp/option_string.{cc,h}
General: the implementation stores the string in a class member variable, which duplicates the storage of the base class. As the "string" nature of the class only appears in the {get,set}Value() methods and the string constructor, this seems wasteful. Can't the data be stored in the underlying class and converted to/from a string as needed? (As an aside, it also means that most process can be done by the parent class methods.) Note that this is a suggestion: the amount of code saved is minimal.

setValue/constructor taking a string argument: should check for an empty string and throw an exception if this is the case. (This puts the exception closer to the point of the error.)

Exceptions: suggest that the exception message includes the type code of the option - it will make diagnosis of any problem easier.

src/lib/dhcp/tests/libdhcp++_unittest.cc
unpackOptions4 test: trivial point: "v4Opts+n" should have spaces around the "+". (This appears in several places in this test)

stdOptionDefs4 test: interesting correction. I note that before the change, "end" was equal to "begin", yet apparently the test passed. This suggests that testStdOptionDefs4() will always register a "pass" regardless of the value of the "end" argument, so either:

  • "end" is not really needed as an argument.
  • testStdOptionDefs4() registers a "pass" regardless of its arguments.

It's worth investigating this.

src/lib/dhcp/tests/option_string_unittest.cc
Should include a test for setValue() with an empty string.

Should include a test for trying to construct a string option using a zero-length string.

src/lib/dhcp/tests/pkt4_unittest.cc
unpackOptions test: comment under the "x = pkt->getOption(14)" statement is incorrect - it is option 14 that should exist, not option 13. (A reference to option 13 also occurs in another comment later in that block of code).

ChangeLog entry
This is OK.

comment:7 in reply to: ↑ 6 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit commit b3c562f2bd85fa9f825d59f8601ba99e04e661f3

src/lib/dhcp/option_string.{cc,h}
General: the implementation stores the string in a class member variable, which duplicates the storage of the base class. As the "string" nature of the class only appears in the {get,set}Value() methods and the string constructor, this seems wasteful. Can't the data be stored in the underlying class and converted to/from a string as needed? (As an aside, it also means that most process can be done by the parent class methods.) Note that this is a suggestion: the amount of code saved is minimal.

I have modified the class as suggested.

setValue/constructor taking a string argument: should check for an empty string and throw an exception if this is the case. (This puts the exception closer to the point of the error.)

Done.

Exceptions: suggest that the exception message includes the type code of the option - it will make diagnosis of any problem easier.

Added.

src/lib/dhcp/tests/libdhcp++_unittest.cc
unpackOptions4 test: trivial point: "v4Opts+n" should have spaces around the "+". (This appears in several places in this test)

I added spaces where applicable. Also, ''v4Opts'' is the invalid name for variable. I renamed it to ''v4_opts''.

stdOptionDefs4 test: interesting correction. I note that before the change, "end" was equal to "begin", yet apparently the test passed. This suggests that testStdOptionDefs4() will always register a "pass" regardless of the value of the "end" argument, so either:

  • "end" is not really needed as an argument.
  • testStdOptionDefs4() registers a "pass" regardless of its arguments.

It's worth investigating this.

The ''OptionCustom'' can be created from the buffer (when decoding received packet) or it can be created for the outgoing packet. In the former case, it is desired that the constructor performs validation on the provided buffer to verify that data is consistent, the buffer is not truncated etc. In the latter case, you may want to first create an object and later set the data, e.g. using class modifiers.

OptionCustom class assumed that if the buffer provided in the constructor was empty, the data was supposed to be later set using modifiers and the validation of the buffer was not performed on the constructor level (the latter use case when sending option in an outgoing packet). Thus, it was possible to specify an empty buffer when creating some of the options and pass (successfuly create option instance). This is a little odd and inconsistent with other classes so I modified OptionCustom so as it always validates the provided buffer if it is provided (even when empty). There is one more constructor which doesn't take a buffer argument. If this constructor is used, the instance of the OptionCustom object is created and the default data is set for all option fields. This data can be later modified using modifier functions.

With these changes, the ''libdhcp++_unittest'' will fail if the provided buffer is empty (two the same iterators).

src/lib/dhcp/tests/option_string_unittest.cc
Should include a test for setValue() with an empty string.

Should include a test for trying to construct a string option using a zero-length string.

As a consequence of the changes to setValue, this is necessary. Added.

src/lib/dhcp/tests/pkt4_unittest.cc
unpackOptions test: comment under the "x = pkt->getOption(14)" statement is incorrect - it is option 14 that should exist, not option 13. (A reference to option 13 also occurs in another comment later in that block of code).

Corrected.

ChangeLog entry
This is OK.

comment:8 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit ee886b45eb503cf048abf8416ce7b37430f6fb25

src/lib/dhcp/option_string.{cc,h}
Should try to avoid using the base class member data_. The base class includes the getData() method which returns a reference to data_: that should be used instead.

OptionString::setValue - could call the parent setData() instead of referring to the parent data_ element.

As an aside, when setting the data_ element (which is an OptionBuffer, which in turn is a typedef for std::vector), three methods are used:

a) data_.resize() followed by std::copy (in Option::setData)
b) Creating a temporary vector and copying it into data_ (Option::unpack)
c) Calling data_.assign() Assigning the data in an iterator range to data_ (OptionString::setValue/OptionString::unpack)

The latter is probably the best one to use in all three cases. Option::setData() should use that method, and everything else call Option::setData().

Last edited 7 years ago by stephen (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

Reviewed commit ee886b45eb503cf048abf8416ce7b37430f6fb25

src/lib/dhcp/option_string.{cc,h}
Should try to avoid using the base class member data_. The base class includes the getData() method which returns a reference to data_: that should be used instead.

I now use getData() to get the reference to the data_.

OptionString::setValue - could call the parent setData() instead of referring to the parent data_ element.

It does now.

As an aside, when setting the data_ element (which is an OptionBuffer, which in turn is a typedef for std::vector), three methods are used:

a) data_.resize() followed by std::copy (in Option::setData)
b) Creating a temporary vector and copying it into data_ (Option::unpack)
c) Calling data_.assign() Assigning the data in an iterator range to data_ (OptionString::setValue/OptionString::unpack)

It is now unified and all methods use setData() instead.

The latter is probably the best one to use in all three cases. Option::setData() should use that method, and everything else call Option::setData().

I also slightly modified OptionCustom? class to access or modified the data using parent class procedures.

comment:10 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Reviewed commit e817fc7e97fff519a1e6b7a0ef56fd966a274039

Two trivial changes:

src/lib/dhcp/option.{cc,h}
Copyright date should be updated to 2013.

I don't need to see it again - please merge.

comment:11 Changed 7 years ago by marcin

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

Merged with commit 96b1a7eb31b16bf9b270ad3d82873c0bd86a3530.

Note: See TracTickets for help on using tickets.