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

[Backport 2.7] X509 verify non-DNS SANs #3556

Merged
merged 1 commit into from
Aug 18, 2020
Merged

[Backport 2.7] X509 verify non-DNS SANs #3556

merged 1 commit into from
Aug 18, 2020

Conversation

mpg
Copy link
Contributor

@mpg mpg commented Aug 11, 2020 •
edited
Loading

This is a very partial backport of #3554 to 2.7 - only the test is backported, as the bug was not present in this branch.

@mpg
Add test: DNS names should not match IP addresses
894c05d 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg mpg added bug mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-x509 needs-ci Needs to pass CI tests labels Aug 11, 2020
@mpg mpg self-assigned this Aug 11, 2020
@mpg mpg mentioned this pull request Aug 11, 2020
Merged
2 tasks
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 12, 2020
gilles-peskine-arm
gilles-peskine-arm requested changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looked ok to me, but CI was unhappy. It turns out that you assumed that x509_check_wildcard behaves sensibly when there's no wildcard, but in fact it returns a systematic success in that case. You fixed this a couple of years ago in 900fba6 but that was after 2.7. I think it would be best to backport that commit.

@hanno-becker
Copy link

@mpg Same comment as for the 2.16 backport: As far as I understand and remember, we silently skip non-DNS entries in the SubjectAlternativeNames extension in those older versions of Mbed TLS.

@mpg mpg force-pushed the x509-verify-non-dns-san-2.7 branch from ceb246f to 894c05d Compare August 14, 2020 07:55
@mpg mpg requested a review from gilles-peskine-arm August 14, 2020 07:59
@mpg
Copy link
Contributor Author

mpg commented Aug 18, 2020

@hanno-arm You approved the 2.16 backport yesterday but not this one, was that intentional?

hanno-becker
hanno-becker approved these changes Aug 18, 2020
View reviewed changes
Copy link

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hanno-becker
Copy link

@mpg Apologies, that was not intentional - something must have distracted me so I forgot about the 2.7 backport.

@mpg mpg removed needs-ci Needs to pass CI tests needs-work labels Aug 18, 2020
@mpg mpg merged commit 918b5f1 into Mbed-TLS:mbedtls-2.7 Aug 18, 2020
@mpg mpg deleted the x509-verify-non-dns-san-2.7 branch August 18, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@hanno-becker hanno-becker hanno-becker approved these changes

Assignees

@mpg mpg

Labels
bug component-x509
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mpg @hanno-becker @gilles-peskine-arm