Skip to content

Required For HTTPS URLSession requests to work on Android #1264

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

Merged
merged 2 commits into from
Oct 16, 2017
Merged

Required For HTTPS URLSession requests to work on Android #1264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e992b8d
For HTTPS on Android III
johnno1962 Oct 13, 2017
739f4d3
Merge branch 'master' into HTTPS4Android
johnno1962 Oct 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 13 additions & 29 deletions Foundation/URLSession/http/EasyHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ internal final class _EasyHandle {
fileprivate var pauseState: _PauseState = []
internal var timeoutTimer: _TimeoutSource!
internal lazy var errorBuffer = [UInt8](repeating: 0, count: Int(CFURLSessionEasyErrorSize))
#if os(Android)
static fileprivate var _CAInfoFile: UnsafeMutablePointer<Int8>?
#endif

init(delegate: _EasyHandleDelegate) {
self.delegate = delegate
Expand Down Expand Up @@ -172,20 +169,20 @@ extension _EasyHandle {
let protocols = (CFURLSessionProtocolHTTP | CFURLSessionProtocolHTTPS)
try! CFURLSession_easy_setopt_long(rawHandle, CFURLSessionOptionPROTOCOLS, protocols).asError()
try! CFURLSession_easy_setopt_long(rawHandle, CFURLSessionOptionREDIR_PROTOCOLS, protocols).asError()
#if os(Android)
// See https://curl.haxx.se/docs/sslcerts.html
// For SSL to work you need "cacert.pem" to be accessable
// at the path pointed to by the URLSessionCAInfo env var.
// Downloadable here: https://curl.haxx.se/ca/cacert.pem
if let caInfo = _EasyHandle._CAInfoFile {
if String(cString: caInfo) == "UNSAFE_SSL_NOVERIFY" {
try! CFURLSession_easy_setopt_int(rawHandle, CFURLSessionOptionSSL_VERIFYPEER, 0).asError()
}
else {
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionCAINFO, caInfo).asError()
}
#if os(Android)
// See https://curl.haxx.se/docs/sslcerts.html
// For SSL on Android you need a "cacert.pem" to be
// accessible at the path pointed to by this env var.
// Downloadable here: https://curl.haxx.se/ca/cacert.pem
if let caInfo = getenv("URLSessionCertificateAuthorityInfoFile") {
Copy link

@ephemer ephemer Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for making this change, I was able to get SSL working on Android today using this env variable. Is this really the nicest way about this though? The fact that it's alterable like this is kind of confusing as an API consumer, plus it looks a lot like a security risk to me.

https://curl.haxx.se/docs/sslcerts.html details the option of building libcurl with the flag --with-ca-bundle=FILE. So from what I understand, we could remove this code complexity entirely by just building libcurl with the certificate mentioned in your comment here. That would also save the need to bundle this certificate file in the APK (another potential security risk).

What do you think @johnno1962 ?

Copy link
Contributor Author

@johnno1962 johnno1962 Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @ephemer, there was a lot of to-ing and fro-ing on the PR. The original PR #1103 which was merged was an API which was developed in #1113 until it was decided we couldn't add api to foundation and we switched back to the original proposal. As far as one can know I don’t think it’s a security issue as if you use it it will be set explicitly in the code and there will be no opportunity to root the device and override it by setting the var maliciously for example. I wasn’t aware you could hard code it into libcurl but that doesn’t help as it wouldn't have a fixed path on the menagerie that is android.

If I were to do it all again and known #1113 wouldn’t be merged for three months. I would have left it as it was after #1103 but by then the toolchain was committed to an env var.

Copy link

@ephemer ephemer Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @johnno1962, if I understand correctly, this is essentially a process-wide public variable that can be changed at any time before any URLSession is created. That certainly seems like a security risk to me. I don't know enough about SSL (or the finer details of Android for that matter) to know to what extent this may be risky, but it'd be hard to argue that it's secure. That is, unless you're calling setenv religiously before every initialization of URLSession, but that seems like an unreasonable expectation to have of Foundation's API consumers.

From what I understand, implementing --with-ca-bundle wouldn't mean hard-coding a file path into libcurl, but rather incorporating the certificate bundle into libcurl.so statically. If that's not the case I agree it'd be pointless though. I'll do some more testing on this tomorrow. If a static solution does turn out to be possible, would you be ok with this code being removed?

EDIT: If statically including the file doesn't work (and it does search a device-specific path after all), what about telling it to search in /system/etc/security/cacerts/ via --with-ca-path=?

Copy link
Contributor Author

@johnno1962 johnno1962 Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Having fought hard to get this small change in I’d rather it wasn’t rolled back! Besides, I don’t see the alternative though I’m open to practical alternatives. You can hard code the path to the certificates in libcurl if there was a standard one but the problem is they are in the wrong format on most android devices ass libssl1.0+ moved from a MD5 hash as the filename to to a SHA1 http://www.javacms.tech/questions/1728116/how-to-make-ssl-peer-verify-work-on-android. Including the certificate bundle in libcurl i.e. the toolchain seems like a very bad idea to me.

Whether this presents a security risk, not being an expert this is something I’d rather not express an opinion on but I doubt it. If you can’t rely on an apps code bundle all bets are probably off anyway. In this case, using an environment variable vs. an api doesn’t make much difference. If you find a better way though then all power to your elbow.

Copy link

Choose a reason for hiding this comment

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

Hi @johnno1962, thanks for your patience. I've just spent a couple of hours reading into this and have come to the conclusion that you may well have the only reasonable solution that is usable for this. I am still sceptical that it's entirely secure but now equally as sceptical that there's any other way on Android.

Do you have an idea of how best to use this in an APK? libcurl expects an absolute path.

Copy link
Contributor Author

@johnno1962 johnno1962 Nov 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @ephemer, checkout https://github.com/SwiftJava/swift-android-kotlin/blob/master/app/src/main/java/com/example/user/myapplication/MainActivity.kt#L48 & https://github.com/SwiftJava/swift-android-kotlin/blob/master/app/src/main/swift/Sources/main.swift#L60 for examples on how it all comes together. You need to have the cacert file as a resource, copy it to the app’s cacheDir and set the environment variable to that path in Swift. There just didn’t seem to be a better way to do it.

Copy link

Choose a reason for hiding this comment

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

Thanks for the info. Ok, so you're basically copying the .pem from the APK's resources into the app cache (which has an absolute path), accessed via the applicationContext. It's a pain in the butt but I guess there's not much else we could do other than loading the .pem ourselves and adding the certs manually.

Maybe it's worth us putting in an issue to the NDK asking them to provide a libcurl (with SSL, using the system certificates)? It might take a few years, but hey..

if String(cString: caInfo) == "INSECURE_SSL_NO_VERIFY" {
try! CFURLSession_easy_setopt_long(rawHandle, CFURLSessionOptionSSL_VERIFYPEER, 0).asError()
}
else {
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionCAINFO, caInfo).asError()
}
#endif
}
#endif
//TODO: Added in libcurl 7.45.0
//TODO: Set default protocol for schemeless URLs
//CURLOPT_DEFAULT_PROTOCOL available only in libcurl 7.45.0
Expand Down Expand Up @@ -632,19 +629,6 @@ extension _EasyHandle._CurlStringList {
}
}

#if os(Android)
extension URLSession {

public static func setCAInfoFile(_ _CAInfoFile: String) {
free(_EasyHandle._CAInfoFile)
_CAInfoFile.withCString {
_EasyHandle._CAInfoFile = strdup($0)
}
}

}
#endif

extension CFURLSessionEasyCode : Equatable {
public static func ==(lhs: CFURLSessionEasyCode, rhs: CFURLSessionEasyCode) -> Bool {
return lhs.value == rhs.value
Expand Down
51 changes: 0 additions & 51 deletions android/README.md
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions android/builder.sh
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions android/install.sh
View file
Open in desktop

This file was deleted.

22 changes: 0 additions & 22 deletions android/package.sh
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions android/prepare.sh
View file
Open in desktop

This file was deleted.

40 changes: 0 additions & 40 deletions android/updater.sh
View file
Open in desktop

This file was deleted.