RE: [openssl/openssl] bio_dgram vs IPv6
> From: Michael Richardson > Sent: Friday, 1 April, 2022 07:40 > > Michael Wojcik wrote: > > Actually, in the context of #if expressions, unrecognized tokens > expand to 0 anyway: > > > After all replacements due to macro expansion and the defined unary > > operator have been performed, all remaining identifiers are replaced > > with the pp-number 0... > > > (ISO 9899:1999 6.10.1 #3) > > Yes, but that generates a warning, and then error via -Werror with some > set > of compile options that at least one CI run uses. Oh, well. An implementation is allowed to generate any diagnostics it wishes, and is allowed to fail to translate even a conforming program. Ultimately we're at the mercy of the implementation, and GCC is not a particularly good C implementation. (Of course, in its default mode, it doesn't implement C; it implements a language similar to, but not, C.)
Re: [openssl/openssl] bio_dgram vs IPv6
Michael Wojcik wrote: > Actually, in the context of #if expressions, unrecognized tokens expand to 0 anyway: > After all replacements due to macro expansion and the defined unary > operator have been performed, all remaining identifiers are replaced > with the pp-number 0... > (ISO 9899:1999 6.10.1 #3) Yes, but that generates a warning, and then error via -Werror with some set of compile options that at least one CI run uses. signature.asc Description: PGP signature
RE: [openssl/openssl] bio_dgram vs IPv6
> From: Michael Richardson > Sent: Thursday, 31 March, 2022 14:18 > > Michael Wojcik wrote: > > #if defined OPENSSL_SYS_WINDOWS > > # include > > #else > > # include > > #endif > > But, don't all the OPENSSL_* macros expand to 0/1, anyway, so we actually > just want #if OPENSSL_SYS_WINDOWS? I did a quick grep through the source for 1.1.1k (just because that's what I had to hand; we've actually just finished updating my portfolio to 1.1.1n), and there's a mix of #if OPENSSL_SYS_WINDOWS and #if defined(OPENSSL_SYS_WINDOWS). apps/s_client.c uses the latter, for example. Actually, in the context of #if expressions, unrecognized tokens expand to 0 anyway: After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers are replaced with the pp-number 0... (ISO 9899:1999 6.10.1 #3) So defining a macro used for conditional inclusion to the value 0 is kind of a bad idea, since that means there's different behavior between #if FOO and #if defined FOO. Much better to not define it and get the default value of 0 if you want to turn it off. But that said, #if OPENSSL_SYS_WINDOWS is safer for the same reason: it doesn't matter whether it's defined as 0, or not defined at all. The "defined" operator is overused in C source generally. It's good for things like header inclusion guards. It's not really a good choice for most other cases of conditional inclusion. -- Michael Wojcik
RE: [openssl/openssl] bio_dgram vs IPv6
> From: openssl-users On Behalf Of > Michael Richardson > Sent: Thursday, 31 March, 2022 14:19 > > The clang-9 test fails with: > > # ERROR: @ test/bio_dgram_test_helpers.c:150 > # failed to v6 bind socket: Permission denied > # > # > # OPENSSL_TEST_RAND_ORDER=1648577511 > not ok 2 - iteration 1 > > https://github.com/mcr/openssl/runs/5741887864?check_suite_focus=true > > The other clang-XX tests seem to run fine. > This smells like the problem with TRAVIS where IPv6 was not enabled in the > Google VMs, but we aren't using those anymore. > > It does not bind specific sockets (lets kernel choose), so there shouldn't > a > conflict between test cases. Anyway, if that were the case, I'd expect to > see in-use error rather than permission denied. > > Smells to me like someone has restricted network sockets in order to avoid > being used as an attack system. Yes, the EPERM certainly suggests that. Are these running on Linux VMs? SELinux or similar in use, perhaps? -- Michael Wojcik
Re: [openssl/openssl] bio_dgram vs IPv6
The clang-9 test fails with: # ERROR: @ test/bio_dgram_test_helpers.c:150 # failed to v6 bind socket: Permission denied # # # OPENSSL_TEST_RAND_ORDER=1648577511 not ok 2 - iteration 1 https://github.com/mcr/openssl/runs/5741887864?check_suite_focus=true The other clang-XX tests seem to run fine. This smells like the problem with TRAVIS where IPv6 was not enabled in the Google VMs, but we aren't using those anymore. It does not bind specific sockets (lets kernel choose), so there shouldn't a conflict between test cases. Anyway, if that were the case, I'd expect to see in-use error rather than permission denied. Smells to me like someone has restricted network sockets in order to avoid being used as an attack system. signature.asc Description: PGP signature
Re: [openssl/openssl] bio_dgram vs IPv6
Michael Wojcik wrote: > #if defined OPENSSL_SYS_WINDOWS > # include > #else > # include > #endif But, don't all the OPENSSL_* macros expand to 0/1, anyway, so we actually just want #if OPENSSL_SYS_WINDOWS? > (Note C does not require the argument of the operator "defined" to be > parenthesized. Doing so just adds visual noise. ISO 9899-1999 6.10.1 > #1.) Thank you. I've updated bss_dgram.c, squashed against the last commit relating to USE_IPV6 and pushed. -- ] Never tell me the odds! | ipv6 mesh networks [ ] Michael Richardson, Sandelman Software Works|IoT architect [ ] m...@sandelman.ca http://www.sandelman.ca/| ruby on rails[
RE: [openssl/openssl] bio_dgram vs IPv6
> From: openssl-users On Behalf Of Matt > Caswell > Sent: Tuesday, 22 March, 2022 10:31 > > There is already code in bss_dgram.c that is conditionally compiled on > OPENSSL_USE_IPV6. Is it reasonable to assume that if AF_INET6 is defined > then ip6.h exists? I meant to look into this earlier but got distracted. Windows has IPv6 support and defines AF_INET6, but does not have ip6.h (at least in the SDK version I have installed on this machine). If you do a search online you'll see many projects have copied the ip6.h from some other platform into their source trees for use by Windows. I've confirmed it's present on: * AIX 7.1 * HP-UX 11.31316 * Solaris 11.3 and of course on Linux generally. I don't have other platforms handy to test. Windows will be the sticking point. However, the Microsoft Windows SDK includes a header shared/netiodef.h, which includes at least some of the structures defined by RFC 3542, albeit with different type and field names; and macros mapping the RFC 3542 names to those identifiers. At least the following are available in that header: ip6_hdr ip6_flow ip6_plen ip6_nxt ip6_hops ip6_hlim ip6_src ip6_dst So something like this might work: #if defined OPENSSL_SYS_WINDOWS # include #else # include #endif (Note C does not require the argument of the operator "defined" to be parenthesized. Doing so just adds visual noise. ISO 9899-1999 6.10.1 #1.) -- Michael Wojcik
Re: [openssl/openssl] bio_dgram vs IPv6
Matt Caswell wrote: > There is already code in bss_dgram.c that is conditionally compiled on > OPENSSL_USE_IPV6. Is it reasonable to assume that if AF_INET6 is > defined then ip6.h exists? I think so, so I changed that code, and also made it consistently use OPENSSL_USE_IPV6, rather than having AF_INET6 sprinkled in. I don't know how much to squash all of this down again. signature.asc Description: PGP signature
Re: [openssl/openssl] bio_dgram vs IPv6
On 22/03/2022 16:22, Michael Richardson wrote: Michael Wojcik wrote: > The RFC specifically mentions using this API to retrieve and set > addresses, so it seems like a fix for issue 5257 does need to use it, > if that's to be done in a portable way. > 3542 is only Informational, but I'd expect most or all platforms with > IPv6 support to conform to it. The issue isn't whether we can expect it to be standard. The issue is what we can use as a signal that the header exists. To date, I don't think that openssl has had to know if IPv6 existed or not on a particular platform. internal/sockets.h has this snippet in it: /* * Some IPv6 implementations are broken, you can disable them in known * bad versions. */ # if !defined(OPENSSL_USE_IPV6) # if defined(AF_INET6) # define OPENSSL_USE_IPV6 1 # else # define OPENSSL_USE_IPV6 0 # endif # endif There is already code in bss_dgram.c that is conditionally compiled on OPENSSL_USE_IPV6. Is it reasonable to assume that if AF_INET6 is defined then ip6.h exists? Matt
Re: [openssl/openssl] bio_dgram vs IPv6
Matt Caswell wrote: >> Matt Caswell wrote: > Nit; We insert an >> extra space when enclosed within a "#if", i.e. >> >> I assume that this applies recursively? > Yes. >> I think that in some cases the indent could be quite deep. > It hasn't been a major issue so far. I'm sure that I need to make another pass to get it perfect. Once the issues in my other email are resolved, I will squash it all down to one commit, I think. I had tried up to now to keep, for instance, the *BSD code as a separate commit to make it easier to review. -- ] Never tell me the odds! | ipv6 mesh networks [ ] Michael Richardson, Sandelman Software Works| network architect [ ] m...@sandelman.ca http://www.sandelman.ca/| ruby on rails[ signature.asc Description: PGP signature
Re: [openssl/openssl] bio_dgram vs IPv6
Michael Wojcik wrote: > The RFC specifically mentions using this API to retrieve and set > addresses, so it seems like a fix for issue 5257 does need to use it, > if that's to be done in a portable way. > 3542 is only Informational, but I'd expect most or all platforms with > IPv6 support to conform to it. The issue isn't whether we can expect it to be standard. The issue is what we can use as a signal that the header exists. To date, I don't think that openssl has had to know if IPv6 existed or not on a particular platform. signature.asc Description: PGP signature
RE: [openssl/openssl] bio_dgram vs IPv6
> From: openssl-users On Behalf Of Matt > Caswell > Sent: Monday, 21 March, 2022 05:33 > > Given that OpenSSL already supports IPv6 but we've never needed to > include [netinet/ip6.h], I am wondering what is in that header that needs to > be used? netinet/ip6.h is for the "Advanced API for IPv6", detailed in RFC 3542. It's typically used for raw-socket access to IPv6, for things like traceroute and hop-by-hop. The RFC specifically mentions using this API to retrieve and set addresses, so it seems like a fix for issue 5257 does need to use it, if that's to be done in a portable way. 3542 is only Informational, but I'd expect most or all platforms with IPv6 support to conform to it. -- Michael Wojcik
Re: [openssl/openssl] bio_dgram vs IPv6
On 19/03/2022 13:28, Michael Richardson wrote: I'm working on dealing with Matt's detailed review. This issue seems bigger than the github issue. https://github.com/openssl/openssl/pull/5257 about: #include matt> This remains an issue. It's unclear to me whether all of these headers will matt> be available on all platforms. At least in some files we wrap some of these matt> in an "ifdef OPENSSL_SYS_UNIX". We have, as yet, never used netinet/ip6.h - matt> so I am concerned it may not be universally available on all the platforms matt> that we might need to support. These would platforms that are not in the CI. I agree that IPv6 not be present. I could put all the IPv6 code behind some #ifndef IPV6_MISSING which would presently be never defined, but could be added later to ./Configure if/when we figure out where it's a problem. Given that OpenSSL already supports IPv6 but we've never needed to include that header, I am wondering what is in that header that needs to be used? Matt Caswell wrote: > Nit; We insert an extra space when enclosed within a "#if", i.e. I assume that this applies recursively? Yes. I think that in some cases the indent could be quite deep. It hasn't been a major issue so far. Matt I wonder if there an emacs mode that does this I browsed the man page for indent(1), but I didn't find an option for this year, but I'll look again closer. -- ] Never tell me the odds! | ipv6 mesh networks [ ] Michael Richardson, Sandelman Software Works| network architect [ ] m...@sandelman.ca http://www.sandelman.ca/| ruby on rails[
Re: [openssl/openssl] bio_dgram vs IPv6
I'm working on dealing with Matt's detailed review. This issue seems bigger than the github issue. https://github.com/openssl/openssl/pull/5257 about: #include matt> This remains an issue. It's unclear to me whether all of these headers will matt> be available on all platforms. At least in some files we wrap some of these matt> in an "ifdef OPENSSL_SYS_UNIX". We have, as yet, never used netinet/ip6.h - matt> so I am concerned it may not be universally available on all the platforms matt> that we might need to support. These would platforms that are not in the CI. I agree that IPv6 not be present. I could put all the IPv6 code behind some #ifndef IPV6_MISSING which would presently be never defined, but could be added later to ./Configure if/when we figure out where it's a problem. Matt Caswell wrote: > Nit; We insert an extra space when enclosed within a "#if", i.e. I assume that this applies recursively? I think that in some cases the indent could be quite deep. I wonder if there an emacs mode that does this I browsed the man page for indent(1), but I didn't find an option for this year, but I'll look again closer. -- ] Never tell me the odds! | ipv6 mesh networks [ ] Michael Richardson, Sandelman Software Works| network architect [ ] m...@sandelman.ca http://www.sandelman.ca/| ruby on rails[
