From bb412d39abe81b5fdf52973161a112efcbd25c41 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Tue, 12 Apr 2022 22:15:32 +0200 Subject: [PATCH] Fix obfuscated libs instrumentation (adcolony) (#307) --- CHANGELOG.md | 2 + .../wrap/visitor/WrappingVisitor.kt | 23 +++++++--- .../SentryPluginWithMinifiedLibsTest.kt | 1 + .../wrap/visitor/WrappingVisitorTest.kt | 43 +++++++++++++++++++ 4 files changed, 62 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a7f9f56..88cc45c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +* Fix: obfuscated libs instrumentation (adcolony) (#307) + ## 3.0.1 * Fix: obfuscated libs instrumentation (gms) (#303) diff --git a/plugin-build/src/main/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitor.kt b/plugin-build/src/main/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitor.kt index c6bb338f..206595bf 100644 --- a/plugin-build/src/main/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitor.kt +++ b/plugin-build/src/main/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitor.kt @@ -15,6 +15,7 @@ import org.objectweb.asm.commons.Method import org.objectweb.asm.tree.InsnNode import org.objectweb.asm.tree.MethodNode import org.objectweb.asm.tree.TypeInsnNode +import org.objectweb.asm.tree.VarInsnNode import org.slf4j.Logger class WrappingVisitor( @@ -46,18 +47,26 @@ class WrappingVisitor( } private val className = classContext.className.replace('.', '/') - private var newWithoutDupInsn = false + private var manuallyDuped = false private var varIndex = -1 override fun visitTypeInsn(opcode: Int, type: String?) { super.visitTypeInsn(opcode, type) if (opcode == Opcodes.NEW && type in targetTypes) { - val nextInsn = newInsnsForTargets.poll()?.next + val nextInsn = newInsnsForTargets.poll()?.next ?: return // in case the next insn after NEW is not a DUP, we inject our own DUP and flip a flag // to later store our wrapping instance with the same index as the original instance - if (nextInsn != null && (nextInsn as? InsnNode)?.opcode != Opcodes.DUP) { + val isNextInsnDup = (nextInsn as? InsnNode)?.opcode == Opcodes.DUP + // in case the next insn after NEW is DUP, but is followed by ASTORE, we do the same + // thing as above. This case cannot be written by a developer, but rather is produced + // by a code obfuscator/compiler, because the usage of uninitialized instance + // is prohibited by the java verifier (NEW should be followed by INVOKESPECIAL before + // using it, which is not the case with ASTORE) + val isDupFollowedByStore = + isNextInsnDup && (nextInsn.next as? VarInsnNode)?.opcode == Opcodes.ASTORE + if (!isNextInsnDup || isDupFollowedByStore) { dup() - newWithoutDupInsn = true + manuallyDuped = true } } } @@ -66,7 +75,7 @@ class WrappingVisitor( super.visitVarInsn(opcode, `var`) // capture the variable index of the instrumented type to later store our wrapped type // with the same index - if (opcode == Opcodes.ASTORE && newWithoutDupInsn && varIndex == -1) { + if (opcode == Opcodes.ASTORE && manuallyDuped && varIndex == -1) { varIndex = `var` } } @@ -177,10 +186,10 @@ class WrappingVisitor( replacement.descriptor, false ) - if (newWithoutDupInsn && varIndex >= 0) { + if (manuallyDuped && varIndex >= 0) { mv.visitVarInsn(Opcodes.ASTORE, varIndex) varIndex = -1 - newWithoutDupInsn = false + manuallyDuped = false } } } diff --git a/plugin-build/src/test/kotlin/io/sentry/android/gradle/SentryPluginWithMinifiedLibsTest.kt b/plugin-build/src/test/kotlin/io/sentry/android/gradle/SentryPluginWithMinifiedLibsTest.kt index 91249d16..de42accb 100644 --- a/plugin-build/src/test/kotlin/io/sentry/android/gradle/SentryPluginWithMinifiedLibsTest.kt +++ b/plugin-build/src/test/kotlin/io/sentry/android/gradle/SentryPluginWithMinifiedLibsTest.kt @@ -21,6 +21,7 @@ class SentryPluginWithMinifiedLibsTest : implementation 'com.google.android.play:core-ktx:1.8.1' implementation 'com.google.android.gms:play-services-vision:20.1.3' implementation 'com.google.android.gms:play-services-mlkit-text-recognition:18.0.0' + implementation 'com.adcolony:sdk:4.7.1' } """.trimIndent() ) diff --git a/plugin-build/src/test/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitorTest.kt b/plugin-build/src/test/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitorTest.kt index 1237be9d..988e2565 100644 --- a/plugin-build/src/test/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitorTest.kt +++ b/plugin-build/src/test/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitorTest.kt @@ -319,6 +319,49 @@ class WrappingVisitorTest { fixture.visitor.insnVisits.first() == InsnVisit(Opcodes.DUP) } } + + @Test + fun `when NEW insn with DUP followed by ASTORE - modifies operand stack with DUP and ASTORE`() { + val methodVisit = MethodVisit( + opcode = Opcodes.INVOKESPECIAL, + owner = "java/io/FileInputStream", + name = "", + descriptor = "(Ljava/lang/String;)V", + isInterface = false + ) + val firstPassVisitor = MethodNode(Opcodes.ASM9).apply { + visitTypeInsn(Opcodes.NEW, "java/io/FileInputStream") + visitInsn(Opcodes.DUP) + visitVarInsn(Opcodes.ASTORE, 1) + } + + fixture.getSut( + replacements = mapOf(Replacement.FileInputStream.STRING), + firstPassVisitor = firstPassVisitor + ).run { + visitTypeInsn(Opcodes.NEW, "java/io/FileInputStream") + visitInsn(Opcodes.DUP) + visitVarInsn(Opcodes.ASTORE, 1) + visitMethodInsn( + methodVisit.opcode, + methodVisit.owner, + methodVisit.name, + methodVisit.descriptor, + methodVisit.isInterface + ) + } + + // DUP was visited by our visitor + assertTrue { + fixture.visitor.insnVisits.size == 2 && + fixture.visitor.insnVisits.all { it == InsnVisit(Opcodes.DUP) } + } + // ASTORE was visited by our visitor in the end + assertTrue { + fixture.visitor.varVisits.size == 5 && + fixture.visitor.varVisits.last() == VarVisit(Opcodes.ASTORE, 1) + } + } } data class MethodVisit(