Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix GH-9356: Incomplete validation of IPv6 Address fields in subjectAltNames #11145

Closed
wants to merge 9 commits into from
Closed

Fix GH-9356: Incomplete validation of IPv6 Address fields in subjectAltNames #11145

wants to merge 9 commits into from

Conversation

lucasnetau
Copy link
Contributor

@lucasnetau lucasnetau commented Apr 28, 2023 •
edited
Loading

Fixes GH-9356

IPv6 addresses are valid entries in subjectAltNames. Certificate Authorities may issue certificates including IPv6 addresses except if they fall within addresses in the RFC 4193 range. Google and CloudFlare provide IPv6 addresses in their DNS over HTTPS services.

Internal CAs do not have those restrictions and can issue Unique local addresses in certificates.

My C is a bit rusty and this is my first attempt at fixing this issue. I would appreciate any feedback, and assistance in checking I have enabled Windows support for inet_pton correctly, and what to do on systems without inet_pton support (any still around in the supported systems?)

@lucasnetau lucasnetau requested a review from bukka as a code owner April 28, 2023 01:09
@lucasnetau lucasnetau changed the base branch from master to PHP-8.1 April 30, 2023 05:13
@lucasnetau lucasnetau changed the title [WIP] Fix GH-9356: Incomplete validation of IP Address fields in subject… Fix GH-9356: Incomplete validation of IPv6 Address fields in subjectAltNames Apr 30, 2023
@lucasnetau
Copy link
Contributor Author

Hi @bukka Could you please review this PR and let me know if I need to add/change anything to add the missing IPv6 support?

bukka
bukka requested changes May 6, 2023
View reviewed changes
ext/openssl/xp_ssl.c Outdated Show resolved Hide resolved
ext/openssl/xp_ssl.c Outdated Show resolved Hide resolved
@bukka
Copy link
Member

bukka commented May 6, 2023

I think the change makes sense but in my eyes it's more feature than bug as it was not done on purpose. I'm fine with changing that but just in master so please rebase it.

@lucasnetau
Copy link
Contributor Author

Thank you for the feedback, I'll make the required code changes.

I was hoping this would be considered a bug per the initial issue #9356 post triage categorisation . The initial support for IP Address fields was implemented as a bug fix (https://bugs.php.net/bug.php?id=68879) and it made the incorrect statement that SAN IP Addresses were deprecated and skipped the implementation of IPv6. So the missing functionality is more a bug in implementing certificate validation specification. I hope you can please reconsider?

This is an issue we encounter when doing IPv6 only work and the various work around we have including peer cert signature verification does not always work as providers such as Google use different certificates on various edge points (even in the same location), having this at least land in the PHP8.2 branch would at least allow us to no longer rely on these workarounds.

bukka
bukka reviewed May 8, 2023
View reviewed changes
ext/openssl/xp_ssl.c Outdated Show resolved Hide resolved
@bukka
Copy link
Member

bukka commented May 8, 2023 •
edited
Loading

Ok after thinking about we can treat this as a bug. Except one small NIT, the changes look fine to me. I just approved CI so we will see how it goes.

lucasnetau and others added 7 commits May 9, 2023 10:15
@lucasnetau
Fixed bug php#9356 Incomplete validation of IP Address fields in subj…
3d10ba0 
…ectAltNames

IPv6 addresses are valid entries in subjectAltNames. Certificate Authorities may issue certificates including IPv6 addresses except if they fall within addresses in the RFC 4193 range. Google and CloudFlare provide IPv6 addresses in their DNS over HTTPS services.

Internal CAs do not have those restrictions and can issue Unique local addresses in certificates.
@lucasnetau
Conditionally include arpa/inet.h if detected
70a523d 
Include <Ws2tcpip.h> for Windows inet_pton support
Conditionally do subject_name IPv6 expansion only when both inet_pton is available and IPv6 support is comiled in
@lucasnetau
Implement suggested expanded flag instead of checking strlen on each …
d38f3e0 
…iteration
@lucasnetau @bukka
snprintf buffer size includes the \0
7da946e 
Co-authored-by: Jakub Zelenka <bukka@php.net>
@lucasnetau
CS: Space after comma
d297059
@lucasnetau
Skip test if IPv6 support is not available
d445ec5 
Use different port number (possible re-use failure in tests)
@lucasnetau
Add News
0c6b7ca
@lucasnetau
Copy link
Contributor Author

lucasnetau commented May 9, 2023 •
edited
Loading

Requested CS change made. I also added a test SKIPIF if IPv6 support isn't enabled at compile time or if it has been disabled in CI via the CI_NO_IPV6 flag

@lucasnetau
Copy link
Contributor Author

LINUX_X32_DEBUG_ZTS doesn't appear to have IPv6 support enabled at the Docker layer and CI_NO_IPV6 is not set, this causes the test to fail due to the inability to create a IPv6 bound server. I've added an additional SKIPIF test taken from (/ext/sockets/tests/ipv6_skipif.inc) to check if hosts level support for IPv6 is enabled.

========DIFF========
001+ Warning: stream_socket_client(): Unable to connect to ssl://[::1]:64324 (Cannot assign requested address) in /__w/php-src/php-src/ext/openssl/tests/ServerClientTestCase.inc(130) : eval()'d code on line 10
002+ bool(false)
001- resource(%d) of type (stream)
     
003- Warning: stream_socket_client(): Unable to locate peer certificate CN in %s on line %d
004- 
005- Warning: stream_socket_client(): Failed to enable crypto in %s on line %d
006- 
007- Warning: stream_socket_client(): Unable to connect to ssl://[::1]:64324 (Unknown error) in %s on line %d
004+ Warning: stream_socket_client(): Unable to connect to ssl://[::1]:64324 (Cannot assign requested address) in /__w/php-src/php-src/ext/openssl/tests/ServerClientTestCase.inc(130) : eval()'d code on line 13
     bool(false)
========DONE========
FAIL IPv6 Peer verification matches SAN names [ext/openssl/tests/san_ipv6_peer_matching.phpt]

@lucasnetau
Copy link
Contributor Author

@bukka this should be complete now including ensuring tests can only run on IPv6 compiled php and host enabled CI

@lucasnetau lucasnetau requested a review from bukka May 17, 2023 10:24
@lucasnetau
Copy link
Contributor Author

@bukka Could you please approve the workflow to do the final tests?

@lucasnetau
Copy link
Contributor Author

This is ready for merge, all tests have passed and requested changes in place

@lucasnetau
Copy link
Contributor Author

Hi @bukka Is there any chance of this being merged soon, please?

@bukka bukka closed this in fd09728 Jun 9, 2023
@bukka
Copy link
Member

bukka commented Jun 9, 2023

Apology for slight delay (I usually rotate things to do in PHP every month to not switch context too much and increase my productivity).

The code was good and I also tested everything and all good as well.

It's all merged and also added a follow up minor CS fix in 3fc013b that I didn't want to bother you with.

Thanks for you contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bukka bukka Awaiting requested review from bukka

Assignees
No one assigned
Labels
Extension: openssl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lucasnetau @bukka