Opened 7 years ago

Closed 6 years ago

#2749 closed defect (fixed)

kill io_utilities.h or make it safe

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20131015
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

API defined in util/io_utilities.h looks very dangerous: It
reads/writes raw C pointers without any length information. That's a
typical 20C design and is waiting for arbitrary-code execution bugs.

Fortunately it doesn't seem to be used except in unit tests and
some very limited cases of DHCP code, but I suggest we deprecate it or
at least revise using safer primitive (if performance doesn't matter
such as in cases of tests, use C++ vector, for example; if it's
highly performance sensitive, at least add length arg and explicit
range check inside the functions).

Subtickets

Change History (18)

comment:1 follow-up: Changed 7 years ago by vorner

This was always meant for tests only. Doesn't it say so in documentation?

comment:2 in reply to: ↑ 1 Changed 7 years ago by jinmei

Replying to vorner:

This was always meant for tests only. Doesn't it say so in documentation?

No, and, in fact, it's used by production (non test) DHCP code. See,
e.g. dhcp6_srv.cc.

If it was meant to be used for tests only, I also suggest moving it
to lib/util/unittests/ (I'd still suggest doing range check, too,
though).

comment:3 Changed 7 years ago by jinmei

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:4 Changed 7 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-DHCP-20130328

comment:5 Changed 7 years ago by stephen

  • Milestone changed from Sprint-DHCP-20130328 to Next-Sprint-Proposed

comment:6 Changed 7 years ago by shane

So I guess Stephen took the ticket, but then decided that it was more appropriate for the DNS side to look at. I'm happy either way, although possibly it makes more sense for the DHCP team since they are actually using it outside of tests?

comment:7 Changed 7 years ago by jinmei

  • Priority changed from high to medium

comment:8 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Owner set to UnAssigned
  • Status changed from new to reviewing

Up for review.

comment:9 Changed 6 years ago by muks

No ChangeLog entry is necessary as it is an internal API change and update of affected code. This should not be user-visible (unless it uncovers any bugs in the future).

comment:10 Changed 6 years ago by kean

  • Owner changed from UnAssigned to kean

comment:11 Changed 6 years ago by kean

  • Owner changed from kean to muks

Tested (including lettuce) on FC19. Everything seems OK. Since no default argument was set in the header, the mandatory new length will have caught all uses of the functions, so this looks fine and can be merged and closed.

comment:12 Changed 6 years ago by muks

  • Owner changed from muks to UnAssigned

Please can someone from the DHCP team also review all the DHCP related changes that went in for the length argument?

comment:13 Changed 6 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to muks

Reviews commit c7aeb4bec30cf6373e77216058174d6d35850dde

src/bin/dhcp6/dhcp6_srv.cc
generateServerID(): the construct "srvid.size() - n" is used to ensure that the buffer is large enough for the writeUintNN() calls. However, as the preceding line declares the OptionBuffer with a size of at least 8, I think this construct - although general - is unnecessary. I suggest preceding the three write statements with a comment along the lines of "We know that srvid is at least 8 bytes in length, so...". This makes the code marginally faster.

(Note that the assumption that there is enough space in a buffer is made in unpackOptions(), where an explicit length is passed to the readUint16() calls. In that case, the assumption is made in the "while" condition in that the readUint16() functions won't be executed if there is less than four bytes remaining.)

src/lib/asiodns/tests/io_fetch_unittest.cc
Although "sizeof" is a keyword, the general usage elsewhere in BIND 10 appears to be treat it as a function. So the "standard" appears to write it as "sizeof(xxx)". not "sizeof (xxxgdif )". The embedded space should be removed.

src/lib/asiolink/tests/tcp_socket_unittest.cc
See comment for io_fetch_unittest.cc

SequenceTest: to be really generic, TCPCallback::MIN_SIZE could be used as the size argument for writeUint16(), as that is the size to which the data buffer returned by server_cb.data() is set.

src/lib/asiolink/tests/udp_socket_unittest.cc
See comment for io_fetch_unittest.cc

src/lib/dhcp/option.cc
As readUintNN() now checks the size and throws the OutOfRange exception if the size is invalid, the equivalent checks in Option::getUintNN() can be removed.

Although not strictly part of this change, in the setUintNN() functions the argument to the data_.resize() function call could be a "sizeof(datatype)" instead of an explicit literal.

src/lib/dhcp/option4_addrlst.cc
src/lib/dhcp/option6_ia.cc
src/lib/dhcp/option6_iaaddr.cc
src/lib/dhcp/option6_iaprefix.cc
src/lib/dhcp/option_int.h
src/lib/dhcp/option_int_array.h
src/lib/dhcp/option_vendor.cc
I'm not sure that use of "distance" is strictly correct here. The readUintNN() calls require as argument the number of bytes in the buffer, but "distance" returns the number of elements (which may not be equal to the number of bytes) between two pointers. Use of "distance" relies on OptionBufferConstIterator being a pointer to a byte. However, as most of the code here in these files make that (valid) assumption, I don't think it is worth changing.

src/lib/resolve/tests/recursive_query_unittest_2.cc
src/lib/resolve/tests/recursive_query_unittest_3.cc
src/lib/util/io_utilities.h
src/lib/util/tests/io_utilities_unittest.cc
See comment for io_fetch_unittest.cc

comment:15 in reply to: ↑ 14 Changed 6 years ago by muks

  • Owner changed from muks to stephen

Hi Stephen

Here are my replies:

Replying to stephen:

Reviews commit c7aeb4bec30cf6373e77216058174d6d35850dde

src/bin/dhcp6/dhcp6_srv.cc
generateServerID(): the construct "srvid.size() - n" is used to ensure
that the buffer is large enough for the writeUintNN() calls. However,
as the preceding line declares the OptionBuffer with a size of at
least 8, I think this construct - although general - is unnecessary.
I suggest preceding the three write statements with a comment along
the lines of "We know that srvid is at least 8 bytes in length,
so...". This makes the code marginally faster.

This has been updated.

(Note that the assumption that there is enough space in a buffer is
made in unpackOptions(), where an explicit length is passed to the
readUint16() calls. In that case, the assumption is made in the
"while" condition in that the readUint16() functions won't be executed
if there is less than four bytes remaining.)

A comment has been added in that while loop.

src/lib/asiodns/tests/io_fetch_unittest.cc
Although "sizeof" is a keyword, the general usage elsewhere in BIND 10
appears to be treat it as a function. So the "standard" appears to
write it as "sizeof(xxx)". not "sizeof (xxxgdif )". The embedded
space should be removed.

Updated.

src/lib/asiolink/tests/tcp_socket_unittest.cc
See comment for io_fetch_unittest.cc

Updated.

SequenceTest: to be really generic, TCPCallback::MIN_SIZE could be
used as the size argument for writeUint16(), as that is the size to
which the data buffer returned by server_cb.data() is set.

Nod. I thought this would be at least 2 bytes in length, so I put in 2
there. But the buffer size is better.

src/lib/asiolink/tests/udp_socket_unittest.cc
See comment for io_fetch_unittest.cc

Updated.

src/lib/dhcp/option.cc
As readUintNN() now checks the size and throws the OutOfRange
exception if the size is invalid, the equivalent checks in
Option::getUintNN() can be removed.

The appropriate methods were updated.

Although not strictly part of this change, in the setUintNN()
functions the argument to the data_.resize() function call could be a
"sizeof(datatype)" instead of an explicit literal.

This has also been fixed.

src/lib/dhcp/option4_addrlst.cc
src/lib/dhcp/option6_ia.cc
src/lib/dhcp/option6_iaaddr.cc
src/lib/dhcp/option6_iaprefix.cc
src/lib/dhcp/option_int.h
src/lib/dhcp/option_int_array.h
src/lib/dhcp/option_vendor.cc
I'm not sure that use of "distance" is strictly correct here. The
readUintNN() calls require as argument the number of bytes in the
buffer, but "distance" returns the number of elements (which may not
be equal to the number of bytes) between two pointers. Use of
"distance" relies on OptionBufferConstIterator being a pointer to a
byte. However, as most of the code here in these files make that
(valid) assumption, I don't think it is worth changing.

For this point, I didn't make any changes to the code.

src/lib/resolve/tests/recursive_query_unittest_2.cc
src/lib/resolve/tests/recursive_query_unittest_3.cc
src/lib/util/io_utilities.h
src/lib/util/tests/io_utilities_unittest.cc
See comment for io_fetch_unittest.cc

Updated.

comment:16 Changed 6 years ago by stephen

  • Owner changed from stephen to muks

Reviewed commit 1685d98e142c2b5915d19bf17f4029459a922321

src/bin/dhcp6/dhcp6_srv.cc
generateServerID(): changes are OK, but when reviewing the code it occurred to me that if the "iface->getMacLen()" call returns zero, the target address of the memcpy() is invalid. Admittedly the memcpy() should not copy anything because the length will be zero, but a StackOverflow answer here quotes the C standard indicating that arguments to memcpy() must always be valid, even if the size argument is zero.

However, looking at the code in detail reveals that earlier in the function the MAC length is checked against MIN_MAC_LEN. (Also, isRangeZero() is called which returns "true" if the MAC comprises entirely of zeroes or if the length it zero.) I've changed the comment from "at least 8 bytes" to "more than 8 bytes".

If you're happy with this change, please merge.

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

comment:17 Changed 6 years ago by muks

The change looks fine. We could use &srvid.at(8) (which would throw if it was out of bounds) instead of &srvid[8], but I guess for this use-case it's unnecessary as multiple checks are done in this codepath.

comment:18 Changed 6 years ago by muks

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

Merged to master branch in commit f70ff164d003111eeb20a83f13073ab9a9a4a2b7:

* 6b3bc78 [2749] Minor comment change.
* 1685d98 [2749] Update resize() call arguments to be sizeof instead of literal values
* 04f5b14 [2749] Remove redundant checks
* 9eedd90 [2749] Update name of test (fix case)
* 99fe38e [2749] Update length argument
* a071815 [2749] Remove space between sizeof and (
* 44b8509 [2749] Replace size() calls with literal values
* 45809d9 [2749] Add comment that we know there's room available in the buffer
* c7aeb4b [2749] Update rest of tree to use the modified io_utilities.h API
* d584332 [2749] Check sizes in memory access functions in io_utilities.h

Resolving as fixed. Thank you for the reviews Stephen.

Note: See TracTickets for help on using tickets.