fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()

vasild commented at 10:06 am on March 21, 2025: contributor

It would be a bug in getsockname(2) if it returns a result that is smaller than the returned socket address family. For example, if it indicates that the result is less than sizeof(sockaddr_in6) and sets sa_family equal to AF_INET6 in the output.

In other words, the name->sa_family in the output should be consistent with the returned *name_len.

The current code could fail to do that if:

the caller provides sockaddr_in6 and an input value of *name_len=28
ConsumeRandomLengthByteVector() returns a vector of 20 bytes. Then the code would only set the first 20 bytes in name.
name->sa_family from the fuzz data ends up being AF_INET6.

To produce consistent *name_len and name->sa_family, return one of AF_INET, AF_INET6 or AF_UNIX for family with the corresponding *name_len.

For reference:

0sizeof(sockaddr) = 16
1sizeof(sockaddr_in) = 16
2sizeof(sockaddr_in6) = 28
3sizeof(sockaddr_un) = 110 on Linux, 106 on FreeBSD (unix socket)
4sizeof(sockaddr_storage) = 128

https://www.man7.org/linux/man-pages/man3/sockaddr.3type.html

DrahtBot commented at 10:06 am on March 21, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32109.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	darosior
Concept ACK	laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Mar 21, 2025

fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()

It would be a bug in `getsockname(2)` if it returns a result that is
smaller than the returned socket address family. For example, if it
indicates that the result is less than `sizeof(sockaddr_in6)` and sets
`sa_family` equal to `AF_INET6` in the output.

In other words, the `name->sa_family` in the output should be consistent
with the returned `*name_len`.

The current code could fail to do that if:
* the caller provides `sockaddr_in6` and an input value of `*name_len=28`
* `ConsumeRandomLengthByteVector()` returns a vector of `20` bytes. Then
  the code would only set the first `20` bytes in `name`.
* `name->sa_family` from the fuzz data ends up being `AF_INET6`.

To produce consistent `*name_len` and `name->sa_family`, return one of
`AF_INET`, `AF_INET6` or `AF_UNIX` for family with the corresponding
`*name_len`.

For reference:
```
sizeof(sockaddr) = 16
sizeof(sockaddr_in) = 16
sizeof(sockaddr_in6) = 28
sizeof(sockaddr_un) = 110 on Linux, 106 on FreeBSD (unix socket)
sizeof(sockaddr_storage) = 128
```

https://www.man7.org/linux/man-pages/man3/sockaddr.3type.html

52b65186e1

vasild commented at 10:14 am on March 21, 2025: contributor

cc @darosior, @marcofleon

DrahtBot added the label CI failed on Mar 21, 2025

DrahtBot commented at 10:53 am on March 21, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39169547262

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

vasild force-pushed on Mar 21, 2025

vasild commented at 10:59 am on March 21, 2025: contributor

9c3298eb04...52b65186e1: fix CI (lint and win64 build)

laanwj commented at 12:00 pm on March 21, 2025: member

Concept ACK

if it indicates that the result is less than sizeof(sockaddr_in6) and sets sa_family equal to AF_INET6 in the output.

FWIW, i’ve tried to make our code robust to this case by adding the length checks in ab1d3ece026844e682676673b8a461964a5b3ce4.

But it’s a very unlikely OS bug and this change will likely result in more useful test coverage.

DrahtBot removed the label CI failed on Mar 21, 2025

darosior commented at 2:35 pm on March 24, 2025: member

Leaning toward Concept NACK.

OP is suggesting to provide more guarantees in our mocked getsockname than the one used in production. On its face i think the motivation is misguided and we should rather aim for having stricter tests to make sure our code is robust as well as to surface some edge case we might have not considered. If the reason for opening this PR is that the change in #31676 led to some fuzz crashes, then i would say it works as intended. Instead of working around the crash in the fuzz utility, the crashing code should be made more robust.

In addition i think that trying to give the guarantees that OP suggests will lead to unnecessarily convoluted code in the fuzz utility.

It would be a bug in getsockname(2) if it returns a result that is smaller than the returned socket address family.

Maybe. But it also does not give any such guarantee, so code assuming this is brittle.

In other words, the name->sa_family in the output should be consistent with the returned *name_len.

Much better this way, but not enough - it could still return e.g. IPv6 address that is smaller than the expected size for an IPv6 address, possibly confusing callers to read data past the end of the buffer.

(Second quote is from #31676 (review))

Code that, when reading the socket addr buffer, disregards said buffer size and blindly relies on the socket family type is brittle and should be fixed.

laanwj commented at 12:51 pm on March 26, 2025: member

Code that, when reading the socket addr buffer, disregards said buffer size and blindly relies on the socket family type is brittle and should be fixed.

Yes. Though to be fair, it’s something hard to get right due to lack of guarantees with regard to the socket name structure on different platforms. For example, even reading .sa_family could ostensibly reach outside the buffer. It’s common that these are the first two bytes, but not guaranteed (for example macOS has a .sa_len field i don’t remember if before or after). So to be 100% sure you’d have to check first against min(sizeof(sockaddr_in), sizeof(sockaddr_in6), sizeof(sockaddr_un), ...) (depending on the families you’re going to accept. Of course sockaddr_in is likely to be smallest but who really knows…).

How much effort to spend on this depends on how far you want to go down this rabbit hole of “the OS could give us malicious data”.

fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName() #32109

Code Coverage & Benchmarks

Reviews