Few enhancements to gadget injection into APK (patchapk) #586
Conversation
From https://developer.android.com/studio/command-line/aapt2 > Prior to AAPT2, AAPT was the default version of Android Asset Packaging Tool and it has now been deprecated. Although AAPT2 should immediately work with older projects, this section describes some behavior changes that you should be aware of.
| help='Do not clean temporary files once finished.', show_default=True) | ||
| @click.option('--enable-debug', '-d', is_flag=True, | ||
| @click.option('--enable-debug', '-d', is_flag=True, default=True, |
There was a problem hiding this comment.
I'd prefer this to be false by default.
| @@ -255,18 +255,25 @@ def patchipa(source: str, gadget_version: str, codesign_signature: str, provisio | |||
| 'specified, the latest version will be used.'), default=None) | |||
| @click.option('--pause', '-p', is_flag=True, help='Pause the patcher before rebuilding the APK.', | |||
| show_default=True) | |||
| @click.option('--skip-cleanup', '-k', is_flag=True, | |||
| @click.option('--skip-cleanup', '-k', is_flag=True, default=True, | |||
There was a problem hiding this comment.
This flag would need to be changed quite a bit now. The current behaviour is a flag, meaning the default false is flipped to true when the flag --skip-cleanup exists. Flipping it means we need to change this to --cleanup or something.
That said, this needs to stay it's default please.
There was a problem hiding this comment.
Not sure to understand this comment (and the following ones) about switching to flags. Isn't this the current way the code is written in master branch? https://github.com/sensepost/objection/blob/master/objection/console/cli.py#L270-L271
There was a problem hiding this comment.
@Phyks you are correct that they are currently flags, it's not that they need to be switched to flags. Leonza was saying that changing the default to true would change the option's meaning. Simply, if you were to specify "--skip-cleanup" it would be performing cleanup instead of skipping it (since skipping it would be the default).
That said, he wished to keep this option as is since it would be better to perform cleanup by default. (I.e. reverting this change)
| @click.option('--skip-resources', '-D', is_flag=True, default=False, | ||
| help='Skip resource decoding as part of the apktool processing.', show_default=False) | ||
| help='Skip resource decoding as part of the apktool processing.', show_default=True) |
There was a problem hiding this comment.
Similar to --skip-cleanup, this needs to change to something like --decode-resources.
| @click.option('--target-class', '-t', help='The target class to patch.', default=None) | ||
| @click.option('--use-aapt2', '-2', is_flag=True, default=False, | ||
| help='Use the aapt2 binary instead of aapt as part of the apktool processing.', show_default=False) | ||
| @click.option('--use-aapt2', '-2', is_flag=True, default=True, |
There was a problem hiding this comment.
Like --skip-cleanup, this flag now needs to specifically set the command to use 1.
| @@ -211,6 +213,8 @@ def __init__(self, skip_cleanup: bool = False, skip_resources: bool = False, man | |||
| self.skip_resources = skip_resources | |||
| self.manifest = manifest | |||
|
|
|||
| self.architecture = None | |||
There was a problem hiding this comment.
Don't think this is used for anything.
| for lib in existing_libs_in_apk: | ||
| libnative = lief.parse(os.path.join(libs_path, lib)) | ||
| libnative.add_library(self.libfridagadget_name) # Injection! | ||
| libnative.write(os.path.join(libs_path, lib)) |
There was a problem hiding this comment.
If I have this right, I think we are going to be injecting into every native library here? I'm not sure that is what we want :) Not sure what the right approach would be here to choose a target. Maybe random?
| @@ -9,3 +9,4 @@ requests | |||
| flask | |||
| pygments | |||
| litecli>=1.3.0 | |||
| lief | |||
There was a problem hiding this comment.
Ideally we should pin this to a major version (I'm going to do that with the rest of the dependencies soon, fwiw).
Hi,
Here are a few proposals for enhancements of the
patchapkfunction, in line with #582:-[x] Try to make the default value of flags and arguments clearer to the reader, through the
--helpinterface. Everything enabled by default should now be explicitly stated as such.libfrida-gadget.soby adding it in existing shared libraries if possible, and resort to Activity patching if this is not possible.frida-gadget-xxx-arm64.so.xz=>arm64) as provided architecture, it would now match a folder such asarm64-v8ainside the unpacked APK tree.aapt2for repacking. As far as I understand and experimented,aapt2should work out of the box for most cases, while regularaaptwould often fail on some resources. Is there a specific reason for keepingaaptas default?Best,