Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Adds Support For Loading Native Libraries From Alternative Path #86

Merged
merged 2 commits into from
Sep 11, 2016

Conversation

thorbenprimke
Copy link
Contributor

Summary: In order to support not shipping the native libraries
with an APK and instead downloading the architecture specific
.so files from the network and storing them in an app's data
folder, this adds support for setting an alternative lib search
path via the CardIONativeLibsConfig.

Test Plan:

  • Verified that manual entry screen is displayed when native libs are not downloaded / available.
  • Verified that setting the alterantive path works and the camera screen is shown.

Summary: In order to support not shipping the native libraries
with an APK and instead downloading the architecture specific
.so files from the network and storing them in an app's data
folder, this adds support for setting an alternative lib search
path via the `CardIONativeLibsConfig`.

Test Plan:
 - Verified that manual entry screen is displayed when native libs are not downloaded / available.
 - Verified that setting the alterantive path works and the camera screen is shown.
@lkorth
Copy link
Member

lkorth commented Jun 24, 2016

If the native libraries are not present, does CardIOActivity#canReadCardWithCamera correctly return false?

/**
* A config class that is used to specify an alternative search path for the native libraries.
*
* It's primary use case is for when the libraries are loaded over the network instead of being
Copy link
Member

Choose a reason for hiding this comment

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

Its, not it's.

@thorbenprimke
Copy link
Contributor Author

thorbenprimke commented Jun 24, 2016

@lkorth Yes, because manualFallbackForError is set to false. I tested it with both the config not being set as well as providing an incorrect alternative search path (in both cases the manual entry screen is shown).

Since CardIOActivity#canReadCardWithCamera is a static method, I chose the config class solution rather than putting a meta-data entry into the AndroidManifest (which requires Context to access it).

@lkorth
Copy link
Member

lkorth commented Jun 24, 2016

Going forward, at some point a new major version is going to be required and canReadCardWithCamera will need to take a context to use some new methods on the Camera2 API.

@thorbenprimke
Copy link
Contributor Author

Ah okay. Perhaps at that point this could be changed as well. The config solution seemed clean but the meta-data one would be a bit nicer (I think).

Also tested with ./gradlew clean :card.io:assembleRelease to ensure that no additional proguard entries are needed.

@thorbenprimke
Copy link
Contributor Author

@lkorth, @braebot - is this PR acceptable / mergeable?

@thorbenprimke
Copy link
Contributor Author

friendlyping

@braebot
Copy link
Member

braebot commented Jul 28, 2016

Sorry for the late response. I'm concerned about the security implications of this change. Could a malicious actor replace these downloaded files easier than an apk?

@thorbenprimke
Copy link
Contributor Author

Yes, possibly if they know the path in the app's directory (and the device is rooted). As far as I know, app A cannot modify app B's data directory (unless they share the same id).

In our case, I verify the lib files integrity on each application start by matching the files against expected MD5 hashes to ensure file integrity. As far as I know, it's possible to have two files with the same Hash but I'm not sure if that's possible with compiled files (unless they can be altered without corruption).

If a device is rooted though, it seems that a malicious actor could replace the (APK) packaged so files in the extracted APK directory just as easy as replacing them in the app's data directory.

@braebot braebot merged commit 20eeb96 into card-io:master Sep 11, 2016
@braebot
Copy link
Member

braebot commented Sep 11, 2016

Agree. I'll go ahead and merge this PR, and it will go out in the next release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants