wultra / ssl-pinning-android

Android SDK for our dynamic SSL pinning
Apache License 2.0
60 stars 8 forks source link

Updating CertStore fails in case of multiple certificates when any one of them is invalid #32

Closed divyeshdhedhi closed 4 years ago

divyeshdhedhi commented 4 years ago

Error - Updating CertStore in case of multiple certificates from hosted JSON fails if any single certificate is invalid with result : INVALID_DATA on calling below method certStore.update(UpdateMode.DEFAULT, object : DefaultUpdateObserver() { }

There is an another issue I faced while resolving the above as: The CertStore doesn't get updated after updating the hosted JSON file, it needs 2-3 restarts of the app (as I am updating the certificates every time app starts)

The JSON is hosted on - https://demo7361063.mockable.io/test

Any idea what I can do in both situations?

hvge commented 4 years ago

Hello Divyesh, thank you for your report. I'll try to explain each of your points.

1. INVALID_DATA

We're quite more paranoid here, so any invalid partial certificate we treat as a malicious attempt. This is due to fact, that we provide our software mostly for the financial clients (like banks), so the paranoid behavior is very expected. We simply don't trust the data, once some signature is invalid.

The best what you can do is to always provide a valid JSON with valid certificates.

2. Delayed update

This is due the fact, that the library locally caches an already loaded certificates. You can use UpdateMode.FORCED to override such behavior on your application's startup. See UpdateMode.kt for more details.

Btw, it's impossible to test your JSON without knowing the public key you're using in your application.

hvge commented 4 years ago

I think that in case that you do the pinning for the various domains and you expect that each domain update its TLS certificate independently, then you can create a multiple CertStore instances with their own remote JSONs. Basically, failure on one domain will not affect another. It requires more work, but that's only one workaround I can recommend to you.

But I would still recommend to you - just keep the JSON valid. Our clients usually have some automatic job, that update JSON once the TLS certificate is modified. Especially if "Let's Encrypt" is used.

petrdvorak commented 4 years ago

Hello @divyeshdhedhi. Could you maybe tell us more abou your use-case? In most cases, our customers use just one private key for signing and related public key for signature verification. Do you need to use multiple keypairs in your setup?

This could be a reasonable hint for possible enhancement.

Fenil15 commented 4 years ago

@hvge However, the same above usecase is working fine for ssl-pinning-ios Is this same behaviour yet to be implemented for ios?

@petrdvorak Our App provides a platform to develop and deploy miniapps over the air. Now this miniapps can be developed by our team (In that as well backend will be hosted in multiple domains) or by our clients team where they may be hosting there backend. In order to make the communication more secure SSL pinning will help at the same time the certificate ownership is not always in our hands. So we are fine if a particular certificate is expired, then that particular miniapp which is communicating to that domain will not work but this will cause entire App to stop working which is critical to our reliability. Hence the need to have this flexibility.

Now if you extrapolate this to multiple clients, having multiple ssl pinning files will add on to our overhead as well. Providing a flexibility to UpdateCertstore using optional parameter should help as signature verification takes places eventually.

Hope this provides clarity!

hvge commented 4 years ago

@Fenil15 Thank you for that clarification. For your scenario it makes sense to update the cert store's database with all possible valid hashes. So, I'm looking at the code and unfortunately, both platforms use the similar logic. The error is always returned when there's something wrong with the received JSON. The returned error depends on whether there's a deserialization problem (INVALID_DATA) or an invalid signature (INVALID_SIGNATURE). This doesn't correspond with your findings. I cannot test your data in detail without a public key you're using for ECDSA validation. So, at first, reveal your public key, that we can move further in the problem analysis.

divyeshdhedhi commented 4 years ago

Update on the raised issue: @hvge Actually we were getting the INVALID_SIGNATURE while any of the domains was invalid and INVALID_DATA while there was a mismatch in fingerprint and signature (which will not be the case ideally).

we have worked on your earlier provided feedback, i.e we are now maintaining multiple certstore instances and working on keeping all the hosted JSONs valid.

Also, we have noticed some behavior as android clears the stored cerstore from a session at our logout feature or clearing data of the app but ios is not following the same flow i.e certstore is not getting cleared after logout or even after uninstalling the app, is there a different location where the certsore is being stored outside of the application scope? (this observation was done on iOS simulator)

hvge commented 4 years ago

@divyeshdhedhi On iOS, we're using iOS keychain to store certstore content. So, the content should be removed on app uninstall (since iOS 10.3). iOS Simulator's behavior might be a quite different to the real device. I remember that for older iOS versions, the keychain was shared across all apps installed in simulator. I don't know whether Apple fixed this or not. So, don't trust your results from simulator :)

I don't know what do you mean by "logout" feature. We don't have any state, or logout implemented in the library. It's completely up to your application to implement such things. Our library only maintains a list of hashes to allow SSL pinning. Even TLS session is completely in your hands. The URLSession should invalidate cached TLS connection once the object is destroyed.

hvge commented 4 years ago

Closing as "not a bug"