Add non-copying string and byte getters on a TLV reader.

[bzbarsky-apple]

We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate. But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1. Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2. Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes #9009

Problem

See above.

Change overview

See above.

Testing

Added unit tests for the new readers. I haven't found a good way to test the buggy codepaths being fixed: for retrieveOptionalInfoString we would need to feed it invalid TLV data, and AdditionalDataPayloadParser is completely untested (tracked in #9248).

[woody-apple]

@bzbarsky-apple Looks like a real build failure

[andy31415]

Marking request changes to understand the separation - on second consideration, the new code also creates spans using GetLength() as the previous code, so actual length is identical as before, except the benefit of not really allocating it but just creating a view.

Following that line of reasoning, these methods could work on TLVReader as before or we may not even need the methods and just call GetDataPtr/GetLength in the places in setup payload as needed (stop allocating temporary storage space)

[bzbarsky-apple]

the new code also creates spans using GetLength() as the previous code

But only after calling a method (GetDataPtr, see comment on that line) that validates that the length is at least sane in the sense of not overrunning the available buffer. That is a key difference.

these methods could work on TLVReader as before

Sure thing. If you know enough about the state of your reader. The new setup just delegates that state verification to the compiler, which is less error-prone than having humans do it.

just call GetDataPtr/GetLength in the places in setup payload as needed (stop allocating temporary storage space)

Yes, we could absolutely do that. The drawbacks there are that it's non-obvious code to write, so people are not likely to write it by default. Instead they reach for GetString/GetBytes, which have a nicer API and very non-obvious pitfalls.

I initially started with an attempt to just remove GetString and GetBytes altogether, because they are so easy to misuse. That ran into problems with the non-contiguous reader cases, where GetString and GetBytes cannot be easily replaced with in-place views because the data is not all there all at once. I am still thinking about what we can actually do with GetString and GetBytes to make them safe to use, if it's even possible in general, but decided to not block on solving that problem before fixing the memory-safety issues we have with their current consumers....

I am open to other API shapes here, by the way; what we have so far is not perfect by any means. It's a bit of a compromise between expediency and redesigning the entire TLV reader setup wholesale.

[andy31415]

Approved based on comments. I wish we had some resources to make TLV better - I think it went too far into trying to optimize backing store at the expense of safety and I am not convinced the fancy backing store is actually that used.

[bzbarsky-apple]

@msandstedt @saurabhst @LuDuda @jepenven-silabs @jmartinez-silabs Please take a look?

[github-actions]

Size increase report for "esp32-example-build" from 488a3b0

File	Section	File	VM
chip-bridge-app.elf	.flash.text	48	48
chip-temperature-measurement-app.elf	.flash.text	60	60
chip-shell.elf	.flash.text	56	56
chip-lock-app.elf	.flash.text	68	68

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,1555
.debug_str,0,891
.debug_line,0,561
.debug_frame,0,72
.debug_ranges,0,48
.flash.text,48,48
.debug_abbrev,0,26
.debug_aranges,0,24
.debug_loc,0,3
[Unmapped],0,-48

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_info,0,1555
.debug_str,0,893
.debug_line,0,551
.debug_loc,0,363
.debug_frame,0,72
.flash.text,60,60
.debug_ranges,0,48
.debug_abbrev,0,26
.debug_aranges,0,24
[Unmapped],0,-60

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

sections,vmsize,filesize

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

sections,vmsize,filesize

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_info,0,1555
.debug_str,0,883
.debug_line,0,561
.debug_loc,0,267
.debug_frame,0,72
.flash.text,56,56
.debug_ranges,0,48
.debug_abbrev,0,26
.debug_aranges,0,24
[Unmapped],0,-56

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

sections,vmsize,filesize
.debug_info,0,1555
.debug_str,0,891
.debug_line,0,561
.debug_loc,0,219
.debug_frame,0,72
.flash.text,68,68
.debug_ranges,0,48
.debug_abbrev,0,26
.debug_aranges,0,24
[Unmapped],0,-68

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

sections,vmsize,filesize
.debug_info,0,1340
.debug_str,0,892
.debug_line,0,415
.debug_frame,0,100
.debug_loc,0,58
.debug_ranges,0,48
.debug_abbrev,0,26
.debug_aranges,0,24
.riscv.attributes,0,1