p2p: fix CSubNet::ToString() UBSan and banman fuzz crash #23197

jonatack commented at 7:41 PM on October 5, 2021: member

If an invalid subnet netmask is encountered and NetmaskBits() returns -1, it could be written to uint8_t cidr in CSubNet::ToString() with an implicit conversion to 255 instead of exiting.

Solution: in the case of an invalid subnet netmask, break to exit the loop like when the netmask bit is 0 in the preceding conditional.

Fixes this banman fuzzer crash and allows dropping the suppression:

https://cirrus-ci.com/task/4787303177519104?logs=ci#L3020

SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation netaddress.cpp:1190:18 in
MS: 0 ; base unit: 0000000000000000000000000000000000000000
artifact_prefix='./'; Test unit written to ./crash-0671aac15e619e99522e2119487eaa9cc97e5a34

netaddress.cpp:1190:18: runtime error: implicit conversion
  from type 'int' of value -1 (32-bit, signed) to type 'uint8_t' (aka 'unsigned char') changed the value to 255 (8-bit, unsigned)

p2p: fix ubsan implicit conversion in CSubNet::ToString()

where the value of -1 could be written to uint8_t cidr.

If an invalid subnet netmask is encountered and NetmaskBits() returns -1, we can
break to exit the loop like when the number of 1-bits is 0, or do so in that
case only if the value of cidr is also 0.  This commit proposes the first option.

Fixes the following banman fuzzer crash:

https://cirrus-ci.com/task/4787303177519104?logs=ci#L3020

SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation netaddress.cpp:1190:18 in
MS: 0 ; base unit: 0000000000000000000000000000000000000000
artifact_prefix='./'; Test unit written to ./crash-0671aac15e619e99522e2119487eaa9cc97e5a34

netaddress.cpp:1190:18: runtime error: implicit conversion
  from type 'int' of value -1 (32-bit, signed)
  to type 'uint8_t' (aka 'unsigned char')
  changed the value to 255 (8-bit, unsigned)

b85c12bc67

fuzz: remove netaddress ubsan sanitizer suppression 799704d8ed

jonatack commented at 7:43 PM on October 5, 2021: member

This patch previously had 2 ACKs from @tryphe and @GeneFerneau (thanks!) in #22586.

DrahtBot added the label P2P on Oct 5, 2021

sipa commented at 10:27 PM on October 5, 2021: member

How is it possible to construct CNetMask objects with non-CIDR mask?

in src/netaddress.cpp:1192 in 799704d8ed

1186 | @@ -1187,7 +1187,12 @@ std::string CSubNet::ToString() const
1187 |              if (netmask[i] == 0x00) {
1188 |                  break;
1189 |              }
1190 | -            cidr += NetmaskBits(netmask[i]);
1191 | +            const int bits{NetmaskBits(netmask[i])};
1192 | +            if (bits == -1) {
1193 | +                // Invalid subnet mask.

laanwj commented at 1:05 PM on October 7, 2021:

using -1 as special return value is nasty, let's turn this into optional<int>
is break a good way to handle invalid values? this will still return a "valid" but incorrect value; do we need any kind of error feedback to the caller?

vasild commented at 12:54 PM on November 19, 2021:

CSubNet::ToString() probably needs something like the following at the start:

if (!valid) {
    return "invalid subnet"; // or throw?
}

And then, here, assert that NetmaskBits() always returns a good value.

In general, I don't like the notion of allowing objects that are not valid. I think it is clearer to never allow invalid objects in the first place - throw an exception from the constructor on invalid input instead of creating "an invalid" object and carry it all over the place and then wonder "How should I proceed if asked to convert this invalid stuff to string?" or "How should I compare this invalid subnet with another one? Is it bigger or smaller or equal to a valid one? To another invalid one?" (end of rant)

vasild commented at 1:10 PM on November 19, 2021: member

How is it possible to construct CNetMask objects with non-CIDR mask?

Right, how is that possible? The fuzzer uses ConsumeSubNet() which calls this constructor: CSubNet(const CNetAddr& addr, uint8_t mask). I don't see how that constructor could create CSubNet object with netmask member that has 0-bits followed by 1-bits. Even if it creates CSubNet with valid equal to false, then netmask should be all zeros.

The logs from the CI are gone. Maybe remove the URL to cirrus-ci.com from the commit message because it is now useless.

MarcoFalke commented at 3:41 PM on November 19, 2021: member

I already fixed this in commit efd6f904c78769ad2e93c1f1de43014d284e7561

vasild commented at 4:24 PM on November 19, 2021: member

Ok, the removed CSubNet unserialize method could have produced such a bricked CSubNet object (even with valid set to true).

I removed the suppression and pushed to my personal CI at https://cirrus-ci.com/task/5343275400822784 and it is green.

I think this PR should be reduced to just removing the suppression.

MarcoFalke commented at 4:25 PM on November 19, 2021: member

See #23553

MarcoFalke closed this on Nov 19, 2021

jonatack deleted the branch on Nov 21, 2021

DrahtBot locked this on Nov 21, 2022