Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64190

[sashashura]

No description provided.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (34e4b62) 82.71% compared to head (964c7cd) 82.70%.
Report is 1 commits behind head on dev.

Files	Patch %	Lines
Packet++/src/SomeIpSdLayer.cpp	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1240      +/-   ##
==========================================
- Coverage   82.71%   82.70%   -0.02%     
==========================================
  Files         159      159              
  Lines       20329    20338       +9     
  Branches     7687     7693       +6     
==========================================
+ Hits        16816    16821       +5     
+ Misses       2904     2895       -9     
- Partials      609      622      +13     

Flag	Coverage Δ
alpine317	72.31% <71.42%> (-0.02%)	⬇️
centos7	74.47% <71.42%> (-0.02%)	⬇️
fedora37	72.27% <71.42%> (-0.02%)	⬇️
macos-11	61.31% <58.82%> (-0.01%)	⬇️
macos-12	61.37% <58.82%> (-0.01%)	⬇️
macos-ventura	61.34% <58.82%> (-0.02%)	⬇️
mingw32	70.24% <61.53%> (-0.01%)	⬇️
mingw64	70.26% <61.53%> (-0.01%)	⬇️
npcap	83.23% <87.50%> (-0.05%)	⬇️
ubuntu1804	74.89% <71.42%> (-0.06%)	⬇️
ubuntu2004	73.10% <71.42%> (-0.01%)	⬇️
ubuntu2204	72.11% <71.42%> (-0.06%)	⬇️
ubuntu2204-icpx	59.24% <64.70%> (-0.02%)	⬇️
unittest	82.70% <89.47%> (-0.02%)	⬇️
windows-2019	83.27% <87.50%> (-0.02%)	⬇️
windows-2022	83.27% <87.50%> (-0.03%)	⬇️
winpcap	83.25% <87.50%> (-0.02%)	⬇️
xdp	∅ <ø> (∅)
zstd	73.74% <64.70%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sashashura]

I thought the pattern is to reject invalid options instead of allowing to create them and then having multiple checks all over different get methods (and not only getters. Add* methods should also handle the corrupted data).

…

On Fri, Nov 17, 2023, 08:12 seladb ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Packet++/src/SomeIpSdLayer.cpp <#1240 (comment)>: > + size_t offsetOption = sizeof(someipsdhdr) + sizeof(uint32_t) + getLenEntries(data) + sizeof(uint32_t); + size_t lenOptions = getLenOptions(data); + uint32_t len = 0; + while (len < lenOptions) + { + if (len + sizeof(uint16_t) + 3 * sizeof(uint8_t) > lenOptions) + return false; + + uint32_t lenOption = be16toh(*((uint16_t *)(data + offsetOption + len))) + 3 * sizeof(uint8_t); + len += lenOption; + if (len > lenOptions) // the last must be equal to lenOptions + return false; + } it's a bit weird to parse all the options in isDataValid(). Maybe we should fix countOptions() and findOption() to avoid going into non-allocated memory? — Reply to this email directly, view it on GitHub <#1240 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWINCMX4TPGEUYVQBGYXNETYE4E7BAVCNFSM6AAAAAA7MTWO56VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMZWGE3TQOJTGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[seladb]

Yes, usually we prefer to reject invalid layers using the checks in isDataValid(). But in this case it feels like we do too much parsing to understand if the layer is valid, even before we actually create the layer...

[sashashura]

countOptions is called in constructor. As I understand it is a deliberate design of the library not to throw exceptions in constructors and have isDataValid. The question is why countOptions should try to get sense from non-valid data?

OK, ignoring the corrupted tail sounds easy, but there are all kind of fields in the beginning that describe the size of remaining data. Handling such data means to remember to use the "trimmed" sizes instead.

Also this "tolerate and fix" led to #1237. In short the construction of BgpUpdateMessageLayer tolerated incorrect withdrawnRoutes size. Adding new withdrawnRoutes correctly discarded the garbage data and added the new withdrawnRoutes. Clearing the routes correctly trimmed them, except that the final object became without the space for the attributes data size (which was previously assumed to be in the non-parsed data after routes). To summarize "tolerate and fix" leads to construction of an object with ticking bombs where some clear or add operation may lead to invalid memory access.

[seladb]

countOptions is called in constructor. As I understand it is a deliberate design of the library not to throw exceptions in constructors and have isDataValid. The question is why countOptions should try to get sense from non-valid data?

I agree it's probably wrong to do this calculation in the c'tor. A lazy calculation might have been better + making countOptions more resilient to bad data

Also this "tolerate and fix" led to #1237. In short the construction of BgpUpdateMessageLayer tolerated incorrect withdrawnRoutes size. Adding new withdrawnRoutes correctly discarded the garbage data and added the new withdrawnRoutes. Clearing the routes correctly trimmed them, except that the final object became without the space for the attributes data size (which was previously assumed to be in the non-parsed data after routes). To summarize "tolerate and fix" leads to construction of an object with ticking bombs where some clear or add operation may lead to invalid memory access.

I agree that when possible we shouldn't use the "tolerate and fix" approach because of the reasons you mentioned. However, protocol parsing is not always easy, and for some protocols verifying that the data is fully valid would require a very complex logic in isDataValid() which would become as complex as parsing the entire layer, which is what the layer itself is doing.
So we need to find the right balance on which checks are simple enough to include in isDataValid() and which are not, and we should "tolerate" them after the layer is created.

Going back to this specific case - I think the addition to isDataValid() is quite complex and hard to read. So I see 2 options:

• Remove it, don't calculate countOptions in the c'tor and make sure the layer "tolerates" bad data
• Keep this validation in isDataValid() but find a way to make it simpler and more readable, not only by just using comments, but by making the code itself easier to understand. For example: extract pieces to smaller functions and/or reuse some of the code between isDataValid(), countOptions() and findOption(),

Please let me know what you think

[sashashura]

Please take a look.