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.

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

Require UTF8 encoding in GetX509NameInfo #59116

Merged
merged 1 commit into from
Sep 15, 2021
Merged

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Sep 14, 2021

This changes GetX509NameInfo to force UTF8 encoding when writing to the BIO with ASN1_STRING_print_ex. The OpenSSL function won't return UTF8 unless you ask it to. Otherwise, it returns an escaped string of the contents of the string.

We assume the BIO is going to contain UTF8 here:

Test result, before the fix:

xUnit.net 00:00:01.07]     System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: True) [FAIL]
[xUnit.net 00:00:01.09]     System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: False) [FAIL]
  Failed System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: True) [6 ms]
  Error Message:
   Assert.Equal() Failure
          ↓ (pos 0)
Expected: картошка
Actual:   \U043A\U0430\U0440\U0442\U043E\U0448\U043···
          ↑ (pos 0)
  Stack Trace:
     at System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(Boolean issuer) in /workspaces/runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/PropsTests.cs:line 403
  Failed System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: False) [< 1 ms]
  Error Message:
   Assert.Equal() Failure
          ↓ (pos 0)
Expected: картошка
Actual:   \U043A\U0430\U0440\U0442\U043E\U0448\U043···
          ↑ (pos 0)
  Stack Trace:
     at System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(Boolean issuer) in /workspaces/runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/PropsTests.cs:line 403

Closes #59105

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 14, 2021
@ghost
Copy link

ghost commented Sep 14, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

This changes GetX509NameInfo to force UTF8 encoding when writing to the BIO with ASN1_STRING_print_ex. However the OpenSSL function won't return UTF8 unless you ask it to. Otherwise, it returns an escaped string of the contents of the string.

We assume the BIO is going to contain UTF8 here:

Test result, before the fix:

xUnit.net 00:00:01.07]     System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: True) [FAIL]
[xUnit.net 00:00:01.09]     System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: False) [FAIL]
  Failed System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: True) [6 ms]
  Error Message:
   Assert.Equal() Failure
          ↓ (pos 0)
Expected: картошка
Actual:   \U043A\U0430\U0440\U0442\U043E\U0448\U043···
          ↑ (pos 0)
  Stack Trace:
     at System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(Boolean issuer) in /workspaces/runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/PropsTests.cs:line 403
  Failed System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(issuer: False) [< 1 ms]
  Error Message:
   Assert.Equal() Failure
          ↓ (pos 0)
Expected: картошка
Actual:   \U043A\U0430\U0440\U0442\U043E\U0448\U043···
          ↑ (pos 0)
  Stack Trace:
     at System.Security.Cryptography.X509Certificates.Tests.PropsTests.GetNameInfo_HandlesUtf8Encoding(Boolean issuer) in /workspaces/runtime/src/libraries/System.Security.Cryptography.X509Certificates/tests/PropsTests.cs:line 403

Closes #59105

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@bartonjs
Copy link
Member

@danmoseley Would you be supportive of taking this as a 6.0 candidate?

  • Globalization impact
  • Very scoped fix
  • Now with tests
  • Externally reported (StackOverflow)

@danmoseley
Copy link
Member

Might. Can you share SO link? How long has this bug been present without previous reports?

Let's get this merged meantime

@bartonjs
Copy link
Member

Can you share SO link?

https://stackoverflow.com/questions/69105533/net-5-x509certificate-getnameinfo-uppercases-unicode-escape-characters

How long has this bug been present without previous reports?

.NET Core 1.0 preview 8, or thereabouts 😄.

@vcsjones
Copy link
Member Author

Failure:

System.Net.Security.Tests.ApmSslStreamSystemDefaultTest.ClientAndServer_OneOrBothUseDefault_Ok(clientProtocols: Tls13, serverProtocols: null) [FAIL]
System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
---- System.ComponentModel.Win32Exception : The Local Security Authority cannot be contacted

Seems like #58927 and is unrelated from the PR.

@vcsjones
Copy link
Member Author

I don't think the macOS libraries tests ran, either.

@danmoseley
Copy link
Member

Rerunning failed jobs...

thinking about it, this might meet the servicing bar. Let's get a PR up against 6.0 once this goes in. Or preemptively, if you like.

@bartonjs bartonjs merged commit 687c51d into dotnet:main Sep 15, 2021
@bartonjs
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1238458928

@vcsjones vcsjones deleted the 59105-fix-utf8 branch September 15, 2021 16:35
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X509Certificate2.GetNameInfo(SimpleName) with non-ASCII data on Linux returns oddly encoded results.
3 participants