Skip to content

For https: to work on Android #1103

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 5 commits into from
Jul 10, 2017
Merged

For https: to work on Android #1103

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
674fc16
For https: to work on Android
johnno1962 Jul 9, 2017
0c3a45f
For debug logging on Android
johnno1962 Jul 9, 2017
712f270
UNSAFE_SSL_NOVERIFY
johnno1962 Jul 9, 2017
9655700
Use setter rather than environment
johnno1962 Jul 10, 2017
5890353
Use setter rather than environment - lint
johnno1962 Jul 10, 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
16 changes: 15 additions & 1 deletion Foundation/NSURLSession/http/EasyHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,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
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "accessible"

// at the path pointed to by the URLSessionCAInfo env var.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs to be updated.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks again @xwu, anything else? Naming conventions etc? I’ll raise another PR.

        #if os(Android)
            // See https://curl.haxx.se/docs/sslcerts.html
            // For SSL to work you need a "cacert.pem" to be accessible
            // at the path set by URLSession.setCAInfoFile( <string> ).
            // Downloadable here: https://curl.haxx.se/docs/caextract.html
            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()
                }
            }
        #endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you ask... :P

  • These should probably be named _CAInfoFilePath and setCAInfoFilePath, as it's not the file but the file path (versus, say, the file URL).
  • Style nit: unsure why you're indenting this block between #if and #endif but not any of the others you added.
  • Style nit: why have a blank line here and before the closing brace?
  • Nit: if you're going to log instead of print from printLibcurlDebug, shouldn't it be logLibcurlDebug?

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Roger, not much I can do about the last one. There is no stdout on Android. Can you check johnno1962b@d3805cb please and I’ll raise a new PR tomorrow. Thanks again! I updated the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on URLSession.setCAInfo(filePath: path)?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to follow the naming conventions put forth for Foundation API;

  1. if you have a setter you should have a getter (unless it is absolutely justifiable that it can only be set)
  2. it should be a URL not a path
  3. don't use abbreviations like CA, instead it should be probably named URLSession.defaultCertificateAuthorityFile or something along those lines so that it does what it says on the tin.
  4. it should probably validate that the url is a valid file path url, and perhaps it should be nullable?
  5. it should probably also be atomic (using some sort of lock to ensure thread safe access)

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnno1962 👍 , but for the full monty Philippe certainly lists some important considerations.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

URLSession.sslCertificateAuthorityFile as a URL static var with validation enough? I’m not sure I can stretch to atomicity. I’m thinking I preferred the old API tho. I like URLSession .setCertificateAuthorityInfo(filePath: “UNSAFE_SSL_NOVERIFY”) from a health warning point of view.

#if os(Android)
extension URLSession {
    public static func setCertificateAuthorityInfo(filePath: String) {
        free(_EasyHandle._CAInfoFilePath)
        filePath.withCString {
            _EasyHandle._CAInfoFilePath = strdup($0)
        }
    }
    public static var sslCertificateAuthorityFile: URL? {
        set(value) {
            if value == nil {
                free(_EasyHandle._CAInfoFilePath)
                _EasyHandle._CAInfoFilePath = strdup("UNSAFE_SSL_NOVERIFY")
                return
            }
            guard value?.isFileURL == true else { return }
            value!.path.withCString {
                free(_EasyHandle._CAInfoFilePath)
                _EasyHandle._CAInfoFilePath = strdup($0)
            }
        }
        get {
            return URL(fileURLWithPath: String(cString: _EasyHandle._CAInfoFilePath!))
        }
    }
}
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Validation should check that it's a file URL that actually exists. When that fails, the setter should do something other than return silently. There should be one API, not two, so sslCertificateAuthorityFile should be the only way to set the underlying stored property. And the underlying property should probably be renamed to match.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

OK, a version with no more dummy values left over from being an env var..

#if os(Android)
extension URLSession {
    /// See https://curl.haxx.se/docs/sslcerts.html
    /// For SSL to work you need a "cacert.pem" to be accessible at the
    /// by setting URLSession.sslCertificateAuthorityFile to a file URL.
    /// Downloadable here: https://curl.haxx.se/docs/caextract.html
    /// Alternatively you can bypass SSL peer verification altogether:
    /// URLSession.unsafeBypassSSLPeerVerify = true (not advised)
    fileprivate static var _sslCertificateAuthorityFile: UnsafeMutablePointer<Int8>?
    public static var unsafeBypassSSLPeerVerify = false
    public static var sslCertificateAuthorityFile: URL {
        set(value) {
            unsafeBypassSSLPeerVerify = false
            free(_sslCertificateAuthorityFile)
            guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
                NSLog("*** sslCertificateAuthorityFile not FileURL or does not exist ***")
                _sslCertificateAuthorityFile = nil
                return
            }
            value.path.withCString {
                _sslCertificateAuthorityFile = strdup($0)
            }
        }
        get {
            if let value = _sslCertificateAuthorityFile {
                return URL(fileURLWithPath: String(cString: value))
            }
            else {
                return URL(fileURLWithPath: "NOVALUE")
            }
        }
    }
}
#endif

// Downloadable here: https://curl.haxx.se/ca/cacert.pem
if let caInfo = getenv("URLSessionCAInfo") {
Copy link
Member

@pushkarnk pushkarnk Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ProcessInfo.processInfo.environment is another way (Swifty) to access env vars, but I am not sure if it offers benefits in this context.

Copy link
Contributor

Choose a reason for hiding this comment

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

umm isn't this a security hole? If someone sets URLSessionCAInfo then they can circumvent the root certificate authorities? Thusly making all traffic vulnerable to being spoofed for certificates.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Without providing a URLSessionCAInfo file SSL doesn’t work at all using libcurl on Android. It doesn’t seem to have an idea about where the android root CA certs are stored and how to use them.

Copy link
Contributor

Choose a reason for hiding this comment

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

is there another way we can obtain the path other than env? My worry is that is a global state of the operating system that is publicly writable; so it could easily be hijacked by a malicious actor and circumvent root certificates.

I would presume that most android projects would be an APK that would load the libraries; can we add a JNI call or something like that so that once libFoundation.so is loaded the call is made to assign the appropriate path (as per the trust of the application).

Alternatively if there was some way we could grab that from the OS that would be preferable. Since I would assume that the cacert.pem would be stored as read only unless you are root.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Fair enough, It does bear all the hallmarks of an attack vector of some sort. Would a static public setter (Android only) on URLSession be OK? Any preference for the name? Grabbing it from the OS would be a bit of a rewrite somewhere in libcurl which I don’t fancy taking on. Android does not store Root CA’s in .pem format as far as I can see: https://stackoverflow.com/questions/4461360/how-to-install-trusted-ca-certificate-on-android-device

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

New version updated with setter i.e. URLSession.setCAInfoFile( "/mnt/sdcard/cacert.pem" )

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()
}
}
#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 @@ -268,7 +282,7 @@ fileprivate func printLibcurlDebug(handle: CFURLSessionEasyHandle, type: CInt, d

fileprivate func printLibcurlDebug(type: CFURLSessionInfo, data: String, task: URLSessionTask) {
// libcurl sends is data with trailing CRLF which inserts lots of newlines into our output.
print("[\(task.taskIdentifier)] \(type.debugHeader) \(data.mapControlToPictures)")
NSLog("[\(task.taskIdentifier)] \(type.debugHeader) \(data.mapControlToPictures)")
}

fileprivate extension String {
Expand Down