fuzz: Make ConsumeNetAddr always produce valid onion addresses #26497
Conversation
0d2fd4c to
4e1bc19
Compare
vasild
left a comment
There was a problem hiding this comment.
Approach ACK 4e1bc1971245714735eff70700dfd0f336169db5
maflcko
left a comment
There was a problem hiding this comment.
Good catch.
Nit: Maybe move it to a separate fuzz util helper? It is used in the following tests:
src/test/fuzz/addrman.cpp
src/test/fuzz/banman.cpp
src/test/fuzz/connman.cpp
src/test/fuzz/netaddress.cpp
src/test/fuzz/netbase_dns_lookup.cpp
and could be placed in a new src/test/fuzz/util/net.h?
4e1bc19 to
6052380
Compare
|
Thanks for the reviews @vasild @MarcoFalke! Addressed all the comments and added a new commit for moving |
vasild
left a comment
There was a problem hiding this comment.
ACK 60523801db69efccbe5e997237dc41a9684bc3a2
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
|
There was a problem hiding this comment.
tested at runtime that this SetSpecial always fails on master with this diff:
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
index d495a6bfe3..74cddd037d 100644
--- a/src/test/fuzz/util.cpp
+++ b/src/test/fuzz/util.cpp
@@ -524,7 +524,7 @@ CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider) noexcept
} else if (network == Network::NET_INTERNAL) {
net_addr.SetInternal(fuzzed_data_provider.ConsumeBytesAsString(32));
} else if (network == Network::NET_ONION) {
- net_addr.SetSpecial(fuzzed_data_provider.ConsumeBytesAsString(32));
+ Assert(!net_addr.SetSpecial(fuzzed_data_provider.ConsumeBytesAsString(32)));
}
return net_addr;
}review ACK 60523801db69efccbe5e997237dc41a9684bc3a2 🍦
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 60523801db69efccbe5e997237dc41a9684bc3a2 🍦
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjUcwv+LS3rIkpeUS3dDy6+pcvtTGUM5DUcyT+gYPxd+1vGujS1wk20fN5JqE+n
ILMK5kgtRcQe/EV8yW9coVjWwiMJ2F5SGuJnw0+1ZeO7B6sCAsgpg7ouLoDyedbv
H2HK3B8gtsu82SYaWP22Lh3lM5wASWeKdT3lrn+eBHTWBQd4A0+446h3cfM9h0sM
RFtDdwr21vb5zQoq3KgbN6l8+RQMzn29QnFWYxPkS08pyqVacib4m+htZxFo3XWs
Sez8d/YTBmg63YC6AdZ7Oz40icJsb75WJwNQxmRQLXpxlwkAaJYchtEKAJ0HRtZJ
Uqz+OAwnlAkOIypfTH3yDqIFE9I2KbE5kvMlVbUfEnSJ9tVSAjTCgKw8chPMm6iW
faFSf9fUc0ob0PiJlTXvplG444Sy4iT5C+m2+7LVfz2atVF6bpIUbs2rtGfZ3RWr
dgv6TeLPAzjmUemug9KBvog4P5DhUYQWghwa6NG75pEkv68ulciyadlN5jNQtZCG
AfEYffWE
=yuEE
-----END PGP SIGNATURE-----
@MarcoFalke, do you mean that it always succeeds? I mean, |
6052380 to
78b6372
Compare
vasild
left a comment
There was a problem hiding this comment.
ACK 78b6372863a4b22023a7d141ea5c854a67d41363
brunoerg
left a comment
There was a problem hiding this comment.
crACK 78b6372863a4b22023a7d141ea5c854a67d41363
78b6372 to
0eeb9b0
Compare
|
Changed the approach a bit to use |
| /// SAM 3.1 and earlier do not support specifying ports and force the port to 0. | ||
| static constexpr uint16_t I2P_SAM31_PORT{0}; | ||
|
|
||
| std::string OnionToString(Span<const uint8_t> addr); |
There was a problem hiding this comment.
Since this is now public, it would be nice to document it. Something like:
/**
* Generate the ".onion" address which corresponds to a public key.
* @param[in] addr public key bytes, must be 32.
* @returns for example 5g72ppm3krkorsfopcm2bi7wlv4ohhs4u4mlseymasn7g7zhdcyjpfid.onion
*/|
Btw, if there is value in breaking the inputs (#26497 (comment)), it can be done, because fuzzing is running 24/7 anyway, so any "lost" inputs should be re-discovered eventually. |
…nion addresses 0eeb9b0 [fuzz] Move ConsumeNetAddr to fuzz/util/net.h (dergoegge) 291c869 [fuzz] Make ConsumeNetAddr produce valid onion addresses (dergoegge) c9ba3f8 [netaddress] Make OnionToString public (dergoegge) Pull request description: The chance that the fuzzer is able to guess a valid onion address is probably slim, as they are Base32 encoded and include a checksum. Right now, any target using `ConsumeNetAddr` would have a hard time uncovering bugs that require valid onion addresses as input. This PR makes `ConsumeNetAddr` produce valid onion addresses by using the 32 bytes given by the fuzzer as the pubkey for the onion address and forming a valid address according to the torv3 spec. ACKs for top commit: vasild: ACK 0eeb9b0 brunoerg: ACK 0eeb9b0 Tree-SHA512: 7c687a4d12f9659559be8f0c3cd4265167d1261d419cfd3d503fd7c7f207cc0db745220f02fb1737e4a5700ea7429311cfc0b42e6c15968ce6a85f8813c7e1d8
|
A natural followup to this would be to also generate I2P and CJDNS addresses. That needs to modify the |
Done in #26859. |
The chance that the fuzzer is able to guess a valid onion address is probably slim, as they are Base32 encoded and include a checksum. Right now, any target using
ConsumeNetAddrwould have a hard time uncovering bugs that require valid onion addresses as input.This PR makes
ConsumeNetAddrproduce valid onion addresses by using the 32 bytes given by the fuzzer as the pubkey for the onion address and forming a valid address according to the torv3 spec.