Add verification of the range of integer values for TLVReader -- 2nd attempt #5764

bitkis · 2021-04-02T04:48:47Z

Problem

Currently, TVLReader's method Get does not validate sizes of requested integers. In case when the size is less than size of TVL field, incorrect value is returned to caller; no error condition is indicated.

Summary of Changes

Size validation is added, error conditions are handled properly.

Fixes #5152
[see https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/2115 and https://github.com//pull/5286 discussions]

project-chip#5152

github-actions · 2021-04-02T05:28:43Z

Size increase report for "esp32-example-build" from f3ec8e6

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	72	72

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,3391
.debug_str,0,1701
.debug_line,0,949
.debug_loc,0,945
.debug_ranges,0,248
.flash.text,72,72
.debug_abbrev,0,40
.debug_frame,0,24
.debug_aranges,0,8
.strtab,0,1
.shstrtab,0,-1
.xt.prop._ZN4chip8Encoding22PacketBufferWriterBaseINS0_12LittleEndian12BufferWriterEEC2EONS_6System18PacketBufferHandleEj,0,-2

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

github-actions · 2021-04-02T06:00:12Z

Size increase report for "nrfconnect-example-build" from f3ec8e6

File	Section	File	VM
chip-lighting.elf	text	60	60
chip-lighting.elf	device_handles	4	4
chip-lock.elf	text	60	60
chip-lock.elf	device_handles	4	4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,3375
.debug_str,0,1701
.debug_loc,0,939
.debug_line,0,397
.debug_ranges,0,176
.debug_frame,0,84
text,60,60
.debug_aranges,0,8
device_handles,4,4
.strtab,0,1
.shstrtab,0,-1

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_loc,0,-16

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,3375
.debug_str,0,1701
.debug_loc,0,927
.debug_line,0,401
.debug_ranges,0,176
.debug_frame,0,84
text,60,60
.debug_aranges,0,8
device_handles,4,4
.strtab,0,1
.shstrtab,0,-1

Damian-Nordic · 2021-04-02T11:28:10Z

src/lib/core/CHIPTLVReader.cpp

+    return err;
+}
+
+CHIP_ERROR TLVReader::_Get(uint64_t & v)


nit: wouldn't having two methods for signed and unsigned types, returning int64_t and uint64_t, respectively, result in a simpler code?

bzbarsky-apple · 2021-04-03T04:33:51Z

src/lib/core/CHIPTLV.h

+    template<typename int_type>
+    bool TLVTypeValid(uint64_t val64) const;
+    CHIP_ERROR _Get(uint64_t & v);


These need to be documented.

bzbarsky-apple · 2021-04-03T04:38:49Z

src/lib/core/CHIPTLVReader.cpp

+
+    CHIP_ERROR err = _Get(v64);
+    ReturnErrorCodeIf(!TLVTypeValid<int8_t>(v64), CHIP_ERROR_WRONG_TLV_TYPE);
+    v = CastToSigned(static_cast<uint8_t>(v64));


Suggested change

v = CastToSigned(static_cast<uint8_t>(v64));

v = static_cast<int8_t>(CastToSigned(v64));

is clearer. But even clearer would be to have an overload of _Get that takes int64_t and checks for the SignedInteger() type before returning success. Then here we could just CanCastTo and return with a cast...

Similar comments for all the signed types.

This comment is still unaddressed.

bzbarsky-apple · 2021-04-03T04:43:44Z

src/lib/core/tests/TestCHIPTLV.cpp

+    TEST_GET_NOERROR(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<int32_t>(INT8_MIN));
+    TEST_GET_NOERROR(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<int64_t>(INT8_MIN));
+
+    TEST_GET(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<uint8_t>(INT8_MIN), CHIP_ERROR_WRONG_TLV_TYPE);


Should probably check that INT8_MAX can't be read as uint8_t (below), since that one would actually be in range.

bzbarsky-apple · 2021-04-03T04:44:21Z

src/lib/core/tests/TestCHIPTLV.cpp

+    TEST_GET_NOERROR(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<int16_t>(INT8_MAX));
+    TEST_GET_NOERROR(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<int32_t>(INT8_MAX));
+    TEST_GET_NOERROR(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<int64_t>(INT8_MAX));
+


Should probably check that can't read as uint64_t, and same for all the signed max values.

bzbarsky-apple · 2021-04-03T04:46:01Z

src/lib/core/tests/TestCHIPTLV.cpp

+
+    TestNext<TLVReader>(inSuite, reader);
+
+    TEST_GET(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<int8_t>(INT16_MIN), CHIP_ERROR_WRONG_TLV_TYPE);


I believe that cast is undefined behavior... Should use 0 instead of INT16_MIN there, perhaps? Similar for the other to-signed narrowing casts.

bzbarsky-apple · 2021-04-03T04:48:59Z

src/lib/core/tests/TestCHIPTLV.cpp

@@ -485,11 +639,11 @@ void ReadEncoding1(nlTestSuite * inSuite, TLVReader & reader)

        TestNext<TLVReader>(inSuite, reader2);

-        TestGet<TLVReader, bool>(inSuite, reader2, kTLVType_Boolean, ProfileTag(TestProfile_1, 2), true);


Why all the changes away from using TestGet?

All Test*() inline functions in this file have the same issue: they are functions, and, therefore, the assertion macros in those functions get expended before the functions are called/inlined. As the result, shown locations of assertion failures have nothing to do with the locations of actual problems.

The reason I have not completely removed TestGet was that for some reason TEST_GET macro had a problem with float/double types -- still need to figure out what is wrong.

As the result, shown locations of assertion failures have nothing to do with the locations of actual problems.

Ah, right. Would have been handy to put that in the commit message. ;)

stale · 2021-04-15T19:43:38Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

woody-apple · 2021-04-21T17:03:20Z

@bitkis Any update on the PR feedback?

stale · 2021-04-28T19:40:00Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2021-05-05T23:00:36Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

woody-apple

Per the updated template, can you update the PR here?

#### Problem
What is being fixed?  Examples:
* Fix crash on startup
* Fixes #12345 12345 Frobnozzle is leaky (exactly like that, so GitHub will auto-close the issue).

#### Change overview
What's in this PR

#### Testing
How was this tested? (at least one bullet point required)
    • If unit tests were added, how do they cover this issue?
    • If unit tests existed, how were they fixed/modified to prevent this in future?
    • If integration tests were added, how do they verify this change?
    • If manually tested, what platforms controller and device platforms were manually tested, and how?
    • If no testing is required, why not?

bzbarsky-apple

Still need to address issues raised in #5764 (review)

bzbarsky-apple · 2021-05-22T07:22:22Z

src/lib/core/CHIPTLV.h

+    bool TLVTypeValid(uint64_t val64) const;
+    CHIP_ERROR _Get(uint64_t & v);


These still need to be documented.

bzbarsky-apple · 2021-05-22T07:22:52Z

src/lib/core/CHIPTLVReader.cpp

+
+    CHIP_ERROR err = _Get(v64);
+    ReturnErrorCodeIf(!TLVTypeValid<int8_t>(v64), CHIP_ERROR_WRONG_TLV_TYPE);
+    v = CastToSigned(static_cast<uint8_t>(v64));


This comment is still unaddressed.

stale · 2021-05-29T19:59:54Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2021-06-05T20:00:35Z

This stale pull request has been automatically closed. Thank you for your contributions.

… unexpected values. Two changes: 1) Ensure the value was encoded with the signed-ness the reader expects. 2) Ensure the value is in the range the reader expects instad of ending up with overflow and whatever that does.. Fixes project-chip#5152 The test changes were largely lifted from project-chip#5764

… unexpected values. (#9944) Two changes: 1) Ensure the value was encoded with the signed-ness the reader expects. 2) Ensure the value is in the range the reader expects instad of ending up with overflow and whatever that does.. Fixes #5152 The test changes were largely lifted from #5764

Add verification of the range of integer values for TLVReader. Resolves

14d8c4a

project-chip#5152

boring-cyborg bot added the lib label Apr 2, 2021

bitkis mentioned this pull request Apr 2, 2021

Add verification of the range of integer values for TLVReader. #5286

Closed

Restyled by clang-format

faa9923

restyled-io bot mentioned this pull request Apr 2, 2021

Restyle Add verification of the range of integer values for TLVReader -- 2nd attempt #5765

Closed

bitkis requested review from bzbarsky-apple, rwalker-apple, mspang, andy31415 and hawk248 April 2, 2021 04:53

Damian-Nordic reviewed Apr 2, 2021

View reviewed changes

Damian-Nordic approved these changes Apr 2, 2021

View reviewed changes

bzbarsky-apple requested changes Apr 3, 2021

View reviewed changes

pullapprove bot requested review from chrisdecenzo, jmartinez-silabs and saurabhst April 8, 2021 17:02

stale bot added the stale Stale issue or PR label Apr 15, 2021

stale bot removed the stale Stale issue or PR label Apr 21, 2021

stale bot added the stale Stale issue or PR label May 5, 2021

woody-apple requested a review from bzbarsky-apple May 10, 2021 18:39

stale bot removed the stale Stale issue or PR label May 10, 2021

pullapprove bot added the review - pending label May 18, 2021

woody-apple requested changes May 20, 2021

View reviewed changes

bzbarsky-apple requested changes May 22, 2021

View reviewed changes

stale bot added the stale Stale issue or PR label May 29, 2021

stale bot closed this Jun 5, 2021

bzbarsky-apple mentioned this pull request Sep 23, 2021

Fix TLV test to produce more useful failure output. #9934

Merged

bzbarsky-apple mentioned this pull request Sep 24, 2021

Ensure that attempts to read integers from TLV don't silently produce unexpected values. #9944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verification of the range of integer values for TLVReader -- 2nd attempt #5764

Add verification of the range of integer values for TLVReader -- 2nd attempt #5764

bitkis commented Apr 2, 2021

github-actions bot commented Apr 2, 2021

github-actions bot commented Apr 2, 2021

Damian-Nordic Apr 2, 2021

bzbarsky-apple Apr 3, 2021

bzbarsky-apple Apr 3, 2021

bzbarsky-apple May 22, 2021

bzbarsky-apple Apr 3, 2021

bzbarsky-apple Apr 3, 2021

bzbarsky-apple Apr 3, 2021

bzbarsky-apple Apr 3, 2021

bitkis Apr 3, 2021

bzbarsky-apple Apr 3, 2021

stale bot commented Apr 15, 2021

woody-apple commented Apr 21, 2021

stale bot commented Apr 28, 2021

stale bot commented May 5, 2021

woody-apple left a comment

bzbarsky-apple left a comment

bzbarsky-apple May 22, 2021

bzbarsky-apple May 22, 2021

stale bot commented May 29, 2021

stale bot commented Jun 5, 2021

	v = CastToSigned(static_cast<uint8_t>(v64));
	v = static_cast<int8_t>(CastToSigned(v64));


		TestNext<TLVReader>(inSuite, reader);

		TEST_GET(inSuite, reader, kTLVType_SignedInteger, AnonymousTag, static_cast<int8_t>(INT16_MIN), CHIP_ERROR_WRONG_TLV_TYPE);

		@@ -485,11 +639,11 @@ void ReadEncoding1(nlTestSuite * inSuite, TLVReader & reader)

		TestNext<TLVReader>(inSuite, reader2);

		TestGet<TLVReader, bool>(inSuite, reader2, kTLVType_Boolean, ProfileTag(TestProfile_1, 2), true);

		bool TLVTypeValid(uint64_t val64) const;
		CHIP_ERROR _Get(uint64_t & v);

Add verification of the range of integer values for TLVReader -- 2nd attempt #5764

Add verification of the range of integer values for TLVReader -- 2nd attempt #5764

Conversation

bitkis commented Apr 2, 2021

Problem

Summary of Changes

github-actions bot commented Apr 2, 2021

github-actions bot commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Apr 15, 2021

woody-apple commented Apr 21, 2021

stale bot commented Apr 28, 2021

stale bot commented May 5, 2021

woody-apple left a comment

Choose a reason for hiding this comment

bzbarsky-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented May 29, 2021

stale bot commented Jun 5, 2021