') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Support sites with invalid IDN in SslStream by wfurt · Pull Request #82934 · dotnet/runtime · GitHub
Skip to content

Support sites with invalid IDN in SslStream - #82934

Merged
wfurt merged 4 commits into
dotnet:mainfrom
wfurt:sni
Mar 10, 2023
Merged

Support sites with invalid IDN in SslStream#82934
wfurt merged 4 commits into
dotnet:mainfrom
wfurt:sni

Conversation

@wfurt

@wfurtwfurt commented Mar 3, 2023

Copy link
Copy Markdown
Member

fixes#82464

This is attempt to allow TLS handshake on site with invalid IDN to match browsers and other HTTP/TLS implementations.

Since this is somewhat corner case the change is focusing on allowing the handshake go through via setting policy to ignore name. It does not try to verify that the problematic names would for example match wildcard certificates. It would be up to the caller to verify certificates as most browsers would also show warning and not proceed automatically.

Handling has been centralize to SslAuthenticationOptions and eliminated code scattered in PAL.

I'm not sure how this impacts Android @simonrozsival. I would think that doing the puny code before calling Java should be just fine since that is what should go out on wire.

@wfurt
wfurt requested a review from rzikmMarch 3, 2023 06:50
@wfurtwfurt self-assigned this Mar 3, 2023
@ghost

ghost commented Mar 3, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #82464

This is attempt to allow TLS handshake on site with invalid IDN to match browsers and other HTTP/TLS implementations.

Since this is somewhat corner case the change is focusing on allowing the handshake go through via setting policy to ignore name. It does not try to verify that the problematic names would for example match wildcard certificates. It would be up to the caller to verify certificates as most browsers would also show warning and not proceed automatically.

Handling has been centralize to SslAuthenticationOptions and eliminated code scattered in PAL.

I'm not sure how this impacts Android @simonrozsival. I would think that doing the puny code before calling Java should be just fine since that is what should go out on wire.

Author:wfurt
Assignees:wfurt
Labels:

area-System.Net.Security

Milestone:-

@build-analysisbuild-analysisBot mentioned this pull request Mar 3, 2023
Closed
@simonrozsival

simonrozsival commented Mar 3, 2023

Copy link
Copy Markdown
Member

@wfurt yeah, this change should be just fine for Android. We can run tests on Android just to make sure.

@simonrozsival

simonrozsival commented Mar 3, 2023

Copy link
Copy Markdown
Member

/azp run runtime-android

@azure-pipelines

azure-pipelinesBot commented Mar 3, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

rzikm
rzikm approved these changes Mar 3, 2023
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs Outdated
rzikm
rzikm reviewed Mar 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs Outdated

private static unsafe bool IsSafeDnsString(ReadOnlySpan<char> name)
{
for (int i = 0; i < name.Length; i++)

@rzikmrzikmMar 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could use the new IndexOfAnyValues<T>, but not sure if the allocation of static map is worth it for such short strings.

@MihaZupanMihaZupanMar 3, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this should use IndexOfAnyValues. The allocation there is very small, we'll only store a bitmap for the ASCII range.

// Alphanumeric or '-', '.', '_'privatestaticreadonlyIndexOfAnyValues<char>s_safeDnsChars=IndexOfAnyValues.Create("-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz");privatestaticboolIsSafeDnsString(ReadOnlySpan<char>name)=>name.IndexOfAnyExcept(s_safeDnsChars)<0;

@wfurtwfurtMar 3, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

IndexOfAnyValues crossed my mind but the set of characters I don't want is large for Unicode. I was not aware of IndexOfAnyExcept

@wfurtwfurtMar 5, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

updated. Just to be sure: this would only match that ASCII characters, not their Unicode equivalent (if any), right @MihaZupan ?

@MihaZupanMihaZupanMar 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's right, the set of values you pass to Create is "ordinal".

@build-analysisbuild-analysisBot mentioned this pull request Mar 3, 2023
Closed
@wfurt

wfurt commented Mar 3, 2023

Copy link
Copy Markdown
MemberAuthor

thanks @simonrozsival. I did not know you can just run Android. I use extra-platforms and that is slow, expensive and always fails. The Android run is failing but I could not figure out from the output why. Can you please check of give me hints how to investigate. I suspect that the invalid names are failing inside of the Java engine. We have either choice to disable this test on Android, relax the receiving check or strip the SNI and try to send it without it. As I mentioned this request come from crawling use case and perhaps is not even applicable to mobile devices. (of course, consistency would be nice)

@simonrozsival

simonrozsival commented Mar 8, 2023

Copy link
Copy Markdown
Member

@wfurt The www-. prefix will be rejected when we pass it to the java method. This is the java error in the logs:

System.err: java.lang.IllegalArgumentException: Invalid input to toASCII: www-.volal.cz
System.err: at java.net.IDN.toASCII(IDN.java:115)
System.err: at javax.net.ssl.SNIHostName.<init>(SNIHostName.java:99)
System.err: Caused by: The input does not conform to the STD 3 ASCII rules. line: 0. preContext: . postContext: -
System.err: at android.icu.impl.IDNA2003.convertToASCII(IDNA2003.java:228)
System.err: at android.icu.impl.IDNA2003.convertIDNToASCII(IDNA2003.java:277)
System.err: at android.icu.text.IDNA.convertIDNToASCII(IDNA.java:654)
System.err: at java.net.IDN.toASCII(IDN.java:108)
System.err: ... 1 more

When I comment out the SslException
that we throw when the SNI is rejected as you suggested then all 3 test inputs pass just fine.

runtime/src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs

Line 98 in 6c836e8

thrownewSslException();

@wfurt

wfurt commented Mar 8, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-android

@azure-pipelines

azure-pipelinesBot commented Mar 8, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@build-analysisbuild-analysisBot mentioned this pull request Mar 8, 2023
Closed
@wfurt

wfurt commented Mar 9, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-android

@azure-pipelines

azure-pipelinesBot commented Mar 9, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@wfurt

wfurt commented Mar 10, 2023

Copy link
Copy Markdown
MemberAuthor

test failures are unrelated

@wfurt
wfurt merged commit c3df339 into dotnet:mainMar 10, 2023
@wfurt
wfurt deleted the sni branch March 10, 2023 19:00
@karelzkarelz added this to the 8.0.0 milestone Mar 21, 2023
@rzikmrzikm mentioned this pull request Apr 18, 2023
Merged
@ghostghost locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MihaZupanMihaZupanMihaZupan left review comments
@rzikmrzikmrzikm approved these changes
@simonrozsivalsimonrozsivalsimonrozsival approved these changes

Assignees

@wfurtwfurt

Labels

area-System.Net.Security

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Support sites with invalid IDN in SslStream

5 participants

@wfurt@simonrozsival@MihaZupan@rzikm@karelz