Potential security vulnerability in passing Intents via BBQ
iainmcgin opened this issue · 6 comments
I've been doing another pass of security review over the code and protocol and have spotted a potential security vulnerability for older Android devices using OpenYOLO. This relies on exploitation of deserialization bugs, such as CVE-2015-3837, where deserialization of Intent extras leads to an injection attack. This particular bug affects Android 5.1.1 and lower, if the fix has not been specifically back-ported by the manufacturer. It seems unlikely that this is the only deserialization bug in Android, so it's worth considering how to mitigate this class of attacks entirely.
How to exploit the current OpenYOLO protocol
During the BBQ phase of the retrieve process, providers respond to requests for credentials by sending a serialized Intent if they have credentials available. At this phase, any app can respond, regardless of status as a "known" credential provider. As such, a malicious app could respond to a BBQ query for a credential and provide an Intent crafted to trigger a deserialization bug, potentially resulting in remote code execution as is the case in CVE-2015-3837.
The good news
According to Roee Hay's talk on Android serialization vulnerabilities at RSAConf 2016, most devices are now patched against CVE-2015-3837. However, as other deserialization bugs are possible, it makes sense to pursue mitigation where it does not disrupt the core functionality of OpenYOLO.
Removing the vulnerability entirely
The protocol was designed to allow returning an Intent, so that the provider could potentially include some pre-loaded information that would help in handling the subsequent Intent invocation to retrieve the credential. This extra information is of little interest to the client; as such, an alternative approach is possible:
- Treat the data returned by BBQ as an opaque byte array.
- When the decision is made to communicate with the provider to retrieve a credential, have the client craft the Intent itself - this is already possible, as this form of direct invocation is already specified in the protocol.
- The data returned via BBQ can be attached to this Intent as a byte array extra, leaving the responsibility of the data format and deserialization entirely in the hands of the provider.
This completely avoids the need to deserialize data in the client, removing this class of exploit.
As far as I'm aware, none of the providers are currently returning extra data in the Intents they return via BBQ, so it should be possible to make the necessary change in the client library right now without breakage. The protocol spec and spi should also be updated to clarify the nature of the data returned via the broadcast stage of credential retrieval.
Can this vulnerability affect providers?
It seems plausible that CVE-2015-3837 and similar deserialization bugs could affect providers too, where a malicious client sends an Intent with extra data intended to execute code within the context of the provider. It's not clear to me if there is anything specific that a provider could to to protect itself from this vulnerability, which would also apply if any of its other exported activities reads received Intent extras. One approach to protect against loading unexpected data could be to override the class loader for received intents, via
Intent.setExtrasClassLoader,
such that only a whitelisted set of classes may be used during deserialization. I haven't verified that this approach works, but it is worth further exploration.
FYI @StanKocken @nerdyverde, I'm working to patch this and release 0.3.0 ASAP based on my suggested approach above.
I should actually make clear that this vulnerability is not currently triggered in the 0.2.1 release - the vulnerability only exists if the client actually reads the intent extra data - the extra data bundle is lazy-loaded by the implementation of Intent, and we do not currently look at the Intent data. However, out of an abundance of caution I want to close off the possibility that this will ever be an issue by changing the protocol as described.
Changing the handling now so that this can't be an issue in the future makes sense to me.
Patch is in.
We should keep this open or open a new issue to track the removal of the client using the deprecated response intent.
Let's open a new issue.