-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Crash on Android 10 with underscore in subdomain #5840
Comments
I've reported upstream. I'm not sure this is something we really want to fix in OkHttp
We would catch a specific error due to a specific bug/strict handling in Conscrypt |
Prior to the fix for Android R blacklisting methods, we didn't read the SSL Parameters, we only set the value.
|
If it turns out to be fixed in Android 11 (v30), then we could bump the version check to >= 30, and keep using the Gray listed methods on Android 10 (v29). Since this is a regression in that sense. Or catch this specific exception on all 3 branches, with 3 different patches IIUC.
|
Underscores in domains work in Firefox and Chrome; they should work in Android! We should ask AOSP to fix. But if Conscrypt won't handshake these URLs anyway, we should catch the IllegalArgumentException and rethrow whichever SSLException subtype we were throwing previously. Throwing unchecked exceptions deep inside of OkHttp is bad news. |
I'll put up 3 PRs but it might take a couple of days. Free for someone else to pick it up in the meantime :) |
I'll raise an AOSP bug for this, but it might be a struggle to get it changed in time for Android 11 (that said, the AOSP bug will probably come back to me anyway, so maybe ;) Background: on Android Special casing underscores would be uglier and probably only work for ASCII domain names, e.g. something like: If IDNA rejects the name and it contains underscores, strip them and try again. If the second attempt succeeds and the returned string is identical, then we know the original string is already ASCII. I'm open to better ideas! It might be easier to push that logic into SNIHostname. Personally I think accepting underscores is fine, that fight has been lost and we should be compatible with browsers, and at a cursory glance I don't see any CTS tests for IDN saying they should be rejected, so it should be doable. Meantime, what Jesse said: okhttp should probably catch and rethrow. [*] I did agitate to start moving towards IDN 2008 in Android 11, but it didn't get any traction. |
Android platform engineer here :)
My understanding is that underscores aren't RFC compliant. I'm guessing you'd considered this in your statement, and think that AOSP should favor consistency with common browsers over standards compliance? Personally, I'm more hesitant to allow this than you seem to be; I'd at least not want to rush it, since the fact that Android doesn't support them is not new behaviour; is my understanding correct? |
@15characterlimi As you know OkHttp strongly favours browser consistency. But the focus of the 3 fixes is just to swallow an I|llegalArgumentException that crashes apps, and turn into a IOException that users already deal with. There is no additional work to try to make it work on Android 10 via graylisted methods (wouldn't work in Android 11), it's literally to stop crashing the app if someone posts content with _ in a hostname. It's basically a DOS against Apps in the Google Play store unless we fix it. The Android or Conscrypt fixes are out of scope of this fix, debate with @swankjesse. For now, we've just reported upstream, your call how to deal with those. |
I can also swallow the exception completely, so we just drop back to HTTP/1.1. But otherwise probably still work? |
Fixed in 4.5.0-RC1. |
@swankjesse @yschimke @15characterlimi @prbprbprb |
You would need to swap out platform Conscrypt for an alternative SSL stack. I can try to reproduce with the latest Conscrypt as a dependency. Or maybe you can test with bouncy castle? |
Thanks for the reply. I can see now that exception is related to Conscrypt and found the page with available security providers https://square.github.io/okhttp/security/security_providers/#provider-status |
@yschimke With BouncyCastleJsseProvider I was able to solve the _ underscore issue. Posted answer to the https://stackoverflow.com/a/78095509/1078641 with the solution. |
@electroidru I'm not sure that "solve" is exactly the right term there... ;) I'm sure BC's TLS stack works, but (a) it's pure Java so less performant, (b) less well tested as it's unused by default on Android and (c) I wonder how they're handling IDNs if they never call the platform's This issue kind of fell through the cracks and back in 2020 and at the time I was inclined to agree with Jesse that we should support underscores, but nowadays even the CAs are reluctant to do that for TLS certificates so we should stick with rejecting them: https://groups.google.com/g/mozilla.dev.security.policy/c/7bYUU5uIPCI Also since 2020 that Edit: "we" in the above being Conscrypt and AOSP. |
@prbprbprb Ok, let's call it case instead of issue.
There are situations when it's difficult to promote changes for the current architecture. It was already in use and _ underscores in subdomains was part of the architecture. Also the server uses *. wildcard ssl certificate and quick check on https://www.ssllabs.com/ssltest/ for the url where subdomain contains underscore showing that all is fine for that url. And stackoverflow answer https://stackoverflow.com/a/2183140 quoted that from RFC point of view underscores in the subdomain are also valid. I suppose that you are probably looking further to protect people from developing not secured apps for the google play, but probably it is possible to leave some space to allow people to change their SSL context for that kind of situation. |
Crash on OkHttp 3.12.9 and 3.12.10 when a URL subdomain contains underscores. Reproducible on API 29 (Android 10) emulator but not API 28 (Android 9).
On OkHttp 3.12.8 it errors but does not crash. Similarly OkHttp 3.12.10 errors but does not crash on Android 9.
Reproduce by making a GET request to an HTTPS URL with underscore in subdomain.
I saw old issue #242 closed saying underscores aren't allowed in hostnames, which may be technically true, but they are sometimes seen in the wild. Examples:
Logcats:
OkHttp 3.12.10 on Android 10:
OkHttp 3.12.8 on Android 10, error but no crash:
The text was updated successfully, but these errors were encountered: