diff --git a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/Extensions.kt b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/Extensions.kt index 896f49756..6c52c2860 100644 --- a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/Extensions.kt +++ b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/Extensions.kt @@ -9,6 +9,7 @@ import com.intellij.psi.PsiVariable import com.intellij.psi.impl.compiled.ClsMethodImpl import com.intellij.psi.impl.source.PsiClassReferenceType import com.intellij.psi.impl.source.PsiImmediateClassType +import com.mparticle.lints.dtos.AllowedTypes import com.mparticle.lints.dtos.Constructor import com.mparticle.lints.dtos.Expression import com.mparticle.lints.dtos.MethodCall @@ -238,7 +239,7 @@ internal fun UExpression.resolveChainedCalls(returnValue: Boolean, instance: Exp } } -internal fun Pair<*, *>.resolveToEnum(): Enum<*> { +internal fun Pair<*, *>.resolveToEnum(): Enum<*>? { val className = when (first) { is ClassId -> "${(first as ClassId).packageFqName}.${ @@ -247,6 +248,9 @@ internal fun Pair<*, *>.resolveToEnum(): Enum<*> { is String -> first as String else -> null } + if (!AllowedTypes.isAllowed(className)) { + return null + } return className?.let { className -> val enumName = second.toString() val constructor = @@ -255,7 +259,7 @@ internal fun Pair<*, *>.resolveToEnum(): Enum<*> { .methods .first { it.name == "valueOf" } constructor.invoke(null, enumName) - } as Enum<*> + } as? Enum<*> } internal fun List.resolve(): List = map { it.resolve() } diff --git a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/AllowedTypes.kt b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/AllowedTypes.kt new file mode 100644 index 000000000..b5fb03cb0 --- /dev/null +++ b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/AllowedTypes.kt @@ -0,0 +1,31 @@ +package com.mparticle.lints.dtos + +/** + * The Data Plan detector needs to compute the message body of an event builder chain found in + * analyzed source, which requires resolving constructor/static calls via reflection. Reflection + * must never run against a class named by the analyzed source itself unless it's one of the + * mParticle DTO/builder types (or a small set of collection helpers commonly used inline to build + * attribute maps) - anything else is attacker-controlled input, not code we intend to execute. + */ +internal object AllowedTypes { + private val ALLOWED_CLASS_NAMES = + setOf( + "com.mparticle.MPEvent", + "com.mparticle.MPEvent\$Builder", + "com.mparticle.commerce.CommerceEvent", + "com.mparticle.commerce.CommerceEvent\$Builder", + "com.mparticle.commerce.Product", + "com.mparticle.commerce.Product\$Builder", + "com.mparticle.commerce.Promotion", + "com.mparticle.commerce.Impression", + "com.mparticle.commerce.TransactionAttributes", + "com.mparticle.MParticle\$EventType", + "java.util.HashMap", + "java.util.LinkedHashMap", + "java.util.ArrayList", + "kotlin.collections.MapsKt", + "kotlin.collections.CollectionsKt", + ) + + fun isAllowed(qualifiedClassName: String?): Boolean = qualifiedClassName != null && ALLOWED_CLASS_NAMES.contains(qualifiedClassName) +} diff --git a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/Constructor.kt b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/Constructor.kt index 1c7f5a241..af424cdd7 100644 --- a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/Constructor.kt +++ b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/Constructor.kt @@ -7,9 +7,12 @@ import org.jetbrains.uast.UCallExpression data class Constructor(override val parent: Expression, val methodName: String?, override val node: UCallExpression) : ParameterizedExpression { override var arguments: List = listOf() - override fun resolve(): Any? { + override fun resolve(): Any? = ResolutionGuard.guarded { val qualifiedClassName = node.receiverClassName()?.replace(".Builder", "\$Builder") + if (!AllowedTypes.isAllowed(qualifiedClassName)) { + return@guarded null + } val clazz = Class.forName(qualifiedClassName) val params: List = arguments.resolve() val argumentClasses = @@ -29,15 +32,15 @@ data class Constructor(override val parent: Expression, val methodName: String?, try { if (constructor != null) { if (params.size > 0) { - return constructor.newInstance(*params.toTypedArray()) + return@guarded constructor.newInstance(*params.toTypedArray()) } else { - return constructor.newInstance() + return@guarded constructor.newInstance() } } } catch (ex: Exception) { "no new Instance for $clazz.name, tried constructor: ${constructor?.name}" } - return clazz + return@guarded clazz } override fun forEachExpression(predicate: (Expression) -> Unit) { diff --git a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/MethodCall.kt b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/MethodCall.kt index 760743ee6..23615cf64 100644 --- a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/MethodCall.kt +++ b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/MethodCall.kt @@ -10,10 +10,10 @@ data class MethodCall(override val parent: Expression, val methodName: String?, arguments.forEach { it.parent = this } } - override fun resolve(): Any? { + override fun resolve(): Any? = ResolutionGuard.guarded { val instance = parent.resolve() if (instance == null) { - return null + return@guarded null } var matchingMethods = instance::class.java.methods @@ -33,12 +33,12 @@ data class MethodCall(override val parent: Expression, val methodName: String?, val arguments = arguments.resolve() val value = method.invoke(instance, *arguments.toTypedArray()) if (returnValue) { - return value + return@guarded value } else { - return instance + return@guarded instance } } - return null + return@guarded null } override fun equals(other: Any?): Boolean = node.equals((other as? MethodCall)?.node) diff --git a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/ResolutionGuard.kt b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/ResolutionGuard.kt new file mode 100644 index 000000000..e28c01dd6 --- /dev/null +++ b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/ResolutionGuard.kt @@ -0,0 +1,25 @@ +package com.mparticle.lints.dtos + +/** + * Bounds the depth of expression resolution. Some qualified call chains cause a resolved + * expression's parent to keep resolving into itself; without a bound that recurses until the + * stack overflows, which callers' try/catch(Exception) blocks don't catch. Throwing a regular + * exception once a generous depth is exceeded keeps that failure catchable. + */ +internal object ResolutionGuard { + private const val MAX_DEPTH = 50 + private val depth = ThreadLocal.withInitial { 0 } + + fun guarded(block: () -> T): T { + val current = depth.get() + if (current >= MAX_DEPTH) { + throw IllegalStateException("Expression resolution exceeded max depth of $MAX_DEPTH") + } + depth.set(current + 1) + try { + return block() + } finally { + depth.set(current) + } + } +} diff --git a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/StaticFactory.kt b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/StaticFactory.kt index eda586ac0..73fa071be 100644 --- a/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/StaticFactory.kt +++ b/tooling/custom-lint-rules/src/main/java/com/mparticle/lints/dtos/StaticFactory.kt @@ -9,8 +9,11 @@ class StaticFactory(val methodName: String?, override val node: UCallExpression) override val parent = RootParent(node) override var arguments: List = listOf() - override fun resolve(): Any? { + override fun resolve(): Any? = ResolutionGuard.guarded { val qualifiedClassName = (node.resolve()?.parent as? ClsClassImpl)?.stub?.qualifiedName + if (!AllowedTypes.isAllowed(qualifiedClassName)) { + return@guarded null + } val methods = HashSet() val clazz = Class.forName(qualifiedClassName) methods.addAll(clazz.declaredMethods) @@ -31,9 +34,9 @@ class StaticFactory(val methodName: String?, override val node: UCallExpression) } val arguments = arguments.resolve() method.isAccessible = true - return method.invoke(null, *arguments.toTypedArray()) + return@guarded method.invoke(null, *arguments.toTypedArray()) } - return null + return@guarded null } override fun forEachExpression(predicate: (Expression) -> Unit) { diff --git a/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/Constants.kt b/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/Constants.kt index bdd888f5d..411ed2517 100644 --- a/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/Constants.kt +++ b/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/Constants.kt @@ -22,12 +22,23 @@ object Constants { @Language("JAVA") const val APPLICATION_STUB = """ - package android.app; + package android.app; public class Application { public void onCreate() {} public void onResume() {} }""" + @Language("JAVA") + const val MPEVENT_STUB = """package com.mparticle; + public class MPEvent { + public static class Builder { + public Builder(String eventName) {} + public Builder customAttributes(java.util.Map customAttributes) { return this; } + public MPEvent build() { return new MPEvent(); } + } + }""" + val mParticleStubClass = java(MPARTICLE_STUB) val mApplicationStubClass = java(APPLICATION_STUB) + val mpEventStubClass = java(MPEVENT_STUB) } diff --git a/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/DataplanDetectorTest.kt b/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/DataplanDetectorTest.kt index dabf4c961..c474e6e03 100644 --- a/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/DataplanDetectorTest.kt +++ b/tooling/custom-lint-rules/src/test/java/com/mparticle/lints/DataplanDetectorTest.kt @@ -2,9 +2,14 @@ package com.mparticle.lints import com.android.tools.lint.checks.infrastructure.LintDetectorTest import com.android.tools.lint.checks.infrastructure.TestMode +import com.mparticle.lints.Constants.mApplicationStubClass import com.mparticle.lints.Constants.mParticleStubClass +import com.mparticle.lints.Constants.mpEventStubClass import com.mparticle.lints.detectors.DataplanDetector +import com.mparticle.tooling.Config +import com.mparticle.tooling.Utils import org.intellij.lang.annotations.Language +import org.junit.Assert.assertTrue import org.junit.Test import java.io.File @@ -35,6 +40,97 @@ class DataplanDetectorTest : LintDetectorTest() { .expectErrorCount(0) } + // Regression test: a local variable feeding an MPEvent.Builder argument used to be resolved + // by reflectively replaying every call made on it in the enclosing method, with no + // restriction on what classes/methods those calls could touch. This reproduces that shape + // (an unrelated File operation feeding the event name) and asserts lint never performs it. + @Test + fun testEnclosingMethodSideEffectsAreNeverExecuted() { + val sdkHome = + System.getenv("ANDROID_HOME") + ?: "${System.getProperty("user.home")}/Library/Android/sdk" + + val marker = File.createTempFile("mparticle-lint-regression", ".txt") + marker.writeText("must survive lint analysis") + val markerPath = marker.absolutePath.replace("\\", "\\\\") + + try { + withConfigFile(Config()) { + @Language("JAVA") + val source = """ + package com.mparticle.lints; + import android.app.Application; + import com.mparticle.MPEvent; + import java.io.File; + public class HasUnrelatedFileCall extends Application { + @Override + public void onCreate() { + super.onCreate(); + File f = new File("$markerPath"); + f.delete(); + String name = f.getName(); + new MPEvent.Builder(name); + } + } + """ + lint() + .sdkHome(File(sdkHome)) + .files(java(source), mParticleStubClass, mApplicationStubClass, mpEventStubClass) + .skipTestModes(TestMode.PARENTHESIZED) + .run() + } + assertTrue( + "Data plan lint must not perform filesystem operations found in analyzed source", + marker.exists(), + ) + } finally { + marker.delete() + } + } + + // A legitimate builder chain (the only thing this detector needs to resolve) must keep + // resolving successfully - the allowlist should reject non-DTO reflection, not the feature. + @Test + fun testAllowlistedBuilderChainStillResolves() { + val sdkHome = + System.getenv("ANDROID_HOME") + ?: "${System.getProperty("user.home")}/Library/Android/sdk" + + withConfigFile(Config()) { + @Language("JAVA") + val source = """ + package com.mparticle.lints; + import android.app.Application; + import com.mparticle.MPEvent; + public class HasAllowlistedCall extends Application { + @Override + public void onCreate() { + super.onCreate(); + new MPEvent.Builder("test").build(); + } + } + """ + val result = + lint() + .sdkHome(File(sdkHome)) + .files(java(source), mParticleStubClass, mApplicationStubClass, mpEventStubClass) + .skipTestModes(TestMode.PARENTHESIZED) + .run() + // With no local data plan configured, a successfully-resolved event is reported as + // NO_DATA_PLAN; a failure to resolve at all would instead produce no report. + result.expectContains(DataplanDetector.NO_DATA_PLAN.id) + } + } + + private fun withConfigFile(config: Config, block: () -> Unit) { + Utils.setConfigFile(config) + try { + block() + } finally { + Utils.removeConfigFile() + } + } + override fun requireCompileSdk() = false override fun getDetector() = DataplanDetector()