17: Improve IntentFilterPlugin to work on context

This commit is contained in:
Bartłomiej Pluta
2019-06-09 21:49:25 +02:00
parent adce19f1e7
commit 3af5a43797
8 changed files with 91 additions and 42 deletions

View File

@@ -4,7 +4,7 @@ import com.bartek.esa.context.model.Context;
import com.bartek.esa.context.model.Source; import com.bartek.esa.context.model.Source;
import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.core.xml.XmlHelper;
import com.bartek.esa.error.EsaException; import com.bartek.esa.error.EsaException;
import com.bartek.esa.file.matcher.GlobMatcher; import com.bartek.esa.file.matcher.PackageNameMatcher;
import com.github.javaparser.ParseProblemException; import com.github.javaparser.ParseProblemException;
import com.github.javaparser.Problem; import com.github.javaparser.Problem;
import com.github.javaparser.StaticJavaParser; import com.github.javaparser.StaticJavaParser;
@@ -18,19 +18,18 @@ import javax.xml.xpath.XPathConstants;
import java.io.File; import java.io.File;
import java.util.Optional; import java.util.Optional;
import java.util.Set; import java.util.Set;
import java.util.function.Predicate;
import static java.lang.String.format; import static java.lang.String.format;
import static java.util.stream.Collectors.toSet; import static java.util.stream.Collectors.toSet;
public class ContextConstructor { public class ContextConstructor {
private final XmlHelper xmlHelper; private final XmlHelper xmlHelper;
private final GlobMatcher globMatcher; private final PackageNameMatcher packageNameMatcher;
@Inject @Inject
public ContextConstructor(XmlHelper xmlHelper, GlobMatcher globMatcher) { public ContextConstructor(XmlHelper xmlHelper, PackageNameMatcher packageNameMatcher) {
this.xmlHelper = xmlHelper; this.xmlHelper = xmlHelper;
this.globMatcher = globMatcher; this.packageNameMatcher = packageNameMatcher;
} }
public Context construct(File androidManifestFile, Set<File> javaFiles, Set<File> layoutFiles) { public Context construct(File androidManifestFile, Set<File> javaFiles, Set<File> layoutFiles) {
@@ -76,19 +75,12 @@ public class ContextConstructor {
private Set<Source<CompilationUnit>> parseJavaFiles(Set<File> javaFiles, String packageName) { private Set<Source<CompilationUnit>> parseJavaFiles(Set<File> javaFiles, String packageName) {
return javaFiles.stream() return javaFiles.stream()
.filter(isApplicationPackageFile(packageName)) .filter(file -> packageNameMatcher.doesFileMatchPackageName(file, packageName))
.map(file -> new Source<>(file, parseJava(file))) .map(file -> new Source<>(file, parseJava(file)))
.filter(s -> s.getModel() != null) .filter(s -> s.getModel() != null)
.collect(toSet()); .collect(toSet());
} }
private Predicate<File> isApplicationPackageFile(String packageName) {
return file -> {
String path = packageName.replaceAll("\\.", "/");
return globMatcher.fileMatchesGlobPattern(file, format("**/%s/**", path));
};
}
private CompilationUnit parseJava(File javaFile) { private CompilationUnit parseJava(File javaFile) {
try { try {
return StaticJavaParser.parse(javaFile); return StaticJavaParser.parse(javaFile);

View File

@@ -2,14 +2,14 @@ package com.bartek.esa.context.di;
import com.bartek.esa.context.constructor.ContextConstructor; import com.bartek.esa.context.constructor.ContextConstructor;
import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.core.xml.XmlHelper;
import com.bartek.esa.file.matcher.GlobMatcher; import com.bartek.esa.file.matcher.PackageNameMatcher;
import dagger.Module; import dagger.Module;
@Module @Module
public class ContextModule { public class ContextModule {
public ContextConstructor contextConstructor(XmlHelper xmlHelper, GlobMatcher globMatcher) { public ContextConstructor contextConstructor(XmlHelper xmlHelper, PackageNameMatcher packageNameMatcher) {
return new ContextConstructor(xmlHelper, globMatcher); return new ContextConstructor(xmlHelper, packageNameMatcher);
} }
} }

View File

@@ -7,6 +7,7 @@ import com.bartek.esa.core.helper.StringConcatenationChecker;
import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.java.JavaSyntaxRegexProvider;
import com.bartek.esa.core.plugin.*; import com.bartek.esa.core.plugin.*;
import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.core.xml.XmlHelper;
import com.bartek.esa.file.matcher.PackageNameMatcher;
import dagger.Module; import dagger.Module;
import dagger.Provides; import dagger.Provides;
import dagger.multibindings.ElementsIntoSet; import dagger.multibindings.ElementsIntoSet;
@@ -98,8 +99,8 @@ public class PluginModule {
@Provides @Provides
@IntoSet @IntoSet
public Plugin exportedComponentsPlugin(XmlHelper xmlHelper) { public Plugin exportedComponentsPlugin(XmlHelper xmlHelper, PackageNameMatcher packageNameMatcher) {
return new ExportedComponentsPlugin(xmlHelper); return new ExportedComponentsPlugin(xmlHelper, packageNameMatcher);
} }
@Provides @Provides
@@ -116,8 +117,8 @@ public class PluginModule {
@Provides @Provides
@IntoSet @IntoSet
public Plugin intentFilterPlugin(XmlHelper xmlHelper) { public Plugin intentFilterPlugin(XmlHelper xmlHelper, PackageNameMatcher packageNameMatcher) {
return new IntentFilterPlugin(xmlHelper); return new IntentFilterPlugin(xmlHelper, packageNameMatcher);
} }
@Provides @Provides

View File

@@ -1,11 +1,10 @@
package com.bartek.esa.core.plugin; package com.bartek.esa.core.plugin;
import com.bartek.esa.context.model.Context; import com.bartek.esa.context.model.Context;
import com.bartek.esa.context.model.Source;
import com.bartek.esa.core.archetype.BasePlugin; import com.bartek.esa.core.archetype.BasePlugin;
import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.model.enumeration.Severity;
import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.core.xml.XmlHelper;
import com.github.javaparser.ast.CompilationUnit; import com.bartek.esa.file.matcher.PackageNameMatcher;
import com.github.javaparser.ast.expr.MethodCallExpr; import com.github.javaparser.ast.expr.MethodCallExpr;
import org.w3c.dom.Node; import org.w3c.dom.Node;
import org.w3c.dom.NodeList; import org.w3c.dom.NodeList;
@@ -14,16 +13,17 @@ import javax.inject.Inject;
import javax.xml.xpath.XPathConstants; import javax.xml.xpath.XPathConstants;
import java.util.Map; import java.util.Map;
import java.util.Optional; import java.util.Optional;
import java.util.function.Predicate;
import static java.lang.String.format; import static java.lang.String.format;
public class ExportedComponentsPlugin extends BasePlugin { public class ExportedComponentsPlugin extends BasePlugin {
private final XmlHelper xmlHelper; private final XmlHelper xmlHelper;
private final PackageNameMatcher packageNameMatcher;
@Inject @Inject
public ExportedComponentsPlugin(XmlHelper xmlHelper) { public ExportedComponentsPlugin(XmlHelper xmlHelper, PackageNameMatcher packageNameMatcher) {
this.xmlHelper = xmlHelper; this.xmlHelper = xmlHelper;
this.packageNameMatcher = packageNameMatcher;
} }
@Override @Override
@@ -52,7 +52,7 @@ public class ExportedComponentsPlugin extends BasePlugin {
private boolean isIntentDataBeingUsedInsideComponent(Context context, String componentCanonicalName) { private boolean isIntentDataBeingUsedInsideComponent(Context context, String componentCanonicalName) {
return context.getJavaSources().stream() return context.getJavaSources().stream()
.filter(doesMatchCanonicalName(componentCanonicalName)) .filter(java -> packageNameMatcher.doesFileMatchPackageName(java.getFile(), componentCanonicalName))
.flatMap(java -> java.getModel().findAll(MethodCallExpr.class).stream()) .flatMap(java -> java.getModel().findAll(MethodCallExpr.class).stream())
.filter(expr -> expr.getName().getIdentifier().equals("getIntent")) .filter(expr -> expr.getName().getIdentifier().equals("getIntent"))
.anyMatch(expr -> expr.getArguments().isEmpty()); .anyMatch(expr -> expr.getArguments().isEmpty());
@@ -88,8 +88,4 @@ public class ExportedComponentsPlugin extends BasePlugin {
.map(v -> v.equals("true")) .map(v -> v.equals("true"))
.orElse(false); .orElse(false);
} }
private Predicate<Source<CompilationUnit>> doesMatchCanonicalName(String canonicalName) {
return source -> source.getFile().getAbsolutePath().replaceAll("/", ".").contains(canonicalName);
}
} }

View File

@@ -1,31 +1,51 @@
package com.bartek.esa.core.plugin; package com.bartek.esa.core.plugin;
import com.bartek.esa.context.model.Source; import com.bartek.esa.context.model.Context;
import com.bartek.esa.core.archetype.AndroidManifestPlugin; import com.bartek.esa.core.archetype.BasePlugin;
import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.model.enumeration.Severity;
import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.core.xml.XmlHelper;
import org.w3c.dom.Document; import com.bartek.esa.file.matcher.PackageNameMatcher;
import com.github.javaparser.ast.expr.MethodCallExpr;
import org.w3c.dom.Node; import org.w3c.dom.Node;
import org.w3c.dom.NodeList; import org.w3c.dom.NodeList;
import javax.inject.Inject; import javax.inject.Inject;
import java.util.Map; import java.util.Map;
import java.util.Optional;
public class IntentFilterPlugin extends AndroidManifestPlugin { public class IntentFilterPlugin extends BasePlugin {
private final XmlHelper xmlHelper; private final XmlHelper xmlHelper;
private final PackageNameMatcher packageNameMatcher;
@Inject @Inject
public IntentFilterPlugin(XmlHelper xmlHelper) { public IntentFilterPlugin(XmlHelper xmlHelper, PackageNameMatcher packageNameMatcher) {
this.xmlHelper = xmlHelper; this.xmlHelper = xmlHelper;
this.packageNameMatcher = packageNameMatcher;
} }
@Override @Override
protected void run(Source<Document> manifest) { protected void run(Context context) {
NodeList filters = manifest.getModel().getElementsByTagName("intent-filter"); NodeList filters = context.getManifest().getModel().getElementsByTagName("intent-filter");
xmlHelper.stream(filters) xmlHelper.stream(filters)
.filter(this::isNotMainActivity) .filter(this::isNotMainActivity)
.filter(this::isNotExported)
.map(Node::getParentNode) .map(Node::getParentNode)
.forEach(n -> addIssue(Severity.INFO, getModel(n), manifest.getFile(), null, null)); .forEach(node -> {
String componentName = node.getAttributes().getNamedItem("android:name").getNodeValue();
String canonicalName = context.getPackageName() + componentName;
if (isIntentDataBeingUsedInsideComponent(context, canonicalName)) {
addIssue(Severity.WARNING, ".DATA_USAGE", getModel(node), context.getManifest().getFile(), null, null);
} else {
addIssue(Severity.WARNING, getModel(node), context.getManifest().getFile(), null, null);
}
});
}
private boolean isNotExported(Node component) {
return !Optional.ofNullable(component.getAttributes().getNamedItem("android:exported"))
.map(Node::getNodeValue)
.map(Boolean::parseBoolean)
.orElse(false);
} }
private Map<String, String> getModel(Node node) { private Map<String, String> getModel(Node node) {
@@ -37,16 +57,24 @@ public class IntentFilterPlugin extends AndroidManifestPlugin {
private boolean isNotMainActivity(Node filter) { private boolean isNotMainActivity(Node filter) {
long mainActivityIntentFilters = xmlHelper.stream(filter.getChildNodes()) long mainActivityIntentFilters = xmlHelper.stream(filter.getChildNodes())
.filter(n -> n.getNodeName().matches("action|category")) .filter(n -> n.getNodeName().matches("action|category|data"))
.map(n -> n.getAttributes().getNamedItem("android:name")) .map(n -> n.getAttributes().getNamedItem("android:name"))
.map(Node::getNodeValue) .map(Node::getNodeValue)
.filter(v -> v.equals("android.intent.action.MAIN") || v.equals("android.intent.category.LAUNCHER")) .filter(v -> v.equals("android.intent.action.MAIN") || v.equals("android.intent.category.LAUNCHER"))
.count(); .count();
long currentIntentFilters = xmlHelper.stream(filter.getChildNodes()) long currentIntentFilters = xmlHelper.stream(filter.getChildNodes())
.filter(n -> n.getNodeName().matches("action|category")) .filter(n -> n.getNodeName().matches("action|category|data"))
.count(); .count();
return mainActivityIntentFilters != currentIntentFilters; return mainActivityIntentFilters != currentIntentFilters;
} }
private boolean isIntentDataBeingUsedInsideComponent(Context context, String componentCanonicalName) {
return context.getJavaSources().stream()
.filter(java -> packageNameMatcher.doesFileMatchPackageName(java.getFile(), componentCanonicalName))
.flatMap(java -> java.getModel().findAll(MethodCallExpr.class).stream())
.filter(expr -> expr.getName().getIdentifier().equals("getIntent"))
.anyMatch(expr -> expr.getArguments().isEmpty());
}
} }

View File

@@ -2,6 +2,7 @@ package com.bartek.esa.file.di;
import com.bartek.esa.file.cleaner.FileCleaner; import com.bartek.esa.file.cleaner.FileCleaner;
import com.bartek.esa.file.matcher.GlobMatcher; import com.bartek.esa.file.matcher.GlobMatcher;
import com.bartek.esa.file.matcher.PackageNameMatcher;
import com.bartek.esa.file.provider.FileContentProvider; import com.bartek.esa.file.provider.FileContentProvider;
import com.bartek.esa.file.provider.FileProvider; import com.bartek.esa.file.provider.FileProvider;
import com.bartek.esa.file.zip.ZipTool; import com.bartek.esa.file.zip.ZipTool;
@@ -35,4 +36,9 @@ public class FileModule {
public FileCleaner fileCleaner() { public FileCleaner fileCleaner() {
return new FileCleaner(); return new FileCleaner();
} }
@Provides
public PackageNameMatcher packageNameMatcher() {
return new PackageNameMatcher();
}
} }

View File

@@ -0,0 +1,16 @@
package com.bartek.esa.file.matcher;
import javax.inject.Inject;
import java.io.File;
public class PackageNameMatcher {
@Inject
public PackageNameMatcher() {
}
public boolean doesFileMatchPackageName(File file, String packageName) {
return file.getAbsolutePath().replaceAll(File.separator, ".").contains(packageName);
}
}

View File

@@ -110,12 +110,22 @@ com.bartek.esa.core.plugin.TextInputValidationPlugin=Input type is no selected.\
Consider associating a input type with this view.\n\ Consider associating a input type with this view.\n\
For example: <EditText android:inputType="number" ... For example: <EditText android:inputType="number" ...
com.bartek.esa.core.plugin.IntentFilterPlugin=Implemented intent filter.\n\ com.bartek.esa.core.plugin.IntentFilterPlugin=Implemented intent filter inside private component.\n\
The ${componentType} with name '${componentName}' does have a intent filter declared. \n\ The non-exported ${componentType} with name '${componentName}' does have a intent filter declared. \n\
It means, that the component is implicitly exposed to public.\n\ It means, that the component is implicitly exposed to public.\n\
Consider removing intent filter.\n\ Consider removing intent filter or set it explicitely to be exported using following attribute:\n\
android:exported="true". \n\
Also be aware, that intent filter is not a security tool. It can be easily omitted. Also be aware, that intent filter is not a security tool. It can be easily omitted.
com.bartek.esa.core.plugin.IntentFilterPlugin.DATA_USAGE=Implemented intent filter inside private component making use of incoming data.\n\
The non-exported ${componentType} with name '${componentName}' does have a intent filter declared \n\
and also does make use of incoming intent data. \n\
It means, that the component is implicitly exposed to public and can be spoofed with fake data.\n\
Consider removing intent filter or set it explicitely to be exported using following attribute:\n\
android:exported="true". \n\
Be aware, that intent filter is not a security tool. It can be easily omitted. \n\
Also make sure, that data is correctly validated before taking advantage of it.
com.bartek.esa.core.plugin.SqlInjectionPlugin='rawQuery' method detected. Potential SQL injection attack.\n\ com.bartek.esa.core.plugin.SqlInjectionPlugin='rawQuery' method detected. Potential SQL injection attack.\n\
'rawQuery' method should be avoided because of possibility to inject SQL code. 'rawQuery' method should be avoided because of possibility to inject SQL code.