duddu / cordova-plugin-antitampering

Verify the integrity of cordova static assets - Android / iOS
MIT License
22 stars 22 forks source link

The check is not effective #2

Closed Slumber86 closed 7 years ago

Slumber86 commented 7 years ago

In the hypotesis that an attacker can tamper the app, the first thing will be to disable the anti tampering control. (js files are just plaintext). So the plugin should auto-run the tampering on the app start and check and raise a fatal exception to stop the execution immediately

duddu commented 7 years ago

Yes you're right, I already planned to address the topic and I will definitely start to work on that very soon. The reason why the plugin born this way (with the javascript callback only) is that not every project has the same security requirements. Pick the unfortunate case where the client wants at all costs (sigh) to show something on the client-side after the various security checks, and prefers to take the risk you describe, in order to be able to easily manage some fallback UIs via js. I personally addressed this topic first by using this plugin together with the uglify and encryption of static resources (encrypted on cordova build and decrypted at runtime with java/obj-c); but I know there is no real way to protect the app unless you raise a fatal exception as soon as possibile. I've waited to put that on code because I would like to handle the choice by a plugin preference (aka variable from config.xml): by default, the plugin stops the execution if the check fails, but a variable let you choose to continue the execution and give javascript a callback. A solution like that would work for you? Any comment or observation are very welcome

Slumber86 commented 7 years ago

Also the 100% native solution isn't bulletproof on android. (Easy to decompile, maybe obfuscating the code can help) The config.xml is also not sicure and still as-easy to tamper as javascript. Also understand where to save the hash is problematic. And you made the right choice saving it in the source code.

Slumber86 commented 7 years ago

Last suggestion: replacing SHA1 with SHA-256

duddu commented 7 years ago

By using the config.xml I meant using a variable from the config in the before-compile hook to switch on/off the portion on source code that throws the fatal exception (otherwise, we are still in the js world, hence back to square one), so that the platform code gets only the real native code to be executed (no booleans around!). What do you think about it? As regards the hash, am I not already using sha-256?

I'll try to commit the changes within this week (with a PR linked to this issue), any suggestion will be really appreciated. Thanks a lot!

Slumber86 commented 7 years ago

Sorry, I have mistaken the algorithm :)

duddu commented 7 years ago

I made a draft of the enhancement in this PR: #3 Now by default the plugin stops the execution of the app after a tampering is detected. Instead, installing the plugin with the variable ENABLE_CORDOVA_CALLBACK=true will disable the automatic check (in order to manually execute the check from javascript). Does it seem right? I'll put all the details in the readme on the next release.

Please have a look at the commit, let me know what you think.

Slumber86 commented 7 years ago

Yeah, the fix seems right, i have done a similar thing yesterday, but the problem is that for tampering the app is enough to edit the plaintext config.xml I tried to baypass the protection and done it in nearly half an hour on android, decompiling the app with apktool, re-building it and signing with a debug key. If you want the details i'm happy to share the command sequence in private.

Also, launching the check directly in MainActivity.java make the tampering a bit more complicated, but edit smali assembler to avoid a call is dead-simple, so, I don't expect to be a big step forward in security

Slumber86 commented 7 years ago

Can you merge the PR, so i can try out the fix? After maybe I will open a PR for android app-store key checking

duddu commented 7 years ago

Ok, I'll merge the pr in a few hours

duddu commented 7 years ago

btw, you can test the fix from the branch itself: cordova plugin add https://github.com/duddu/cordova-plugin-antitampering#feature/stop-tampered-execution

duddu commented 7 years ago

Just a few notes about your previous comment: It is true that an anti-tampering detection on the assets alone is not enough. It's just another layer to cross for the attacker (like almost any other app-integrity check). I think the goal is to have the best security checks that can fit the form of a cordova plugin, usable out of the box in every project. On Android, decompiling from apk to java (via jadx, apktool, jeb, etc) will always be pretty easy for any attacker; we can just try to delay as far as possible...

Anyway, I usually do also the following:

I am planning to integrate at least the last two checks of that list in this plugin, since there are some plugins around that provides this features, but no one is really complete and self-sufficient - and having these controls in one single plugin would be useful to me. What do you say? Again, any suggestions or PRs are really welcome

Slumber86 commented 7 years ago

Yeah, all these are good pratices but i'm afraid that as far is done in a cordova plugin, disabling it would be too easy. The only bullet-proof solution would be the server-side app-key and asset hash validation, but is complex for a number of reasons, so this solution would be better than nothing :)

duddu commented 7 years ago

I'm closing the issue, but please let me know if you notice any problem on the merged code. Thank you!

vishwajeetsingh403 commented 5 years ago

How can we implement the same plugin in Angular(2 or above) while using ionic 3.

KITSDavideDoronzo commented 5 years ago

Hi @vishwajeetsingh403, you can still consume this plugin with plain JavaScript, without having the ad-hoc angular 2+ interface. The plugin object, with old fashioned callbacks, is already exposed to the window scope. If you have compatibility issue feel free to open another ticket.