From bccd9c28e1140405e42c95b7e146a5da99b69f6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 13:10:11 +0200 Subject: [PATCH 01/37] 10: Update ColorFormatter colors --- .../com/bartek/esa/formatter/formatter/ColorFormatter.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java b/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java index d7ac99d..b5d2988 100644 --- a/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java +++ b/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java @@ -73,8 +73,9 @@ public class ColorFormatter implements Formatter { .map(file -> ansi .fg(BLUE) .a("File: ") - .reset() + .fg(CYAN) .a(file.getAbsolutePath()) + .reset() .a("\n")) .orElse(ansi); } @@ -86,10 +87,10 @@ public class ColorFormatter implements Formatter { .fg(CYAN) .a("Line"); Optional.ofNullable(issue.getLineNumber()).ifPresentOrElse( - number -> ansi.a(" ").a(number).a(": "), + number -> ansi.a(" ").fg(MAGENTA).a(number).fg(CYAN).a(": "), () -> ansi.a(": ") ); - ansi.reset().a(line).a("\n"); + ansi.fg(BLUE).a(line).reset().a("\n"); }); return ansi; } From cddbbe0b57c1d5c39c0db5da8eef9810bf5c5cde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 23:34:55 +0200 Subject: [PATCH 02/37] 10: Add summary to formatters --- .../formatter/formatter/ColorFormatter.java | 32 ++++++++++++++++++- .../formatter/formatter/SimpleFormatter.java | 22 +++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java b/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java index b5d2988..8b56f46 100644 --- a/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java +++ b/src/main/java/com/bartek/esa/formatter/formatter/ColorFormatter.java @@ -1,6 +1,7 @@ package com.bartek.esa.formatter.formatter; import com.bartek.esa.core.desc.provider.DescriptionProvider; +import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.model.object.Issue; import com.bartek.esa.formatter.archetype.Formatter; import org.fusesource.jansi.Ansi; @@ -16,6 +17,10 @@ import static org.fusesource.jansi.Ansi.Color.*; import static org.fusesource.jansi.Ansi.ansi; public class ColorFormatter implements Formatter { + private static final Ansi.Color INFO_COLOR = GREEN; + private static final Ansi.Color WARNING_COLOR = YELLOW; + private static final Ansi.Color ERROR_COLOR = MAGENTA; + private static final Ansi.Color VULNERABILITY_COLOR = RED; private final DescriptionProvider descriptionProvider; @@ -39,6 +44,7 @@ public class ColorFormatter implements Formatter { .collect(Collectors.joining()); System.out.println(format.substring(0, format.length() - 2)); + System.out.println(printSummary(issues)); AnsiConsole.systemUninstall(); } @@ -95,8 +101,32 @@ public class ColorFormatter implements Formatter { return ansi; } + private String printSummary(Set issues) { + Ansi ansi = ansi(); + ansi.a("\n--- Total:\n"); + Arrays.stream(Severity.values()) + .forEach(severity -> ansi.fg(getColorForSeverity(severity)) + .a(severity.name()) + .a(": ") + .reset() + .a(countIssuesBySeverity(issues, severity)) + .a("\n")); + return ansi.toString(); + } + + private long countIssuesBySeverity(Set issues, Severity severity) { + return issues.stream() + .map(Issue::getSeverity) + .filter(severity::equals) + .count(); + } + private Ansi.Color getColorForSeverity(Issue issue) { - switch (issue.getSeverity()) { + return getColorForSeverity(issue.getSeverity()); + } + + private Ansi.Color getColorForSeverity(Severity severity) { + switch (severity) { case INFO: return GREEN; case WARNING: diff --git a/src/main/java/com/bartek/esa/formatter/formatter/SimpleFormatter.java b/src/main/java/com/bartek/esa/formatter/formatter/SimpleFormatter.java index 8294849..7644ce0 100644 --- a/src/main/java/com/bartek/esa/formatter/formatter/SimpleFormatter.java +++ b/src/main/java/com/bartek/esa/formatter/formatter/SimpleFormatter.java @@ -1,10 +1,12 @@ package com.bartek.esa.formatter.formatter; import com.bartek.esa.core.desc.provider.DescriptionProvider; +import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.model.object.Issue; import com.bartek.esa.formatter.archetype.Formatter; import javax.inject.Inject; +import java.util.Arrays; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -30,6 +32,7 @@ public class SimpleFormatter implements Formatter { .collect(Collectors.joining()); System.out.println(format.substring(0, format.length() - 2)); + System.out.println(printSummary(issues)); } private String format(Issue issue) { @@ -71,4 +74,23 @@ public class SimpleFormatter implements Formatter { format.append(line).append("\n"); }); } + + private String printSummary(Set issues) { + StringBuilder format = new StringBuilder(); + format.append("\n--- Total:\n"); + Arrays.stream(Severity.values()) + .forEach(severity -> format + .append(severity.name()) + .append(": ") + .append(countIssuesBySeverity(issues, severity)) + .append("\n")); + return format.toString(); + } + + private long countIssuesBySeverity(Set issues, Severity severity) { + return issues.stream() + .map(Issue::getSeverity) + .filter(severity::equals) + .count(); + } } From 3a2859b5136a6a3b6a00d25d6edb12e433bcd3e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Sat, 6 Apr 2019 11:50:34 +0200 Subject: [PATCH 03/37] 10: Simplify passing plugins codes through CLI --- .../java/com/bartek/esa/analyser/core/Analyser.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/bartek/esa/analyser/core/Analyser.java b/src/main/java/com/bartek/esa/analyser/core/Analyser.java index 1e9bd0f..f571697 100644 --- a/src/main/java/com/bartek/esa/analyser/core/Analyser.java +++ b/src/main/java/com/bartek/esa/analyser/core/Analyser.java @@ -9,6 +9,7 @@ import com.bartek.esa.file.provider.FileProvider; import java.io.File; import java.util.Collections; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -76,7 +77,7 @@ public abstract class Analyser { outputPlugins = plugins.stream() .filter(plugin -> pluginCodes .stream() - .anyMatch(pluginCode -> plugin.getClass().getCanonicalName().equals(pluginCode)) + .anyMatch(doesNameMatchPlugin(plugin)) ) .collect(Collectors.toSet()); } @@ -85,11 +86,16 @@ public abstract class Analyser { outputPlugins = outputPlugins.stream() .filter(plugin -> excludeCodes .stream() - .noneMatch(pluginCode -> plugin.getClass().getCanonicalName().equals(pluginCode)) + .noneMatch(doesNameMatchPlugin(plugin)) ) .collect(Collectors.toSet()); } return outputPlugins; } + + private Predicate doesNameMatchPlugin(Plugin plugin) { + return pluginCode -> plugin.getClass().getCanonicalName().equals(pluginCode) + || plugin.getClass().getSimpleName().equals(pluginCode); + } } From 7fec068dc759504add01838c583186d5aff7ad69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Wed, 10 Apr 2019 19:48:21 +0200 Subject: [PATCH 04/37] 10: Add description model to issues --- build.gradle | 3 ++- dependency-versions.gradle | 2 ++ .../com/bartek/esa/core/archetype/BasePlugin.java | 12 ++++++++++++ .../com/bartek/esa/core/archetype/JavaPlugin.java | 2 ++ .../esa/core/desc/provider/DescriptionProvider.java | 3 ++- .../java/com/bartek/esa/core/model/object/Issue.java | 2 ++ 6 files changed, 22 insertions(+), 2 deletions(-) diff --git a/build.gradle b/build.gradle index b72d0a5..affc8cf 100644 --- a/build.gradle +++ b/build.gradle @@ -23,7 +23,8 @@ dependencies { compile "com.github.javaparser:javaparser-core:${javaParserVersion}" compile "commons-io:commons-io:${commonsIoVersion}" compile "org.fusesource.jansi:jansi:${jansiVersion}" - + compile "org.apache.commons:commons-lang3:${commonsLangVersion}" + compile "org.apache.commons:commons-text:${commonsTextVersion}" } jar { diff --git a/dependency-versions.gradle b/dependency-versions.gradle index 0114d4f..2156b4b 100644 --- a/dependency-versions.gradle +++ b/dependency-versions.gradle @@ -6,4 +6,6 @@ ext { javaParserVersion = '3.13.4' commonsIoVersion = '2.6' jansiVersion = '1.17.1' + commonsLangVersion = '3.8.1' + commonsTextVersion = '1.6' } \ No newline at end of file diff --git a/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java b/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java index fbcd814..ed3465d 100644 --- a/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java @@ -5,7 +5,9 @@ import com.bartek.esa.core.model.object.Issue; import org.w3c.dom.Document; import java.io.File; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; public abstract class BasePlugin implements Plugin { @@ -32,11 +34,21 @@ public abstract class BasePlugin implements Plugin { addIssue(severity, "", lineNumber, line); } + protected void addIssue(Severity severity, Map descriptionModel, Integer lineNumber, String line) { + addIssue(severity, "", descriptionModel, lineNumber, line); + } + + protected void addIssue(Severity severity, String descriptionCode, Integer lineNumber, String line) { + addIssue(severity, descriptionCode, new HashMap<>(), lineNumber, line); + } + + protected void addIssue(Severity severity, String descriptionCode, Map descriptionModel, Integer lineNumber, String line) { Issue issue = Issue.builder() .severity(severity) .issuer(this.getClass()) .descriptionCode(descriptionCode) + .descriptionModel(descriptionModel) .file(file) .lineNumber(lineNumber) .line(line) diff --git a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java index b438f82..02a4346 100644 --- a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java @@ -13,6 +13,7 @@ import org.w3c.dom.Node; import javax.xml.xpath.XPathConstants; import java.io.File; +import java.util.HashMap; public abstract class JavaPlugin extends BasePlugin { private final GlobMatcher globMatcher; @@ -47,6 +48,7 @@ public abstract class JavaPlugin extends BasePlugin { Issue issue = Issue.builder() .issuer(JavaPlugin.class) .descriptionCode(".NO_PACKAGE") + .descriptionModel(new HashMap<>()) .severity(Severity.ERROR) .build(); diff --git a/src/main/java/com/bartek/esa/core/desc/provider/DescriptionProvider.java b/src/main/java/com/bartek/esa/core/desc/provider/DescriptionProvider.java index b5ac646..14e247b 100644 --- a/src/main/java/com/bartek/esa/core/desc/provider/DescriptionProvider.java +++ b/src/main/java/com/bartek/esa/core/desc/provider/DescriptionProvider.java @@ -3,6 +3,7 @@ package com.bartek.esa.core.desc.provider; import com.bartek.esa.core.model.object.Issue; import com.bartek.esa.error.EsaException; import io.vavr.control.Try; +import org.apache.commons.text.StringSubstitutor; import javax.inject.Inject; import java.util.Optional; @@ -20,7 +21,7 @@ public class DescriptionProvider { public String getDescriptionForIssue(Issue issue) { String code = issue.getIssuer().getCanonicalName() + issue.getDescriptionCode(); - String description = descriptions.getProperty(code); + String description = StringSubstitutor.replace(descriptions.getProperty(code), issue.getDescriptionModel()); return description != null && !description.isEmpty() ? description : "No description provided."; } } diff --git a/src/main/java/com/bartek/esa/core/model/object/Issue.java b/src/main/java/com/bartek/esa/core/model/object/Issue.java index 11ddcd6..1fb713b 100644 --- a/src/main/java/com/bartek/esa/core/model/object/Issue.java +++ b/src/main/java/com/bartek/esa/core/model/object/Issue.java @@ -5,6 +5,7 @@ import lombok.Builder; import lombok.Data; import java.io.File; +import java.util.Map; @Data @Builder @@ -12,6 +13,7 @@ public class Issue implements Comparable { private final Class issuer; private final Severity severity; private final String descriptionCode; + private final Map descriptionModel; private final File file; private final Integer lineNumber; private final String line; From bd6b5df8c1343b14142c938914b5d78161862e9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 11:17:49 +0200 Subject: [PATCH 05/37] 10: Create LoggingPlugin --- .../bartek/esa/core/archetype/JavaPlugin.java | 5 +++ .../com/bartek/esa/core/di/CoreModule.java | 6 ---- .../com/bartek/esa/core/di/PluginModule.java | 10 ++++++ .../core/java/JavaSyntaxRegexProvider.java | 17 ---------- .../bartek/esa/core/plugin/LoggingPlugin.java | 32 +++++++++++++++++++ src/main/resources/description.properties | 5 ++- 6 files changed, 51 insertions(+), 24 deletions(-) delete mode 100644 src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java create mode 100644 src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java diff --git a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java index 02a4346..030587f 100644 --- a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java @@ -7,6 +7,7 @@ import com.bartek.esa.error.EsaException; import com.bartek.esa.file.matcher.GlobMatcher; import com.github.javaparser.StaticJavaParser; import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.Expression; import io.vavr.control.Try; import org.w3c.dom.Document; import org.w3c.dom.Node; @@ -61,5 +62,9 @@ public abstract class JavaPlugin extends BasePlugin { return globMatcher.fileMatchesGlobPattern(file, String.format("**/%s/**", path)); } + protected Integer getLineNumberFromExpression(Expression expression) { + return expression.getRange().map(r -> r.begin.line).orElse(null); + } + public abstract void run(CompilationUnit compilationUnit); } diff --git a/src/main/java/com/bartek/esa/core/di/CoreModule.java b/src/main/java/com/bartek/esa/core/di/CoreModule.java index 59d666d..bafe8ec 100644 --- a/src/main/java/com/bartek/esa/core/di/CoreModule.java +++ b/src/main/java/com/bartek/esa/core/di/CoreModule.java @@ -2,7 +2,6 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.desc.provider.DescriptionProvider; import com.bartek.esa.core.executor.PluginExecutor; -import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.xml.XmlHelper; import dagger.Module; import dagger.Provides; @@ -15,11 +14,6 @@ public class CoreModule { return new PluginExecutor(xmlHelper); } - @Provides - public JavaSyntaxRegexProvider javaSyntaxRegexProvider() { - return new JavaSyntaxRegexProvider(); - } - @Provides public DescriptionProvider descriptionProvider() { return new DescriptionProvider(); diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index c1474b2..662c8fc 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -1,9 +1,13 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.archetype.Plugin; +import com.bartek.esa.core.plugin.LoggingPlugin; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; import dagger.Module; import dagger.Provides; import dagger.multibindings.ElementsIntoSet; +import dagger.multibindings.IntoSet; import java.util.HashSet; import java.util.Set; @@ -16,4 +20,10 @@ public class PluginModule { public Set plugins() { return new HashSet<>(); } + + @Provides + @IntoSet + public Plugin loggingPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new LoggingPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java b/src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java deleted file mode 100644 index d15c375..0000000 --- a/src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java +++ /dev/null @@ -1,17 +0,0 @@ -package com.bartek.esa.core.java; - -import javax.inject.Inject; - -import static java.lang.String.format; - -public class JavaSyntaxRegexProvider { - - @Inject - public JavaSyntaxRegexProvider() { - - } - - public String methodInvocation(String methodName) { - return format("^%s\\s*\\($", methodName); - } -} diff --git a/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java b/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java new file mode 100644 index 0000000..89be22d --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java @@ -0,0 +1,32 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.visitor.VoidVisitorAdapter; + +import javax.inject.Inject; + +public class LoggingPlugin extends JavaPlugin { + + @Inject + public LoggingPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.accept(new VoidVisitorAdapter() { + @Override + public void visit(MethodCallExpr methodCall, Void arg) { + if (methodCall.getName().getIdentifier().matches("v|d|i|w|e|wtf")) { + addIssue(Severity.INFO, getLineNumberFromExpression(methodCall), methodCall.toString()); + } + super.visit(methodCall, arg); + } + }, null); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 7efa949..705c689 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -1,4 +1,7 @@ com.bartek.esa.core.archetype.JavaPlugin.NO_PACKAGE=There is no package defined in AndroidManifest.xml file. \n\ Package should be defined as attribute of tag.\n\ For example: \n\ - Please fix it to use this tool. \ No newline at end of file + Please fix it to use this tool. + +com.bartek.esa.core.plugin.LoggingPlugin=Potential data leakage. \n\ + Logging method was detected. Please check if no sensitive data is logged there. \ No newline at end of file From e8fa888aef6c3e29b4d88ad83f3eff3257c6ff84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 13:35:27 +0200 Subject: [PATCH 06/37] 10: Create DebuggablePlugin --- .../com/bartek/esa/core/di/PluginModule.java | 7 +++++ .../esa/core/plugin/DebuggablePlugin.java | 30 +++++++++++++++++++ src/main/resources/description.properties | 16 ++++++++-- 3 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 662c8fc..0aa2dfb 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -1,6 +1,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.archetype.Plugin; +import com.bartek.esa.core.plugin.DebuggablePlugin; import com.bartek.esa.core.plugin.LoggingPlugin; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; @@ -26,4 +27,10 @@ public class PluginModule { public Plugin loggingPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new LoggingPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin debuggablePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new DebuggablePlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java b/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java new file mode 100644 index 0000000..e198352 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java @@ -0,0 +1,30 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; + +import javax.inject.Inject; +import javax.xml.xpath.XPathConstants; +import java.util.Optional; + +public class DebuggablePlugin extends AndroidManifestPlugin { + + @Inject + public DebuggablePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + Node applicationNode = (Node) xPath(xml, "/manifest/application", XPathConstants.NODE); + Optional.ofNullable(applicationNode.getAttributes().getNamedItem("android:debuggable")).ifPresentOrElse(n -> { + if(!n.getNodeValue().equals("false")) { + addIssue(Severity.WARNING, ".NO_FALSE", null, n.toString()); + } + }, () -> addIssue(Severity.ERROR, ".NO_ATTR",null, null)); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 705c689..287ea05 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -3,5 +3,17 @@ com.bartek.esa.core.archetype.JavaPlugin.NO_PACKAGE=There is no package defined For example: \n\ Please fix it to use this tool. -com.bartek.esa.core.plugin.LoggingPlugin=Potential data leakage. \n\ - Logging method was detected. Please check if no sensitive data is logged there. \ No newline at end of file +com.bartek.esa.core.plugin.LoggingPlugin=Potential data leakage in logs. \n\ + Logging method was detected. Please check if no sensitive data is logged there. + +com.bartek.esa.core.plugin.DebuggablePlugin.NO_ATTR=There is no android:debuggable option. Potential data leakage. \n\ + The android:debuggable option was not found in the AndroidManifest.xml file. \n\ + To avoid any potential data leakage in the future, please explicitly set this flag to false. \n\ + The attribute should be placed in tag.\n\ + For example: + +com.bartek.esa.core.plugin.DebuggablePlugin.NO_FALSE=The android:debuggable is set to 'true'. Potential data leakage. \n\ + The android:debuggable option in AndroidManifest.xml is set to 'true'. \n\ + This will cause application to be debuggable and can result in \ + security issues and data leakage on the production environment. \n\ + Consider setting it to 'false'. \ No newline at end of file From 10d3f7d02d441dd4a6468d77482e05815263fce3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 13:44:42 +0200 Subject: [PATCH 07/37] 10: Create AllowBackupPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 7 +++++ .../esa/core/plugin/AllowBackupPlugin.java | 30 +++++++++++++++++++ src/main/resources/description.properties | 11 +++++++ 3 files changed, 48 insertions(+) create mode 100644 src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 0aa2dfb..bdde37c 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -1,6 +1,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.archetype.Plugin; +import com.bartek.esa.core.plugin.AllowBackupPlugin; import com.bartek.esa.core.plugin.DebuggablePlugin; import com.bartek.esa.core.plugin.LoggingPlugin; import com.bartek.esa.core.xml.XmlHelper; @@ -33,4 +34,10 @@ public class PluginModule { public Plugin debuggablePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new DebuggablePlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin allowBackupPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new AllowBackupPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java b/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java new file mode 100644 index 0000000..87c21a6 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java @@ -0,0 +1,30 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; + +import javax.inject.Inject; +import javax.xml.xpath.XPathConstants; +import java.util.Optional; + +public class AllowBackupPlugin extends AndroidManifestPlugin { + + @Inject + public AllowBackupPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + Node applicationNode = (Node) xPath(xml, "/manifest/application", XPathConstants.NODE); + Optional.ofNullable(applicationNode.getAttributes().getNamedItem("android:allowBackup")).ifPresentOrElse(n -> { + if (!n.getNodeValue().equals("false")) { + addIssue(Severity.WARNING, ".NO_FALSE", null, n.toString()); + } + }, () -> addIssue(Severity.ERROR, ".NO_ATTR", null, null)); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 287ea05..10a1b91 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -16,4 +16,15 @@ com.bartek.esa.core.plugin.DebuggablePlugin.NO_FALSE=The android:debuggable is s The android:debuggable option in AndroidManifest.xml is set to 'true'. \n\ This will cause application to be debuggable and can result in \ security issues and data leakage on the production environment. \n\ + Consider setting it to 'false'. + +com.bartek.esa.core.plugin.AllowBackupPlugin.NO_ATTR=There is no android:allowBackup option. Potential data leakage. \n\ + The android:allowBackup option was not found in the AndroidManifest.xml file. \n\ + To avoid any potential data theft in the future, please explicitly set this flag to false. \n\ + The attribute should be placed in tag.\n\ + For example: + +com.bartek.esa.core.plugin.AllowBackupPlugin.NO_FALSE=The android:allowBackup is set to 'true'. Potential data leakage. \n\ + The android:allowBackup option in AndroidManifest.xml is set to 'true'. \n\ + This will allow accessing the backups via adb if device has USB debugging enabled.\n\ Consider setting it to 'false'. \ No newline at end of file From f4ed3e259d4e58e17290c9a23a1879bf463e10a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 14:16:51 +0200 Subject: [PATCH 08/37] 10: Add checking element in BasePlugin --- .../bartek/esa/core/archetype/BasePlugin.java | 43 ++++++++++++++++++- .../bartek/esa/core/archetype/JavaPlugin.java | 1 + .../bartek/esa/core/archetype/XmlPlugin.java | 1 + .../esa/core/plugin/AllowBackupPlugin.java | 2 +- .../esa/core/plugin/DebuggablePlugin.java | 2 +- src/main/resources/description.properties | 13 ++++++ 6 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java b/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java index ed3465d..4dc66ab 100644 --- a/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java @@ -2,8 +2,11 @@ package com.bartek.esa.core.archetype; import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.model.object.Issue; +import com.bartek.esa.core.xml.XmlHelper; import org.w3c.dom.Document; +import org.w3c.dom.Node; +import javax.xml.xpath.XPathConstants; import java.io.File; import java.util.HashMap; import java.util.HashSet; @@ -11,10 +14,15 @@ import java.util.Map; import java.util.Set; public abstract class BasePlugin implements Plugin { + private final XmlHelper xmlHelper; private Set issues = new HashSet<>(); private Document manifest; private File file; + public BasePlugin(XmlHelper xmlHelper) { + this.xmlHelper = xmlHelper; + } + @Override public void update(File file, Document manifest) { this.file = file; @@ -24,10 +32,43 @@ public abstract class BasePlugin implements Plugin { @Override public Set runForIssues() { - run(file); + if(canBeExecuted()) { + run(file); + } return issues; } + private boolean canBeExecuted() { + return hasDeclaredApiVersion(); + } + + private boolean hasDeclaredApiVersion() { + Node usesSdkNode = (Node) xmlHelper.xPath(manifest, "/manifest/uses-sdk", XPathConstants.NODE); + if(usesSdkNode == null) { + Issue issue = Issue.builder() + .issuer(BasePlugin.class) + .descriptionCode(".NO_USES_SDK") + .severity(Severity.ERROR) + .build(); + addIssue(issue); + + return false; + } + + if(usesSdkNode.getAttributes().getNamedItem("android:minSdkVersion") == null) { + Issue issue = Issue.builder() + .issuer(BasePlugin.class) + .descriptionCode(".USES_SDK.NO_MIN_SDK_VERSION") + .severity(Severity.ERROR) + .build(); + addIssue(issue); + + return false; + } + + return true; + } + protected abstract void run(File file); protected void addIssue(Severity severity, Integer lineNumber, String line) { diff --git a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java index 030587f..d9f9791 100644 --- a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java @@ -21,6 +21,7 @@ public abstract class JavaPlugin extends BasePlugin { private final XmlHelper xmlHelper; public JavaPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(xmlHelper); this.globMatcher = globMatcher; this.xmlHelper = xmlHelper; } diff --git a/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java b/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java index c1bbedd..082c984 100644 --- a/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java @@ -12,6 +12,7 @@ public abstract class XmlPlugin extends BasePlugin { private final XmlHelper xmlHelper; public XmlPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(xmlHelper); this.globMatcher = globMatcher; this.xmlHelper = xmlHelper; } diff --git a/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java b/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java index 87c21a6..d188bc0 100644 --- a/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java @@ -25,6 +25,6 @@ public class AllowBackupPlugin extends AndroidManifestPlugin { if (!n.getNodeValue().equals("false")) { addIssue(Severity.WARNING, ".NO_FALSE", null, n.toString()); } - }, () -> addIssue(Severity.ERROR, ".NO_ATTR", null, null)); + }, () -> addIssue(Severity.WARNING, ".NO_ATTR", null, null)); } } diff --git a/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java b/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java index e198352..481ad80 100644 --- a/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java @@ -25,6 +25,6 @@ public class DebuggablePlugin extends AndroidManifestPlugin { if(!n.getNodeValue().equals("false")) { addIssue(Severity.WARNING, ".NO_FALSE", null, n.toString()); } - }, () -> addIssue(Severity.ERROR, ".NO_ATTR",null, null)); + }, () -> addIssue(Severity.WARNING, ".NO_ATTR",null, null)); } } diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 10a1b91..d0b792b 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -1,3 +1,16 @@ +com.bartek.esa.core.archetype.BasePlugin.NO_USES_SDK=There is no defined in AndroidManifest.xml file. \n\ + In order to use this tool, should be defined in AndroidManifest.xml with android:minSdkVersion attribute at least.\n\ + This element should be placed below the root () level.\n\ + For example: \n\ + \n\ + \t\n\ + \t...\n\ + + +com.bartek.esa.core.archetype.BasePlugin.USES_SDK.NO_MIN_SDK_VERSION=There is no minSdkVersion defined in AndroidManifest.xml file. \n\ + In order to use this tool, minimal SDK version should be provided as the attribute of element.\n\ + For example: + com.bartek.esa.core.archetype.JavaPlugin.NO_PACKAGE=There is no package defined in AndroidManifest.xml file. \n\ Package should be defined as attribute of tag.\n\ For example: \n\ From 5f7dc6c2c9aa4d41334117505efd46da06ef1353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 14:47:04 +0200 Subject: [PATCH 09/37] 10: Create PermissionsRaceConditionPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 7 ++++ .../PermissionsRaceConditionPlugin.java | 35 +++++++++++++++++++ src/main/resources/description.properties | 27 ++++++++------ 3 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index bdde37c..00e1a80 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -4,6 +4,7 @@ import com.bartek.esa.core.archetype.Plugin; import com.bartek.esa.core.plugin.AllowBackupPlugin; import com.bartek.esa.core.plugin.DebuggablePlugin; import com.bartek.esa.core.plugin.LoggingPlugin; +import com.bartek.esa.core.plugin.PermissionsRaceConditionPlugin; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import dagger.Module; @@ -40,4 +41,10 @@ public class PluginModule { public Plugin allowBackupPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new AllowBackupPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin permissionRaceConditionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new PermissionsRaceConditionPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java b/src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java new file mode 100644 index 0000000..f1e3478 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java @@ -0,0 +1,35 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +import javax.inject.Inject; +import javax.xml.xpath.XPathConstants; + +import static java.lang.Integer.parseInt; + +public class PermissionsRaceConditionPlugin extends AndroidManifestPlugin { + + @Inject + public PermissionsRaceConditionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + boolean isAnyPermissionDefined = ((NodeList) xPath(xml, "/manifest/permission", XPathConstants.NODESET)).getLength() > 0; + if(isAnyPermissionDefined) { + Node usesSdkNode = (Node) xPath(xml, "/manifest/uses-sdk", XPathConstants.NODE); + Node minSdkVersionNode = usesSdkNode.getAttributes().getNamedItem("android:minSdkVersion"); + int minSdkVersion = parseInt(minSdkVersionNode.getNodeValue()); + if(minSdkVersion < 21) { + addIssue(Severity.VULNERABILITY, null, minSdkVersionNode.toString()); + } + } + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index d0b792b..e96b19e 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -26,18 +26,23 @@ com.bartek.esa.core.plugin.DebuggablePlugin.NO_ATTR=There is no android:debuggab For example: com.bartek.esa.core.plugin.DebuggablePlugin.NO_FALSE=The android:debuggable is set to 'true'. Potential data leakage. \n\ - The android:debuggable option in AndroidManifest.xml is set to 'true'. \n\ - This will cause application to be debuggable and can result in \ - security issues and data leakage on the production environment. \n\ - Consider setting it to 'false'. +The android:debuggable option in AndroidManifest.xml is set to 'true'. \n\ +This will cause application to be debuggable and can result in \ +security issues and data leakage on the production environment. \n\ +Consider setting it to 'false'. com.bartek.esa.core.plugin.AllowBackupPlugin.NO_ATTR=There is no android:allowBackup option. Potential data leakage. \n\ - The android:allowBackup option was not found in the AndroidManifest.xml file. \n\ - To avoid any potential data theft in the future, please explicitly set this flag to false. \n\ - The attribute should be placed in tag.\n\ - For example: +The android:allowBackup option was not found in the AndroidManifest.xml file. \n\ +To avoid any potential data theft in the future, please explicitly set this flag to false. \n\ +The attribute should be placed in tag.\n\ +For example: com.bartek.esa.core.plugin.AllowBackupPlugin.NO_FALSE=The android:allowBackup is set to 'true'. Potential data leakage. \n\ - The android:allowBackup option in AndroidManifest.xml is set to 'true'. \n\ - This will allow accessing the backups via adb if device has USB debugging enabled.\n\ - Consider setting it to 'false'. \ No newline at end of file +The android:allowBackup option in AndroidManifest.xml is set to 'true'. \n\ +This will allow accessing the backups via adb if device has USB debugging enabled.\n\ +Consider setting it to 'false'. + +com.bartek.esa.core.plugin.PermissionsRaceConditionPlugin=Potential permissions race condition vulnerability. \n\ + There are declared custom permissions in AndroidManifest.xml and the minimal API version is set to less than 21.\n\ + It means that declared permissions can be obtained by malicious application installed before and without need of having 1proper signature.\n\ + Consider setting minimal API version to 21 at least. From b3a88821f5e469d4d2741a9e1db0c2daa37758df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 15:00:20 +0200 Subject: [PATCH 10/37] 10: Create SecureRandomPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 11 ++++-- .../esa/core/plugin/SecureRandomPlugin.java | 38 +++++++++++++++++++ src/main/resources/description.properties | 4 ++ 3 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 00e1a80..c2c7dde 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -1,10 +1,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.archetype.Plugin; -import com.bartek.esa.core.plugin.AllowBackupPlugin; -import com.bartek.esa.core.plugin.DebuggablePlugin; -import com.bartek.esa.core.plugin.LoggingPlugin; -import com.bartek.esa.core.plugin.PermissionsRaceConditionPlugin; +import com.bartek.esa.core.plugin.*; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import dagger.Module; @@ -47,4 +44,10 @@ public class PluginModule { public Plugin permissionRaceConditionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new PermissionsRaceConditionPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin secureRandomPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new SecureRandomPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java b/src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java new file mode 100644 index 0000000..bcefe42 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java @@ -0,0 +1,38 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.NodeList; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.ObjectCreationExpr; +import com.github.javaparser.ast.visitor.VoidVisitorAdapter; + +import javax.inject.Inject; + +public class SecureRandomPlugin extends JavaPlugin { + + @Inject + public SecureRandomPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.accept(new VoidVisitorAdapter() { + @Override + public void visit(ObjectCreationExpr objectCreation, Void arg) { + String identifier = objectCreation.getType().getName().getIdentifier(); + NodeList arguments = objectCreation.getArguments(); + + if(identifier.equals("SecureRandom") && arguments.isNonEmpty()) { + addIssue(Severity.VULNERABILITY, getLineNumberFromExpression(objectCreation), objectCreation.toString()); + } + + super.visit(objectCreation, arg); + } + }, null); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index e96b19e..5f75bcc 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -46,3 +46,7 @@ com.bartek.esa.core.plugin.PermissionsRaceConditionPlugin=Potential permissions There are declared custom permissions in AndroidManifest.xml and the minimal API version is set to less than 21.\n\ It means that declared permissions can be obtained by malicious application installed before and without need of having 1proper signature.\n\ Consider setting minimal API version to 21 at least. + +com.bartek.esa.core.plugin.SecureRandomPlugin=Initializing SecureRandom object with custom seed. \n\ + Specifying custom seed for SecureRandom can produce predictable sequence of numbers. \n\ + Please create SecureRandom object without any arguments instead. \ No newline at end of file From 6a238a497c080cacc8d6cf35289905954537f42a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 17:01:05 +0200 Subject: [PATCH 11/37] 10: Create ImplicitIntentsPlugin --- .../com/bartek/esa/core/di/CoreModule.java | 6 ++ .../com/bartek/esa/core/di/PluginModule.java | 7 ++ .../core/java/JavaSyntaxRegexProvider.java | 31 ++++++++ .../core/plugin/ImplicitIntentsPlugin.java | 74 +++++++++++++++++++ src/main/resources/description.properties | 7 +- 5 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java create mode 100644 src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/CoreModule.java b/src/main/java/com/bartek/esa/core/di/CoreModule.java index bafe8ec..abb9f9d 100644 --- a/src/main/java/com/bartek/esa/core/di/CoreModule.java +++ b/src/main/java/com/bartek/esa/core/di/CoreModule.java @@ -2,6 +2,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.desc.provider.DescriptionProvider; import com.bartek.esa.core.executor.PluginExecutor; +import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.xml.XmlHelper; import dagger.Module; import dagger.Provides; @@ -19,6 +20,11 @@ public class CoreModule { return new DescriptionProvider(); } + @Provides + public JavaSyntaxRegexProvider javaSyntaxRegexProvider() { + return new JavaSyntaxRegexProvider(); + } + @Provides public XmlHelper xmlHelper() { return new XmlHelper(); diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index c2c7dde..3718954 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -1,6 +1,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.archetype.Plugin; +import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.plugin.*; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; @@ -50,4 +51,10 @@ public class PluginModule { public Plugin secureRandomPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new SecureRandomPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin implicitIntentsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, JavaSyntaxRegexProvider javaSyntaxRegexProvider) { + return new ImplicitIntentsPlugin(globMatcher, xmlHelper, javaSyntaxRegexProvider); + } } diff --git a/src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java b/src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java new file mode 100644 index 0000000..552c193 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/java/JavaSyntaxRegexProvider.java @@ -0,0 +1,31 @@ +package com.bartek.esa.core.java; + +import com.github.javaparser.ast.expr.Expression; + +import javax.inject.Inject; +import java.util.regex.Pattern; + +public class JavaSyntaxRegexProvider { + + @Inject + public JavaSyntaxRegexProvider() { + + } + + public Pattern constant() { + return Pattern.compile("^[A-Z0-9_$]*$"); + } + + public boolean isConstant(Expression expression) { + String value = expression.toString(); + if(expression.isNameExpr() && constant().matcher(value).matches()) { + return true; + } + + if(expression.isFieldAccessExpr()) { + return constant().matcher(expression.asFieldAccessExpr().getName().getIdentifier()).matches(); + } + + return false; + } +} diff --git a/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java b/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java new file mode 100644 index 0000000..7e104b3 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java @@ -0,0 +1,74 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.java.JavaSyntaxRegexProvider; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.NodeList; +import com.github.javaparser.ast.body.VariableDeclarator; +import com.github.javaparser.ast.expr.*; + +import javax.inject.Inject; +import java.util.List; +import java.util.stream.Collectors; + +public class ImplicitIntentsPlugin extends JavaPlugin { + private final JavaSyntaxRegexProvider java; + + @Inject + public ImplicitIntentsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, JavaSyntaxRegexProvider javaSyntaxRegexProvider) { + super(globMatcher, xmlHelper); + this.java = javaSyntaxRegexProvider; + } + + @Override + public void run(CompilationUnit compilationUnit) { + List intentVariables = getIntentVariables(compilationUnit); + findAllSetActionMethodInvocations(compilationUnit, intentVariables); + compilationUnit.findAll(ObjectCreationExpr.class).stream() + .filter(expr -> expr.getType().getName().getIdentifier().equals("Intent")) + .filter(this::checkConstructor) + .forEach(objectCreation -> addIssue(Severity.INFO, getLineNumberFromExpression(objectCreation), objectCreation.toString())); + + } + + private boolean checkConstructor(ObjectCreationExpr objectCreation) { + NodeList arguments = objectCreation.getArguments(); + boolean isImplicit = false; + if (arguments.size() == 1) { + Expression argument = arguments.get(0); + isImplicit = java.isConstant(argument); + } + + if(arguments.size() == 2) { + Expression argument = arguments.get(0); + isImplicit = !argument.isThisExpr(); + } + + return isImplicit; + } + + private void findAllSetActionMethodInvocations(CompilationUnit compilationUnit, List intentVariables) { + compilationUnit.findAll(MethodCallExpr.class).forEach(methodCall -> { + boolean isCalledOnIntentObject = methodCall.getScope() + .map(Expression::toString) + .filter(intentVariables::contains) + .isPresent(); + if(isCalledOnIntentObject && methodCall.getName().getIdentifier().equals("setAction")) { + addIssue(Severity.INFO, getLineNumberFromExpression(methodCall), methodCall.toString()); + } + }); + } + + private List getIntentVariables(CompilationUnit compilationUnit) { + return compilationUnit.findAll(VariableDeclarationExpr.class).stream() + .filter(expr -> expr.getElementType().toString().equals("Intent")) + .map(VariableDeclarationExpr::getVariables) + .flatMap(NodeList::stream) + .map(VariableDeclarator::getName) + .map(SimpleName::getIdentifier) + .collect(Collectors.toList()); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 5f75bcc..4f0141d 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -49,4 +49,9 @@ com.bartek.esa.core.plugin.PermissionsRaceConditionPlugin=Potential permissions com.bartek.esa.core.plugin.SecureRandomPlugin=Initializing SecureRandom object with custom seed. \n\ Specifying custom seed for SecureRandom can produce predictable sequence of numbers. \n\ - Please create SecureRandom object without any arguments instead. \ No newline at end of file + Please create SecureRandom object without any arguments instead. + +com.bartek.esa.core.plugin.ImplicitIntentsPlugin=Creating implicit intent. Potential data leakage. \n\ + Implicit intents can be abused in man-in-the-middle attack. Malicious application can hijack intent and start its\n\ + activity/send service etc. to steal sent data. \n\ + Also make sure that no sensitive information is passing to this intent. \ No newline at end of file From c1c8b7e7eedf91c20b8c61230b9ebcf3594e2d98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 19:43:36 +0200 Subject: [PATCH 12/37] 10: Improve ImplicitIntentsPlugin to handle with pending intents fed with implicit intents --- .../core/plugin/ImplicitIntentsPlugin.java | 132 +++++++++++++++--- src/main/resources/description.properties | 8 +- 2 files changed, 120 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java b/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java index 7e104b3..aca4bf6 100644 --- a/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java @@ -6,12 +6,14 @@ import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.body.VariableDeclarator; import com.github.javaparser.ast.expr.*; import javax.inject.Inject; import java.util.List; +import java.util.Optional; import java.util.stream.Collectors; public class ImplicitIntentsPlugin extends JavaPlugin { @@ -25,23 +27,73 @@ public class ImplicitIntentsPlugin extends JavaPlugin { @Override public void run(CompilationUnit compilationUnit) { - List intentVariables = getIntentVariables(compilationUnit); - findAllSetActionMethodInvocations(compilationUnit, intentVariables); - compilationUnit.findAll(ObjectCreationExpr.class).stream() - .filter(expr -> expr.getType().getName().getIdentifier().equals("Intent")) - .filter(this::checkConstructor) - .forEach(objectCreation -> addIssue(Severity.INFO, getLineNumberFromExpression(objectCreation), objectCreation.toString())); - + checkCreatingImplicitIntents(compilationUnit); + checkCreatingPendingIntentsWithoutIntentVariable(compilationUnit); + checkCreatingPendingIntentsWithIntentVariables(compilationUnit); + checkCreatingPendingIntentsWithIntentsArraysVariables(compilationUnit); } - private boolean checkConstructor(ObjectCreationExpr objectCreation) { + // Works for: + // Intent[] myIntents = { new Intent(...), ... } + // getActivities(this, 0, myIntents, 0); + private void checkCreatingPendingIntentsWithIntentsArraysVariables(CompilationUnit compilationUnit) { + List implicitIntentsArraysVariables = compilationUnit.findAll(ObjectCreationExpr.class).stream() + .filter(expr -> expr.getType().getName().getIdentifier().equals("Intent")) + .filter(this::isCreatingImplicitIntent) + .map(Node::getParentNode) + .flatMap(Optional::stream) + .map(Node::getParentNode) + .flatMap(Optional::stream) + .filter(node -> node instanceof VariableDeclarator) + .map(node -> (VariableDeclarator) node) + .map(VariableDeclarator::getName) + .map(SimpleName::getIdentifier) + .collect(Collectors.toList()); + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("getActivities")) + .filter(expr -> expr.getArguments().size() >= 4) + .filter(expr -> expr.getArguments().get(2).isNameExpr()) + .filter(expr -> implicitIntentsArraysVariables.contains(expr.getArguments().get(2).asNameExpr().getName().getIdentifier())) + .forEach(expr -> addIssue(Severity.VULNERABILITY, ".PENDING_INTENT", getLineNumberFromExpression(expr), expr.toString())); + } + + private void checkCreatingImplicitIntents(CompilationUnit compilationUnit) { + List intentVariables = getIntentVariables(compilationUnit); + checkAllSetActionMethodInvocations(compilationUnit, intentVariables); + checkCreatingImplicitIntentsUsingConstructor(compilationUnit); + } + + // Works for: new Intent(Intent.ABC), new Intent(ABC) + private void checkCreatingImplicitIntentsUsingConstructor(CompilationUnit compilationUnit) { + compilationUnit.findAll(ObjectCreationExpr.class).stream() + .filter(expr -> expr.getType().getName().getIdentifier().equals("Intent")) + .filter(this::isCreatingImplicitIntent) + .forEach(objectCreation -> addIssue(Severity.INFO, ".IMPLICIT_INTENT", getLineNumberFromExpression(objectCreation), objectCreation.toString())); + } + + // Returns: i for: Intent i = new Intent(...) + private List getIntentVariables(CompilationUnit compilationUnit) { + return compilationUnit.findAll(VariableDeclarationExpr.class).stream() + .filter(expr -> expr.getElementType().toString().equals("Intent")) + .map(VariableDeclarationExpr::getVariables) + .flatMap(NodeList::stream) + .map(VariableDeclarator::getName) + .map(SimpleName::getIdentifier) + .collect(Collectors.toList()); + } + + // Checks if: new Intent(Intent.ABC), new Intent(ABC) + private boolean isCreatingImplicitIntent(ObjectCreationExpr objectCreation) { NodeList arguments = objectCreation.getArguments(); boolean isImplicit = false; + + // Works for: new Intent(CONSTANT, ...) if (arguments.size() == 1) { Expression argument = arguments.get(0); isImplicit = java.isConstant(argument); } + // Not works for: new Intent(this, ...) if(arguments.size() == 2) { Expression argument = arguments.get(0); isImplicit = !argument.isThisExpr(); @@ -50,25 +102,69 @@ public class ImplicitIntentsPlugin extends JavaPlugin { return isImplicit; } - private void findAllSetActionMethodInvocations(CompilationUnit compilationUnit, List intentVariables) { + // Works for: i.setAction(...) + private void checkAllSetActionMethodInvocations(CompilationUnit compilationUnit, List intentVariables) { compilationUnit.findAll(MethodCallExpr.class).forEach(methodCall -> { boolean isCalledOnIntentObject = methodCall.getScope() .map(Expression::toString) .filter(intentVariables::contains) .isPresent(); if(isCalledOnIntentObject && methodCall.getName().getIdentifier().equals("setAction")) { - addIssue(Severity.INFO, getLineNumberFromExpression(methodCall), methodCall.toString()); + addIssue(Severity.INFO, ".IMPLICIT_INTENT", getLineNumberFromExpression(methodCall), methodCall.toString()); } }); } - private List getIntentVariables(CompilationUnit compilationUnit) { - return compilationUnit.findAll(VariableDeclarationExpr.class).stream() - .filter(expr -> expr.getElementType().toString().equals("Intent")) - .map(VariableDeclarationExpr::getVariables) - .flatMap(NodeList::stream) - .map(VariableDeclarator::getName) - .map(SimpleName::getIdentifier) - .collect(Collectors.toList()); + // Works for: + // Intent myIntent = new Intent(...) + // getActivity(this, 0, myIntent, 0); + private void checkCreatingPendingIntentsWithIntentVariables(CompilationUnit compilationUnit) { + List implicitIntentsVariables = compilationUnit.findAll(ObjectCreationExpr.class).stream() + .filter(expr -> expr.getType().getName().getIdentifier().equals("Intent")) + .filter(this::isCreatingImplicitIntent) + .map(Node::getParentNode) + .flatMap(Optional::stream) + .filter(node -> node instanceof VariableDeclarator) + .map(node -> (VariableDeclarator) node) + .map(VariableDeclarator::getName) + .map(SimpleName::getIdentifier) + .collect(Collectors.toList()); + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("getActivity|getBroadcast|getService")) + .filter(expr -> expr.getArguments().size() >= 4) + .filter(expr -> expr.getArguments().get(2).isNameExpr()) + .filter(expr -> implicitIntentsVariables.contains(expr.getArguments().get(2).asNameExpr().getName().getIdentifier())) + .forEach(expr -> addIssue(Severity.VULNERABILITY, ".PENDING_INTENT", getLineNumberFromExpression(expr), expr.toString())); + } + + private void checkCreatingPendingIntentsWithoutIntentVariable(CompilationUnit compilationUnit) { + // Works for: getActivity(this, 0, new Intent(...), 0) + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("getActivity|getBroadcast|getService")) + .filter(expr -> expr.getArguments().size() >= 4) + .filter(expr -> expr.getArguments().get(2).isObjectCreationExpr()) + .filter(expr -> isCreatingImplicitIntent(expr.getArguments().get(2).asObjectCreationExpr())) + .forEach(expr -> addIssue(Severity.VULNERABILITY, ".PENDING_INTENT", getLineNumberFromExpression(expr), expr.toString())); + + // Works for: getActivities(this, 0, new Intent[] { new Intent(...), ...}, 0) + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("getActivities")) + .filter(expr -> expr.getArguments().size() >= 4) + .filter(expr -> expr.getArguments().get(2).isArrayCreationExpr()) + .filter(expr -> isCreatingImplicitIntentsArray(expr.getArguments().get(2).asArrayCreationExpr())) + .forEach(expr -> addIssue(Severity.VULNERABILITY, ".PENDING_INTENT", getLineNumberFromExpression(expr), expr.toString())); + } + + private boolean isCreatingImplicitIntentsArray(ArrayCreationExpr arrayCreationExpr) { + return arrayCreationExpr.getInitializer() + .map(this::isCreatingImplicitIntentsArray) + .orElse(false); + } + + private boolean isCreatingImplicitIntentsArray(ArrayInitializerExpr arrayInitializerExpr) { + return arrayInitializerExpr.getValues().stream() + .filter(Expression::isObjectCreationExpr) + .map(Expression::asObjectCreationExpr) + .anyMatch(this::isCreatingImplicitIntent); } } diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 4f0141d..9d1137f 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -51,7 +51,11 @@ com.bartek.esa.core.plugin.SecureRandomPlugin=Initializing SecureRandom object w Specifying custom seed for SecureRandom can produce predictable sequence of numbers. \n\ Please create SecureRandom object without any arguments instead. -com.bartek.esa.core.plugin.ImplicitIntentsPlugin=Creating implicit intent. Potential data leakage. \n\ +com.bartek.esa.core.plugin.ImplicitIntentsPlugin.IMPLICIT_INTENT=Creating implicit intent. Potential data leakage. \n\ Implicit intents can be abused in man-in-the-middle attack. Malicious application can hijack intent and start its\n\ activity/send service etc. to steal sent data. \n\ - Also make sure that no sensitive information is passing to this intent. \ No newline at end of file + Also make sure that no sensitive information is passing to this intent. + +com.bartek.esa.core.plugin.ImplicitIntentsPlugin.PENDING_INTENT=Creating pending intent from implicit intent. Potential permission escalation vulnerability\n\ + As far as pending intents contains UID of issuing application and its permissions, they should be fed only\n\ + with explicit intents to avoid permission escalation vulnerability. \ No newline at end of file From c82523246b5ba3eaf274aa47e4161dd2f88c72bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 19:49:00 +0200 Subject: [PATCH 13/37] 10: Refactor Java plugins to use lambdas instead of visitors in CompilationUnit --- .../bartek/esa/core/plugin/LoggingPlugin.java | 13 +++--------- .../esa/core/plugin/SecureRandomPlugin.java | 20 ++++--------------- 2 files changed, 7 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java b/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java index 89be22d..63b3392 100644 --- a/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java @@ -6,7 +6,6 @@ import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.visitor.VoidVisitorAdapter; import javax.inject.Inject; @@ -19,14 +18,8 @@ public class LoggingPlugin extends JavaPlugin { @Override public void run(CompilationUnit compilationUnit) { - compilationUnit.accept(new VoidVisitorAdapter() { - @Override - public void visit(MethodCallExpr methodCall, Void arg) { - if (methodCall.getName().getIdentifier().matches("v|d|i|w|e|wtf")) { - addIssue(Severity.INFO, getLineNumberFromExpression(methodCall), methodCall.toString()); - } - super.visit(methodCall, arg); - } - }, null); + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("v|d|i|w|e|wtf")) + .forEach(expr -> addIssue(Severity.INFO, getLineNumberFromExpression(expr), expr.toString())); } } diff --git a/src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java b/src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java index bcefe42..0587539 100644 --- a/src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/SecureRandomPlugin.java @@ -5,10 +5,7 @@ import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.NodeList; -import com.github.javaparser.ast.expr.Expression; import com.github.javaparser.ast.expr.ObjectCreationExpr; -import com.github.javaparser.ast.visitor.VoidVisitorAdapter; import javax.inject.Inject; @@ -21,18 +18,9 @@ public class SecureRandomPlugin extends JavaPlugin { @Override public void run(CompilationUnit compilationUnit) { - compilationUnit.accept(new VoidVisitorAdapter() { - @Override - public void visit(ObjectCreationExpr objectCreation, Void arg) { - String identifier = objectCreation.getType().getName().getIdentifier(); - NodeList arguments = objectCreation.getArguments(); - - if(identifier.equals("SecureRandom") && arguments.isNonEmpty()) { - addIssue(Severity.VULNERABILITY, getLineNumberFromExpression(objectCreation), objectCreation.toString()); - } - - super.visit(objectCreation, arg); - } - }, null); + compilationUnit.findAll(ObjectCreationExpr.class).stream() + .filter(expr -> expr.getType().getName().getIdentifier().equals("SecureRandom")) + .filter(expr -> expr.getArguments().isNonEmpty()) + .forEach(expr -> addIssue(Severity.VULNERABILITY, getLineNumberFromExpression(expr), expr.toString())); } } From 2bd72508057a40f274ce8b0dbe6b0834caa772cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 21:01:38 +0200 Subject: [PATCH 14/37] 10: Create SharedUidPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 ++++ .../esa/core/plugin/SharedUidPlugin.java | 28 +++++++++++++++++++ src/main/resources/description.properties | 7 ++++- 3 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/SharedUidPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 3718954..c0f59ac 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -57,4 +57,10 @@ public class PluginModule { public Plugin implicitIntentsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, JavaSyntaxRegexProvider javaSyntaxRegexProvider) { return new ImplicitIntentsPlugin(globMatcher, xmlHelper, javaSyntaxRegexProvider); } + + @Provides + @IntoSet + public Plugin sharedUidPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new SharedUidPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/SharedUidPlugin.java b/src/main/java/com/bartek/esa/core/plugin/SharedUidPlugin.java new file mode 100644 index 0000000..b0754ad --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/SharedUidPlugin.java @@ -0,0 +1,28 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; + +import javax.inject.Inject; +import javax.xml.xpath.XPathConstants; +import java.util.Optional; + +public class SharedUidPlugin extends AndroidManifestPlugin { + + @Inject + public SharedUidPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + Node manifestNode = (Node) xPath(xml, "/manifest", XPathConstants.NODE); + Optional.ofNullable(manifestNode.getAttributes().getNamedItem("android:sharedUserId")).ifPresent(node -> { + addIssue(Severity.VULNERABILITY, null, node.toString()); + }); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 9d1137f..aeaae18 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -58,4 +58,9 @@ com.bartek.esa.core.plugin.ImplicitIntentsPlugin.IMPLICIT_INTENT=Creating implic com.bartek.esa.core.plugin.ImplicitIntentsPlugin.PENDING_INTENT=Creating pending intent from implicit intent. Potential permission escalation vulnerability\n\ As far as pending intents contains UID of issuing application and its permissions, they should be fed only\n\ - with explicit intents to avoid permission escalation vulnerability. \ No newline at end of file + with explicit intents to avoid permission escalation vulnerability. + +com.bartek.esa.core.plugin.SharedUidPlugin=Making use of shared UserID.\n\ + Shared UserID violates a sandbox nature of Android system. All applications working with the same UID work also \n\ + within the same process and share granted permissions, resources and so on.\n\ + Remember, that if you really want to use this feature, after publishing your app, you won't be able to change it anymore. \ No newline at end of file From 84f691a8a54646bada97a273e61dcf4ded42904a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 21:02:24 +0200 Subject: [PATCH 15/37] Revert "10: Add checking element in BasePlugin" This reverts commit c254e530ad3ffcc505a5d073744a83278416fc06. --- .../bartek/esa/core/archetype/BasePlugin.java | 43 +------------------ .../bartek/esa/core/archetype/JavaPlugin.java | 1 - .../bartek/esa/core/archetype/XmlPlugin.java | 1 - .../esa/core/plugin/AllowBackupPlugin.java | 2 +- .../esa/core/plugin/DebuggablePlugin.java | 2 +- src/main/resources/description.properties | 13 ------ 6 files changed, 3 insertions(+), 59 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java b/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java index 4dc66ab..ed3465d 100644 --- a/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/BasePlugin.java @@ -2,11 +2,8 @@ package com.bartek.esa.core.archetype; import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.model.object.Issue; -import com.bartek.esa.core.xml.XmlHelper; import org.w3c.dom.Document; -import org.w3c.dom.Node; -import javax.xml.xpath.XPathConstants; import java.io.File; import java.util.HashMap; import java.util.HashSet; @@ -14,15 +11,10 @@ import java.util.Map; import java.util.Set; public abstract class BasePlugin implements Plugin { - private final XmlHelper xmlHelper; private Set issues = new HashSet<>(); private Document manifest; private File file; - public BasePlugin(XmlHelper xmlHelper) { - this.xmlHelper = xmlHelper; - } - @Override public void update(File file, Document manifest) { this.file = file; @@ -32,43 +24,10 @@ public abstract class BasePlugin implements Plugin { @Override public Set runForIssues() { - if(canBeExecuted()) { - run(file); - } + run(file); return issues; } - private boolean canBeExecuted() { - return hasDeclaredApiVersion(); - } - - private boolean hasDeclaredApiVersion() { - Node usesSdkNode = (Node) xmlHelper.xPath(manifest, "/manifest/uses-sdk", XPathConstants.NODE); - if(usesSdkNode == null) { - Issue issue = Issue.builder() - .issuer(BasePlugin.class) - .descriptionCode(".NO_USES_SDK") - .severity(Severity.ERROR) - .build(); - addIssue(issue); - - return false; - } - - if(usesSdkNode.getAttributes().getNamedItem("android:minSdkVersion") == null) { - Issue issue = Issue.builder() - .issuer(BasePlugin.class) - .descriptionCode(".USES_SDK.NO_MIN_SDK_VERSION") - .severity(Severity.ERROR) - .build(); - addIssue(issue); - - return false; - } - - return true; - } - protected abstract void run(File file); protected void addIssue(Severity severity, Integer lineNumber, String line) { diff --git a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java index d9f9791..030587f 100644 --- a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java @@ -21,7 +21,6 @@ public abstract class JavaPlugin extends BasePlugin { private final XmlHelper xmlHelper; public JavaPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { - super(xmlHelper); this.globMatcher = globMatcher; this.xmlHelper = xmlHelper; } diff --git a/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java b/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java index 082c984..c1bbedd 100644 --- a/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java @@ -12,7 +12,6 @@ public abstract class XmlPlugin extends BasePlugin { private final XmlHelper xmlHelper; public XmlPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { - super(xmlHelper); this.globMatcher = globMatcher; this.xmlHelper = xmlHelper; } diff --git a/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java b/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java index d188bc0..87c21a6 100644 --- a/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/AllowBackupPlugin.java @@ -25,6 +25,6 @@ public class AllowBackupPlugin extends AndroidManifestPlugin { if (!n.getNodeValue().equals("false")) { addIssue(Severity.WARNING, ".NO_FALSE", null, n.toString()); } - }, () -> addIssue(Severity.WARNING, ".NO_ATTR", null, null)); + }, () -> addIssue(Severity.ERROR, ".NO_ATTR", null, null)); } } diff --git a/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java b/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java index 481ad80..e198352 100644 --- a/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/DebuggablePlugin.java @@ -25,6 +25,6 @@ public class DebuggablePlugin extends AndroidManifestPlugin { if(!n.getNodeValue().equals("false")) { addIssue(Severity.WARNING, ".NO_FALSE", null, n.toString()); } - }, () -> addIssue(Severity.WARNING, ".NO_ATTR",null, null)); + }, () -> addIssue(Severity.ERROR, ".NO_ATTR",null, null)); } } diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index aeaae18..d18b8bf 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -1,16 +1,3 @@ -com.bartek.esa.core.archetype.BasePlugin.NO_USES_SDK=There is no defined in AndroidManifest.xml file. \n\ - In order to use this tool, should be defined in AndroidManifest.xml with android:minSdkVersion attribute at least.\n\ - This element should be placed below the root () level.\n\ - For example: \n\ - \n\ - \t\n\ - \t...\n\ - - -com.bartek.esa.core.archetype.BasePlugin.USES_SDK.NO_MIN_SDK_VERSION=There is no minSdkVersion defined in AndroidManifest.xml file. \n\ - In order to use this tool, minimal SDK version should be provided as the attribute of element.\n\ - For example: - com.bartek.esa.core.archetype.JavaPlugin.NO_PACKAGE=There is no package defined in AndroidManifest.xml file. \n\ Package should be defined as attribute of tag.\n\ For example: \n\ From dd859e76f4975a1f52039d139823b523b4afb5f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 21:23:06 +0200 Subject: [PATCH 16/37] 10: Create UsesSdkPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 ++++ .../bartek/esa/core/plugin/UsesSdkPlugin.java | 29 +++++++++++++++++++ src/main/resources/description.properties | 15 +++++++++- 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index c0f59ac..1858beb 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -63,4 +63,10 @@ public class PluginModule { public Plugin sharedUidPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new SharedUidPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin usesSdkPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new UsesSdkPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java b/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java new file mode 100644 index 0000000..a8704cb --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java @@ -0,0 +1,29 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; + +import javax.inject.Inject; +import javax.xml.xpath.XPathConstants; +import java.util.Optional; + +public class UsesSdkPlugin extends AndroidManifestPlugin { + + @Inject + public UsesSdkPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + Optional.ofNullable((Node) xPath(xml, "/manifest/uses-sdk", XPathConstants.NODE)).ifPresentOrElse(usesSdkNode -> { + if(usesSdkNode.getAttributes().getNamedItem("android:minSdkVersion") == null) { + addIssue(Severity.ERROR, ".USES_SDK.NO_MIN_SDK_VERSION", null, null); + } + }, () -> addIssue(Severity.ERROR, ".NO_USES_SDK", null, null)); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index d18b8bf..a19a336 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -50,4 +50,17 @@ com.bartek.esa.core.plugin.ImplicitIntentsPlugin.PENDING_INTENT=Creating pending com.bartek.esa.core.plugin.SharedUidPlugin=Making use of shared UserID.\n\ Shared UserID violates a sandbox nature of Android system. All applications working with the same UID work also \n\ within the same process and share granted permissions, resources and so on.\n\ - Remember, that if you really want to use this feature, after publishing your app, you won't be able to change it anymore. \ No newline at end of file + Remember, that if you really want to use this feature, after publishing your app, you won't be able to change it anymore. + +com.bartek.esa.core.plugin.UsesSdkPlugin.NO_USES_SDK=There is no defined in AndroidManifest.xml file.\n\ + In order to use this tool, should be defined in AndroidManifest.xml with android:minSdkVersion attribute at least.\n\ + This element should be placed below the root () level.\n\ + For example:\n\ + \n\ + \t\n\ + \t...\n\ + + +com.bartek.esa.core.plugin.UsesSdkPlugin.USES_SDK.NO_MIN_SDK_VERSION=There is no minSdkVersion defined in AndroidManifest.xml file.\n\ + In order to use this tool, minimal SDK version should be provided as the attribute of element.\n\ + For example: \ No newline at end of file From 866a5d053278cffb7164451a0cda31391284ab8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 21:48:40 +0200 Subject: [PATCH 17/37] 10: Add 'esa' bash script --- esa | 3 +++ 1 file changed, 3 insertions(+) create mode 100755 esa diff --git a/esa b/esa new file mode 100755 index 0000000..c454e05 --- /dev/null +++ b/esa @@ -0,0 +1,3 @@ +#!/bin/bash + +java -jar build/libs/esa-1.0-SNAPSHOT.jar $@ From 11609c693f844f4b1f96451d18f3578851baa403 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 23:03:00 +0200 Subject: [PATCH 18/37] 10: Create CipherInstancePlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 ++ .../esa/core/plugin/CipherInstancePlugin.java | 71 +++++++++++++++++++ src/main/resources/description.properties | 8 ++- 3 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 1858beb..9c2e8ba 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -69,4 +69,10 @@ public class PluginModule { public Plugin usesSdkPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new UsesSdkPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin cipherInstancePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new CipherInstancePlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java b/src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java new file mode 100644 index 0000000..1ff6ee2 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java @@ -0,0 +1,71 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.ImportDeclaration; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.*; + +import javax.inject.Inject; +import java.util.Optional; +import java.util.regex.Pattern; + +public class CipherInstancePlugin extends JavaPlugin { + private static final Pattern ALGORITHM_QUALIFIER = Pattern.compile("^\"\\w+/\\w+/\\w+\"$"); + + @Inject + public CipherInstancePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().equals("getInstance")) + .filter(expr -> isCipherMethod(expr, compilationUnit)) + .filter(expr -> expr.getArguments().isNonEmpty()) + .filter(expr -> !isFullCipherQualifier(expr.getArguments().get(0).toString())) + .forEach(expr -> addIssue(Severity.ERROR, getLineNumberFromExpression(expr), expr.toString())); + } + + private boolean isCipherMethod(MethodCallExpr expr, CompilationUnit compilationUnit) { + boolean isCipherMethod = expr.getScope() + .filter(Expression::isNameExpr) + .map(Expression::asNameExpr) + .map(NameExpr::getName) + .map(SimpleName::getIdentifier) + .filter(name -> name.equals("Cipher")) + .isPresent(); + + if(!isCipherMethod) { + isCipherMethod = compilationUnit.findAll(ImportDeclaration.class).stream() + .filter(ImportDeclaration::isStatic) + .filter(e -> e.getName().getIdentifier().equals("getInstance")) + .map(ImportDeclaration::getName) + .map(Name::getQualifier) + .flatMap(Optional::stream) + .map(Node::toString) + .anyMatch(q -> q.equals("javax.crypto.Cipher")); + } + + if(!isCipherMethod) { + isCipherMethod = compilationUnit.findAll(ImportDeclaration.class).stream() + .filter(ImportDeclaration::isStatic) + .filter(ImportDeclaration::isAsterisk) + .map(ImportDeclaration::getName) + .map(Name::getQualifier) + .flatMap(Optional::stream) + .map(Node::toString) + .anyMatch(q -> q.equals("javax.crypto")); + } + + return isCipherMethod; + } + + private boolean isFullCipherQualifier(String qualifier) { + return ALGORITHM_QUALIFIER.matcher(qualifier).matches(); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index a19a336..bce4886 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -63,4 +63,10 @@ com.bartek.esa.core.plugin.UsesSdkPlugin.NO_USES_SDK=There is no defi com.bartek.esa.core.plugin.UsesSdkPlugin.USES_SDK.NO_MIN_SDK_VERSION=There is no minSdkVersion defined in AndroidManifest.xml file.\n\ In order to use this tool, minimal SDK version should be provided as the attribute of element.\n\ - For example: \ No newline at end of file + For example: + +com.bartek.esa.core.plugin.CipherInstancePlugin=Not fully-qualified algorithm name provided in Cipher.getInstance() method.\n\ + Passing a shortcut instead of fully-qualified algorithm name in Cipher.getInstance() method is not portable across providers\n\ + and can impact the system low secure than intended to be.\n\ + Fully-qualified name matches the pattern: algorithm/mode/pattern\n\ + For example: AES/CBC/PKCS5Padding \ No newline at end of file From 79366845202714af991eea8e2006d2d57fa6dc7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 5 Apr 2019 23:18:16 +0200 Subject: [PATCH 19/37] 10: Update UsesSdkPlugin to check maxSdkVersion definition --- src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java | 4 ++++ src/main/resources/description.properties | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java b/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java index a8704cb..9ac43e9 100644 --- a/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java @@ -24,6 +24,10 @@ public class UsesSdkPlugin extends AndroidManifestPlugin { if(usesSdkNode.getAttributes().getNamedItem("android:minSdkVersion") == null) { addIssue(Severity.ERROR, ".USES_SDK.NO_MIN_SDK_VERSION", null, null); } + + Optional.ofNullable(usesSdkNode.getAttributes().getNamedItem("android:maxSdkVersion")).ifPresent(maxSdkVersion -> + addIssue(Severity.ERROR, ".USES_SDK.MAX_SDK_VERSION", null, maxSdkVersion.toString()) + ); }, () -> addIssue(Severity.ERROR, ".NO_USES_SDK", null, null)); } } diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index bce4886..87a867b 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -65,6 +65,12 @@ com.bartek.esa.core.plugin.UsesSdkPlugin.USES_SDK.NO_MIN_SDK_VERSION=There is no In order to use this tool, minimal SDK version should be provided as the attribute of element.\n\ For example: +com.bartek.esa.core.plugin.UsesSdkPlugin.USES_SDK.MAX_SDK_VERSION=Application defines an upper limit for API version.\n\ + The android:maxSdkVersion is defined in AndroidManifest.xml.\n\ + There is no need to limit available platforms for application.\n\ + Furthermore it can cause unexpected application uninstall\n\ + on upgrading Android version (along with API which can exceed defined maximal API version). + com.bartek.esa.core.plugin.CipherInstancePlugin=Not fully-qualified algorithm name provided in Cipher.getInstance() method.\n\ Passing a shortcut instead of fully-qualified algorithm name in Cipher.getInstance() method is not portable across providers\n\ and can impact the system low secure than intended to be.\n\ From 378b2fa967adaccd878740f62538b7ee201a388f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Sat, 6 Apr 2019 11:32:10 +0200 Subject: [PATCH 20/37] 10: Create StrictModePlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 ++ .../esa/core/plugin/StrictModePlugin.java | 63 +++++++++++++++++++ src/main/resources/description.properties | 5 +- 3 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 9c2e8ba..174f2bc 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -75,4 +75,10 @@ public class PluginModule { public Plugin cipherInstancePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new CipherInstancePlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin strictModePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new StrictModePlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java b/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java new file mode 100644 index 0000000..e31aac0 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java @@ -0,0 +1,63 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.ImportDeclaration; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.*; + +import javax.inject.Inject; +import java.util.Optional; + +public class StrictModePlugin extends JavaPlugin { + + @Inject + public StrictModePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().equals("setThreadPolicy")) + .filter(expr -> isStrictModeScope(expr, compilationUnit)) + .forEach(expr -> addIssue(Severity.INFO, getLineNumberFromExpression(expr), expr.toString())); + } + + private boolean isStrictModeScope(MethodCallExpr expr, CompilationUnit compilationUnit) { + boolean isStrictModeScope = expr.getScope() + .filter(Expression::isNameExpr) + .map(Expression::asNameExpr) + .map(NameExpr::getName) + .map(SimpleName::getIdentifier) + .map(s -> s.equals("StrictMode")) + .orElse(false); + + if(!isStrictModeScope) { + isStrictModeScope = compilationUnit.findAll(ImportDeclaration.class).stream() + .filter(ImportDeclaration::isStatic) + .filter(e -> e.getName().getIdentifier().equals("setThreadPolicy")) + .map(ImportDeclaration::getName) + .map(Name::getQualifier) + .flatMap(Optional::stream) + .map(Node::toString) + .anyMatch(q -> q.equals("android.os.StrictMode")); + } + + if(!isStrictModeScope) { + isStrictModeScope = compilationUnit.findAll(ImportDeclaration.class).stream() + .filter(ImportDeclaration::isStatic) + .filter(ImportDeclaration::isAsterisk) + .map(ImportDeclaration::getName) + .map(Name::getQualifier) + .flatMap(Optional::stream) + .map(Node::toString) + .anyMatch(q -> q.equals("android.os")); + } + + return isStrictModeScope; + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 87a867b..8424a59 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -75,4 +75,7 @@ com.bartek.esa.core.plugin.CipherInstancePlugin=Not fully-qualified algorithm na Passing a shortcut instead of fully-qualified algorithm name in Cipher.getInstance() method is not portable across providers\n\ and can impact the system low secure than intended to be.\n\ Fully-qualified name matches the pattern: algorithm/mode/pattern\n\ - For example: AES/CBC/PKCS5Padding \ No newline at end of file + For example: AES/CBC/PKCS5Padding + +com.bartek.esa.core.plugin.StrictModePlugin=Strict mode is turned on.\n\ + Strict mode was found in the file. Remember to delete it before publishing. \ No newline at end of file From 4bc66a75617ece2a876967f72f7536dde05958ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Sat, 6 Apr 2019 11:46:14 +0200 Subject: [PATCH 21/37] 10: Create StaticScopeHelper --- .../com/bartek/esa/core/di/CoreModule.java | 6 ++ .../com/bartek/esa/core/di/PluginModule.java | 13 +++-- .../esa/core/helper/StaticScopeHelper.java | 56 +++++++++++++++++++ .../esa/core/plugin/CipherInstancePlugin.java | 46 ++------------- .../bartek/esa/core/plugin/LoggingPlugin.java | 6 +- .../esa/core/plugin/StrictModePlugin.java | 46 ++------------- 6 files changed, 86 insertions(+), 87 deletions(-) create mode 100644 src/main/java/com/bartek/esa/core/helper/StaticScopeHelper.java diff --git a/src/main/java/com/bartek/esa/core/di/CoreModule.java b/src/main/java/com/bartek/esa/core/di/CoreModule.java index abb9f9d..af4a11a 100644 --- a/src/main/java/com/bartek/esa/core/di/CoreModule.java +++ b/src/main/java/com/bartek/esa/core/di/CoreModule.java @@ -2,6 +2,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.desc.provider.DescriptionProvider; import com.bartek.esa.core.executor.PluginExecutor; +import com.bartek.esa.core.helper.StaticScopeHelper; import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.xml.XmlHelper; import dagger.Module; @@ -29,4 +30,9 @@ public class CoreModule { public XmlHelper xmlHelper() { return new XmlHelper(); } + + @Provides + public StaticScopeHelper staticScopeHelper() { + return new StaticScopeHelper(); + } } diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 174f2bc..86d313c 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -1,6 +1,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.archetype.Plugin; +import com.bartek.esa.core.helper.StaticScopeHelper; import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.plugin.*; import com.bartek.esa.core.xml.XmlHelper; @@ -24,8 +25,8 @@ public class PluginModule { @Provides @IntoSet - public Plugin loggingPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { - return new LoggingPlugin(globMatcher, xmlHelper); + public Plugin loggingPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, StaticScopeHelper staticScopeHelper) { + return new LoggingPlugin(globMatcher, xmlHelper, staticScopeHelper); } @Provides @@ -72,13 +73,13 @@ public class PluginModule { @Provides @IntoSet - public Plugin cipherInstancePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { - return new CipherInstancePlugin(globMatcher, xmlHelper); + public Plugin cipherInstancePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, StaticScopeHelper staticScopeHelper) { + return new CipherInstancePlugin(globMatcher, xmlHelper, staticScopeHelper); } @Provides @IntoSet - public Plugin strictModePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { - return new StrictModePlugin(globMatcher, xmlHelper); + public Plugin strictModePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, StaticScopeHelper staticScopeHelper) { + return new StrictModePlugin(globMatcher, xmlHelper, staticScopeHelper); } } diff --git a/src/main/java/com/bartek/esa/core/helper/StaticScopeHelper.java b/src/main/java/com/bartek/esa/core/helper/StaticScopeHelper.java new file mode 100644 index 0000000..67c0951 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/helper/StaticScopeHelper.java @@ -0,0 +1,56 @@ +package com.bartek.esa.core.helper; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.ImportDeclaration; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.*; + +import javax.inject.Inject; +import java.util.Optional; +import java.util.function.Predicate; + +import static java.lang.String.format; + +public class StaticScopeHelper { + + @Inject + public StaticScopeHelper() { + + } + + public Predicate isFromScope(CompilationUnit compilationUnit, String methodName, String scope, String importScope) { + return expr -> { + boolean isFromScope = expr.getScope() + .filter(Expression::isNameExpr) + .map(Expression::asNameExpr) + .map(NameExpr::getName) + .map(SimpleName::getIdentifier) + .map(s -> s.equals(scope)) + .orElse(false); + + if(!isFromScope) { + isFromScope = compilationUnit.findAll(ImportDeclaration.class).stream() + .filter(ImportDeclaration::isStatic) + .filter(e -> e.getName().getIdentifier().matches(methodName)) + .map(ImportDeclaration::getName) + .map(Name::getQualifier) + .flatMap(Optional::stream) + .map(Node::toString) + .anyMatch(q -> q.equals(format("%s.%s", importScope, scope))); + } + + if(!isFromScope) { + isFromScope = compilationUnit.findAll(ImportDeclaration.class).stream() + .filter(ImportDeclaration::isStatic) + .filter(ImportDeclaration::isAsterisk) + .map(ImportDeclaration::getName) + .map(Name::getQualifier) + .flatMap(Optional::stream) + .map(Node::toString) + .anyMatch(q -> q.equals(importScope)); + } + + return isFromScope; + }; + } +} diff --git a/src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java b/src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java index 1ff6ee2..4567ced 100644 --- a/src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/CipherInstancePlugin.java @@ -1,70 +1,36 @@ package com.bartek.esa.core.plugin; import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.helper.StaticScopeHelper; import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.ImportDeclaration; -import com.github.javaparser.ast.Node; -import com.github.javaparser.ast.expr.*; +import com.github.javaparser.ast.expr.MethodCallExpr; import javax.inject.Inject; -import java.util.Optional; import java.util.regex.Pattern; public class CipherInstancePlugin extends JavaPlugin { private static final Pattern ALGORITHM_QUALIFIER = Pattern.compile("^\"\\w+/\\w+/\\w+\"$"); + private final StaticScopeHelper staticScopeHelper; @Inject - public CipherInstancePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + public CipherInstancePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, StaticScopeHelper staticScopeHelper) { super(globMatcher, xmlHelper); + this.staticScopeHelper = staticScopeHelper; } @Override public void run(CompilationUnit compilationUnit) { compilationUnit.findAll(MethodCallExpr.class).stream() .filter(expr -> expr.getName().getIdentifier().equals("getInstance")) - .filter(expr -> isCipherMethod(expr, compilationUnit)) + .filter(staticScopeHelper.isFromScope(compilationUnit, "getInstance", "Cipher", "javax.crypto")) .filter(expr -> expr.getArguments().isNonEmpty()) .filter(expr -> !isFullCipherQualifier(expr.getArguments().get(0).toString())) .forEach(expr -> addIssue(Severity.ERROR, getLineNumberFromExpression(expr), expr.toString())); } - private boolean isCipherMethod(MethodCallExpr expr, CompilationUnit compilationUnit) { - boolean isCipherMethod = expr.getScope() - .filter(Expression::isNameExpr) - .map(Expression::asNameExpr) - .map(NameExpr::getName) - .map(SimpleName::getIdentifier) - .filter(name -> name.equals("Cipher")) - .isPresent(); - - if(!isCipherMethod) { - isCipherMethod = compilationUnit.findAll(ImportDeclaration.class).stream() - .filter(ImportDeclaration::isStatic) - .filter(e -> e.getName().getIdentifier().equals("getInstance")) - .map(ImportDeclaration::getName) - .map(Name::getQualifier) - .flatMap(Optional::stream) - .map(Node::toString) - .anyMatch(q -> q.equals("javax.crypto.Cipher")); - } - - if(!isCipherMethod) { - isCipherMethod = compilationUnit.findAll(ImportDeclaration.class).stream() - .filter(ImportDeclaration::isStatic) - .filter(ImportDeclaration::isAsterisk) - .map(ImportDeclaration::getName) - .map(Name::getQualifier) - .flatMap(Optional::stream) - .map(Node::toString) - .anyMatch(q -> q.equals("javax.crypto")); - } - - return isCipherMethod; - } - private boolean isFullCipherQualifier(String qualifier) { return ALGORITHM_QUALIFIER.matcher(qualifier).matches(); } diff --git a/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java b/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java index 63b3392..5359fd1 100644 --- a/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/LoggingPlugin.java @@ -1,6 +1,7 @@ package com.bartek.esa.core.plugin; import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.helper.StaticScopeHelper; import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; @@ -10,16 +11,19 @@ import com.github.javaparser.ast.expr.MethodCallExpr; import javax.inject.Inject; public class LoggingPlugin extends JavaPlugin { + private final StaticScopeHelper staticScopeHelper; @Inject - public LoggingPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + public LoggingPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, StaticScopeHelper staticScopeHelper) { super(globMatcher, xmlHelper); + this.staticScopeHelper = staticScopeHelper; } @Override public void run(CompilationUnit compilationUnit) { compilationUnit.findAll(MethodCallExpr.class).stream() .filter(expr -> expr.getName().getIdentifier().matches("v|d|i|w|e|wtf")) + .filter(staticScopeHelper.isFromScope(compilationUnit, "v|d|i|w|e|wtf", "Log", "android.util")) .forEach(expr -> addIssue(Severity.INFO, getLineNumberFromExpression(expr), expr.toString())); } } diff --git a/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java b/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java index e31aac0..e8532f6 100644 --- a/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java @@ -1,63 +1,29 @@ package com.bartek.esa.core.plugin; import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.helper.StaticScopeHelper; import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.ImportDeclaration; -import com.github.javaparser.ast.Node; -import com.github.javaparser.ast.expr.*; +import com.github.javaparser.ast.expr.MethodCallExpr; import javax.inject.Inject; -import java.util.Optional; public class StrictModePlugin extends JavaPlugin { + private final StaticScopeHelper staticScopeHelper; @Inject - public StrictModePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + public StrictModePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, StaticScopeHelper staticScopeHelper) { super(globMatcher, xmlHelper); + this.staticScopeHelper = staticScopeHelper; } @Override public void run(CompilationUnit compilationUnit) { compilationUnit.findAll(MethodCallExpr.class).stream() .filter(expr -> expr.getName().getIdentifier().equals("setThreadPolicy")) - .filter(expr -> isStrictModeScope(expr, compilationUnit)) + .filter(staticScopeHelper.isFromScope(compilationUnit, "setThreadPolicy", "StrictMode", "android.os")) .forEach(expr -> addIssue(Severity.INFO, getLineNumberFromExpression(expr), expr.toString())); } - - private boolean isStrictModeScope(MethodCallExpr expr, CompilationUnit compilationUnit) { - boolean isStrictModeScope = expr.getScope() - .filter(Expression::isNameExpr) - .map(Expression::asNameExpr) - .map(NameExpr::getName) - .map(SimpleName::getIdentifier) - .map(s -> s.equals("StrictMode")) - .orElse(false); - - if(!isStrictModeScope) { - isStrictModeScope = compilationUnit.findAll(ImportDeclaration.class).stream() - .filter(ImportDeclaration::isStatic) - .filter(e -> e.getName().getIdentifier().equals("setThreadPolicy")) - .map(ImportDeclaration::getName) - .map(Name::getQualifier) - .flatMap(Optional::stream) - .map(Node::toString) - .anyMatch(q -> q.equals("android.os.StrictMode")); - } - - if(!isStrictModeScope) { - isStrictModeScope = compilationUnit.findAll(ImportDeclaration.class).stream() - .filter(ImportDeclaration::isStatic) - .filter(ImportDeclaration::isAsterisk) - .map(ImportDeclaration::getName) - .map(Name::getQualifier) - .flatMap(Optional::stream) - .map(Node::toString) - .anyMatch(q -> q.equals("android.os")); - } - - return isStrictModeScope; - } } From f5e8a161a1d0815d44882a0087f140aa11514216 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Sun, 7 Apr 2019 15:01:13 +0200 Subject: [PATCH 22/37] 10: Create ParentNodeFilter --- .../com/bartek/esa/core/di/CoreModule.java | 6 +++++ .../esa/core/helper/ParentNodeFinder.java | 25 +++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 src/main/java/com/bartek/esa/core/helper/ParentNodeFinder.java diff --git a/src/main/java/com/bartek/esa/core/di/CoreModule.java b/src/main/java/com/bartek/esa/core/di/CoreModule.java index af4a11a..8a9d141 100644 --- a/src/main/java/com/bartek/esa/core/di/CoreModule.java +++ b/src/main/java/com/bartek/esa/core/di/CoreModule.java @@ -2,6 +2,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.desc.provider.DescriptionProvider; import com.bartek.esa.core.executor.PluginExecutor; +import com.bartek.esa.core.helper.ParentNodeFinder; import com.bartek.esa.core.helper.StaticScopeHelper; import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.xml.XmlHelper; @@ -35,4 +36,9 @@ public class CoreModule { public StaticScopeHelper staticScopeHelper() { return new StaticScopeHelper(); } + + @Provides + public ParentNodeFinder parentNodeFinder() { + return new ParentNodeFinder(); + } } diff --git a/src/main/java/com/bartek/esa/core/helper/ParentNodeFinder.java b/src/main/java/com/bartek/esa/core/helper/ParentNodeFinder.java new file mode 100644 index 0000000..fef5131 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/helper/ParentNodeFinder.java @@ -0,0 +1,25 @@ +package com.bartek.esa.core.helper; + + +import com.github.javaparser.ast.Node; + +import javax.inject.Inject; +import java.util.Optional; + +public class ParentNodeFinder { + + @Inject + public ParentNodeFinder() { + + } + + public Optional findParentNode(Node child, Class nodeType) { + Node parent = child.getParentNode().orElse(null); + + while(parent != null && !parent.getClass().equals(nodeType)) { + parent = parent.getParentNode().orElse(null); + } + + return Optional.ofNullable(parent).map(nodeType::cast); + } +} From 44608456eb34afb7bd03b369869775ab28d94349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Tue, 9 Apr 2019 14:48:10 +0200 Subject: [PATCH 23/37] 10: Create ExternalStoragePlugin --- .../com/bartek/esa/core/di/PluginModule.java | 7 +++ .../core/plugin/ExternalStoragePlugin.java | 44 +++++++++++++++++++ src/main/resources/description.properties | 6 ++- 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/ExternalStoragePlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 86d313c..b1e574c 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -1,6 +1,7 @@ package com.bartek.esa.core.di; import com.bartek.esa.core.archetype.Plugin; +import com.bartek.esa.core.helper.ParentNodeFinder; import com.bartek.esa.core.helper.StaticScopeHelper; import com.bartek.esa.core.java.JavaSyntaxRegexProvider; import com.bartek.esa.core.plugin.*; @@ -82,4 +83,10 @@ public class PluginModule { public Plugin strictModePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, StaticScopeHelper staticScopeHelper) { return new StrictModePlugin(globMatcher, xmlHelper, staticScopeHelper); } + + @Provides + @IntoSet + public Plugin externalStoragePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, ParentNodeFinder parentNodeFinder) { + return new ExternalStoragePlugin(globMatcher, xmlHelper, parentNodeFinder); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/ExternalStoragePlugin.java b/src/main/java/com/bartek/esa/core/plugin/ExternalStoragePlugin.java new file mode 100644 index 0000000..2241c37 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/ExternalStoragePlugin.java @@ -0,0 +1,44 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.helper.ParentNodeFinder; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.body.MethodDeclaration; +import com.github.javaparser.ast.expr.MethodCallExpr; + +import javax.inject.Inject; + +public class ExternalStoragePlugin extends JavaPlugin { + private final ParentNodeFinder parentNodeFinder; + + @Inject + public ExternalStoragePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, ParentNodeFinder parentNodeFinder) { + super(globMatcher, xmlHelper); + this.parentNodeFinder = parentNodeFinder; + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("getExternalStorageDirectory|getExternalStoragePublicDirectory")) + .forEach(this::findCheckingStorageStateForAccessingExternalStorage); + } + + private void findCheckingStorageStateForAccessingExternalStorage(MethodCallExpr accessingToExternalStorage) { + parentNodeFinder.findParentNode(accessingToExternalStorage, MethodDeclaration.class).ifPresent(methodDeclaration -> + findCheckingStorageStateInMethodDeclaration(accessingToExternalStorage, methodDeclaration) + ); + } + + private void findCheckingStorageStateInMethodDeclaration(MethodCallExpr accessingToExternalStorage, MethodDeclaration methodDeclaration) { + boolean isStateBeingChecked = methodDeclaration.findAll(MethodCallExpr.class).stream() + .anyMatch(e -> e.getName().getIdentifier().equals("getExternalStorageState")); + + if (!isStateBeingChecked) { + addIssue(Severity.WARNING, getLineNumberFromExpression(accessingToExternalStorage), accessingToExternalStorage.toString()); + } + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 8424a59..5d53f9a 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -78,4 +78,8 @@ com.bartek.esa.core.plugin.CipherInstancePlugin=Not fully-qualified algorithm na For example: AES/CBC/PKCS5Padding com.bartek.esa.core.plugin.StrictModePlugin=Strict mode is turned on.\n\ - Strict mode was found in the file. Remember to delete it before publishing. \ No newline at end of file + Strict mode was found in the file. Remember to delete it before publishing. + +com.bartek.esa.core.plugin.ExternalStoragePlugin=External storage state is not checked.\n\ + There is attempt to access to external storage without checking its state.\n\ + External storage state should be checked through 'Environment.getExternalStorageState()' method. \ No newline at end of file From 888030ea4f862dbae121e4f7e6294ac8a139ed52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Tue, 9 Apr 2019 15:10:47 +0200 Subject: [PATCH 24/37] 10: Create SuppressWarningsPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++++ .../core/plugin/SuppressWarningsPlugin.java | 25 +++++++++++++++++++ src/main/resources/description.properties | 6 ++++- 3 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/SuppressWarningsPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index b1e574c..a549835 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -89,4 +89,10 @@ public class PluginModule { public Plugin externalStoragePlugin(GlobMatcher globMatcher, XmlHelper xmlHelper, ParentNodeFinder parentNodeFinder) { return new ExternalStoragePlugin(globMatcher, xmlHelper, parentNodeFinder); } + + @Provides + @IntoSet + public Plugin suppressWarningsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new SuppressWarningsPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/SuppressWarningsPlugin.java b/src/main/java/com/bartek/esa/core/plugin/SuppressWarningsPlugin.java new file mode 100644 index 0000000..ad4f7fc --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/SuppressWarningsPlugin.java @@ -0,0 +1,25 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.AnnotationExpr; + +import javax.inject.Inject; + +public class SuppressWarningsPlugin extends JavaPlugin { + + @Inject + public SuppressWarningsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(AnnotationExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().equals("SuppressWarnings")) + .forEach(expr -> addIssue(Severity.WARNING, getLineNumberFromExpression(expr), expr.toString())); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 5d53f9a..0bd1c8d 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -82,4 +82,8 @@ com.bartek.esa.core.plugin.StrictModePlugin=Strict mode is turned on.\n\ com.bartek.esa.core.plugin.ExternalStoragePlugin=External storage state is not checked.\n\ There is attempt to access to external storage without checking its state.\n\ - External storage state should be checked through 'Environment.getExternalStorageState()' method. \ No newline at end of file + External storage state should be checked through 'Environment.getExternalStorageState()' method. + +com.bartek.esa.core.plugin.SuppressWarningsPlugin=@SuppressWarnings annotation was found.\n\ + The @SuppressWarnings annotation might be hiding useful warnings.\n\ + Consider removing it. \ No newline at end of file From f7f0a2b2c682fc217c3c7d6022cbcfe34234d88f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Wed, 10 Apr 2019 08:52:34 +0200 Subject: [PATCH 25/37] 10: Create ExportedComponentsPlugin --- .../bartek/esa/core/archetype/XmlPlugin.java | 24 +++++++ .../com/bartek/esa/core/di/PluginModule.java | 6 ++ .../core/plugin/ExportedComponentsPlugin.java | 72 +++++++++++++++++++ .../com/bartek/esa/core/xml/XmlHelper.java | 13 ++++ src/main/resources/description.properties | 22 +++++- 5 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java diff --git a/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java b/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java index c1bbedd..19b41a0 100644 --- a/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/XmlPlugin.java @@ -3,9 +3,16 @@ package com.bartek.esa.core.archetype; import com.bartek.esa.core.xml.XmlHelper; import com.bartek.esa.file.matcher.GlobMatcher; import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; import javax.xml.namespace.QName; import java.io.File; +import java.util.Arrays; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static java.lang.String.format; public abstract class XmlPlugin extends BasePlugin { private final GlobMatcher globMatcher; @@ -32,4 +39,21 @@ public abstract class XmlPlugin extends BasePlugin { protected Object xPath(Document xml, String expression, QName returnType) { return xmlHelper.xPath(xml, expression, returnType); } + + protected Stream stream(NodeList nodeList) { + return xmlHelper.stream(nodeList); + } + + protected String tagString(Node node) { + Node[] attributes = new Node[node.getAttributes().getLength()]; + for(int i=0; i format("%s=\"%s\"", n.getNodeName(), n.getNodeValue())) + .collect(Collectors.joining(" ")); + + return format("<%s %s ...", node.getNodeName(), attributesString); + } } diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index a549835..e99daca 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -95,4 +95,10 @@ public class PluginModule { public Plugin suppressWarningsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new SuppressWarningsPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin exportedComponentsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new ExportedComponentsPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java b/src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java new file mode 100644 index 0000000..ed3e96b --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java @@ -0,0 +1,72 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +import javax.inject.Inject; +import javax.xml.xpath.XPathConstants; +import java.util.Optional; + +import static java.lang.String.format; + +public class ExportedComponentsPlugin extends AndroidManifestPlugin { + + @Inject + public ExportedComponentsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + findExportedComponents(xml, "activity"); + findExportedComponents(xml, "service"); + findExportedComponents(xml, "receiver"); + findExportedProviders(xml); + } + + private void findExportedComponents(Document xml, String component) { + NodeList exportedActivities = (NodeList) xPath(xml, format("/manifest/application/%s", component), XPathConstants.NODESET); + stream(exportedActivities) + .filter(this::isExported) + .filter(node -> doesNotHavePermission(node, "android:permission")) + .forEach(node -> addIssue(Severity.WARNING, format(".%s.NO_PERMISSION", component.toUpperCase()), null, tagString(node))); + } + + private void findExportedProviders(Document xml) { + NodeList exportedProviders = (NodeList) xPath(xml, "/manifest/application/provider", XPathConstants.NODESET); + stream(exportedProviders) + .filter(this::isExported) + .filter(node -> doesNotHavePermission(node, "android:writePermission") + || doesNotHavePermission(node, "android:readPermission")) + .forEach(node -> addIssue(Severity.WARNING, ".PROVIDER.NO_PERMISSION", null, tagString(node))); + } + + private boolean doesNotHavePermission(Node node, String permissionAttribute) { + Boolean doesHavePermission = Optional.ofNullable(node.getAttributes().getNamedItem(permissionAttribute)) + .map(Node::getNodeValue) + .map(s -> !s.isEmpty()) + .orElse(false); + return !doesHavePermission; + } + + private boolean isExported(Node node) { + return Optional.ofNullable(node.getAttributes().getNamedItem("android:exported")) + .map(Node::getNodeValue) + .map(v -> v.equals("true")) + .orElse(false); + } + + private String nodeToString(Node node) { + String nodeName = Optional.ofNullable(node.getAttributes().getNamedItem("android:name")) + .map(Node::getNodeValue) + .map(name -> format(" android:name=\"%s\"", name)) + .orElse(""); + + return format("<%s%s ...", node.getNodeName(), nodeName); + } +} diff --git a/src/main/java/com/bartek/esa/core/xml/XmlHelper.java b/src/main/java/com/bartek/esa/core/xml/XmlHelper.java index 7a9ceee..f533431 100644 --- a/src/main/java/com/bartek/esa/core/xml/XmlHelper.java +++ b/src/main/java/com/bartek/esa/core/xml/XmlHelper.java @@ -3,6 +3,8 @@ package com.bartek.esa.core.xml; import com.bartek.esa.error.EsaException; import io.vavr.control.Try; import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; import javax.inject.Inject; import javax.xml.namespace.QName; @@ -10,6 +12,8 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.xpath.XPathFactory; import java.io.File; +import java.util.Arrays; +import java.util.stream.Stream; public class XmlHelper { @@ -28,4 +32,13 @@ public class XmlHelper { return Try.of(() -> XPathFactory.newDefaultInstance().newXPath().evaluate(expression, xml, returnType)) .getOrElseThrow(EsaException::new); } + + public Stream stream(NodeList nodeList) { + Node[] nodes = new Node[nodeList.getLength()]; + for (int i=0; i Date: Wed, 10 Apr 2019 14:25:25 +0200 Subject: [PATCH 26/37] 10: Create DangerousPermissionPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++ .../plugin/DangerousPermissionPlugin.java | 47 +++++++++++++++++++ src/main/resources/description.properties | 5 ++ 3 files changed, 58 insertions(+) create mode 100644 src/main/java/com/bartek/esa/core/plugin/DangerousPermissionPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index e99daca..2786ae4 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -101,4 +101,10 @@ public class PluginModule { public Plugin exportedComponentsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new ExportedComponentsPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin dangerousPermissionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new DangerousPermissionPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/DangerousPermissionPlugin.java b/src/main/java/com/bartek/esa/core/plugin/DangerousPermissionPlugin.java new file mode 100644 index 0000000..381a09b --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/DangerousPermissionPlugin.java @@ -0,0 +1,47 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +import javax.inject.Inject; +import javax.xml.xpath.XPathConstants; +import java.util.Optional; + +public class DangerousPermissionPlugin extends AndroidManifestPlugin { + + @Inject + public DangerousPermissionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + NodeList customPermissions = (NodeList) xPath(xml, "/manifest/permission", XPathConstants.NODESET); + stream(customPermissions) + .filter(this::isDangerousPermission) + .filter(this::doesNotHaveDescription) + .forEach(permission -> addIssue(Severity.WARNING, null, tagString(permission))); + } + + private boolean isDangerousPermission(Node permission) { + return Optional.ofNullable(permission.getAttributes().getNamedItem("android:protectionLevel")) + .map(Node::getNodeValue) + .map(v -> v.equals("dangerous")) + .orElse(false); + } + + private boolean doesNotHaveDescription(Node permission) { + Boolean doesHaveDescription = Optional.ofNullable(permission.getAttributes().getNamedItem("android:description")) + .map(Node::getNodeValue) + .map(v -> !v.isEmpty()) + .orElse(false); + + return !doesHaveDescription; + } + +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index a8cd05a..b73dbc1 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -107,3 +107,8 @@ com.bartek.esa.core.plugin.ExportedComponentsPlugin.PROVIDER.NO_PERMISSION=Expor The content provider is exported but not protected by any permissions. \n\ It means any malicious application could make use of data provided by the component and/or insert some new data. \n\ Consider using 'android:readPermission' and 'android:writePermission' tags and adding custom permission to protect it. + +com.bartek.esa.core.plugin.DangerousPermissionPlugin=Custom permission without description.\n\ + Custom permission with 'dangerous' protection level was found and it doesn't have any description.\n\ + As long as the permission requires user attention, he should have provided a meaningful description about\n\ + permission. From 40e50cb0f4da1b140a514604c4c78f19c434bc20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Wed, 10 Apr 2019 14:35:47 +0200 Subject: [PATCH 27/37] 10: Create TextInputValidationPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 ++++ .../plugin/TextInputValidationPlugin.java | 36 +++++++++++++++++++ src/main/resources/description.properties | 5 +++ 3 files changed, 47 insertions(+) create mode 100644 src/main/java/com/bartek/esa/core/plugin/TextInputValidationPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 2786ae4..5a220e9 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -107,4 +107,10 @@ public class PluginModule { public Plugin dangerousPermissionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new DangerousPermissionPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin textInputValidationPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new TextInputValidationPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/TextInputValidationPlugin.java b/src/main/java/com/bartek/esa/core/plugin/TextInputValidationPlugin.java new file mode 100644 index 0000000..f9c5054 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/TextInputValidationPlugin.java @@ -0,0 +1,36 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.ResourceLayoutPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +import javax.inject.Inject; +import java.util.Optional; + +public class TextInputValidationPlugin extends ResourceLayoutPlugin { + + @Inject + public TextInputValidationPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + NodeList editTextNodes = xml.getElementsByTagName("EditText"); + stream(editTextNodes) + .filter(this::doesNotHaveInputType) + .forEach(n -> addIssue(Severity.WARNING, null, tagString(n))); + } + + private boolean doesNotHaveInputType(Node editText) { + Boolean doesHaveInputType = Optional.ofNullable(editText.getAttributes().getNamedItem("android:inputType")) + .map(Node::getNodeValue) + .map(v -> !v.isEmpty()) + .orElse(false); + return !doesHaveInputType; + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index b73dbc1..f55f748 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -112,3 +112,8 @@ com.bartek.esa.core.plugin.DangerousPermissionPlugin=Custom permission without d Custom permission with 'dangerous' protection level was found and it doesn't have any description.\n\ As long as the permission requires user attention, he should have provided a meaningful description about\n\ permission. + +com.bartek.esa.core.plugin.TextInputValidationPlugin=Input type is no selected.\n\ + The EditText view doesn't have a input type selected.\n\ + Consider associating a input type with this view.\n\ + For example: Date: Wed, 10 Apr 2019 16:08:16 +0200 Subject: [PATCH 28/37] 10: Create IntentFilterPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++ .../esa/core/plugin/IntentFilterPlugin.java | 43 +++++++++++++++++++ src/main/resources/description.properties | 5 +++ 3 files changed, 54 insertions(+) create mode 100644 src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 5a220e9..4a81811 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -113,4 +113,10 @@ public class PluginModule { public Plugin textInputValidationPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new TextInputValidationPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin intentFilterPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new IntentFilterPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java b/src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java new file mode 100644 index 0000000..237c803 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java @@ -0,0 +1,43 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.AndroidManifestPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +import javax.inject.Inject; + +public class IntentFilterPlugin extends AndroidManifestPlugin { + + @Inject + public IntentFilterPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + protected void run(Document xml) { + NodeList filters = xml.getElementsByTagName("intent-filter"); + stream(filters) + .filter(this::isNotMainActivity) + .map(Node::getParentNode) + .forEach(n -> addIssue(Severity.INFO, null, tagString(n))); + } + + private boolean isNotMainActivity(Node filter) { + long mainActivityIntentFilters = stream(filter.getChildNodes()) + .filter(n -> n.getNodeName().matches("action|category")) + .map(n -> n.getAttributes().getNamedItem("android:name")) + .map(Node::getNodeValue) + .filter(v -> v.equals("android.intent.action.MAIN") || v.equals("android.intent.category.LAUNCHER")) + .count(); + + long currentIntentFilters = stream(filter.getChildNodes()) + .filter(n -> n.getNodeName().matches("action|category")) + .count(); + + return mainActivityIntentFilters != currentIntentFilters; + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index f55f748..217ea56 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -117,3 +117,8 @@ com.bartek.esa.core.plugin.TextInputValidationPlugin=Input type is no selected.\ The EditText view doesn't have a input type selected.\n\ Consider associating a input type with this view.\n\ For example: Date: Wed, 10 Apr 2019 16:17:02 +0200 Subject: [PATCH 29/37] 10: Create SqlInjectionPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++++ .../esa/core/plugin/SqlInjectionPlugin.java | 25 +++++++++++++++++++ src/main/resources/description.properties | 3 +++ 3 files changed, 34 insertions(+) create mode 100644 src/main/java/com/bartek/esa/core/plugin/SqlInjectionPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 4a81811..b4d75e3 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -119,4 +119,10 @@ public class PluginModule { public Plugin intentFilterPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new IntentFilterPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin sqlInjectionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new SqlInjectionPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/SqlInjectionPlugin.java b/src/main/java/com/bartek/esa/core/plugin/SqlInjectionPlugin.java new file mode 100644 index 0000000..2e9ab6b --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/SqlInjectionPlugin.java @@ -0,0 +1,25 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.MethodCallExpr; + +import javax.inject.Inject; + +public class SqlInjectionPlugin extends JavaPlugin { + + @Inject + public SqlInjectionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().equals("rawQuery")) + .forEach(expr -> addIssue(Severity.VULNERABILITY, getLineNumberFromExpression(expr), expr.toString())); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 217ea56..bba2b77 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -122,3 +122,6 @@ com.bartek.esa.core.plugin.IntentFilterPlugin=Implemented intent filter.\n\ Component with intent filter was found. It means, that the component is implicitly exposed to public.\n\ Consider removing intent filter.\n\ Also be aware, that intent filter is not a security tool. It can be easily omitted. + +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. \ No newline at end of file From ce7dd864c719d07c630803fc21e3a4f03fc10739 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Wed, 10 Apr 2019 20:01:07 +0200 Subject: [PATCH 30/37] 10: Update plugins to make use of issue's description model --- .../core/plugin/ExportedComponentsPlugin.java | 12 +++++++-- .../esa/core/plugin/IntentFilterPlugin.java | 10 ++++++- .../PermissionsRaceConditionPlugin.java | 7 ++++- .../bartek/esa/core/plugin/UsesSdkPlugin.java | 3 ++- src/main/resources/description.properties | 26 +++++-------------- 5 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java b/src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java index ed3e96b..01410fa 100644 --- a/src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/ExportedComponentsPlugin.java @@ -10,6 +10,7 @@ import org.w3c.dom.NodeList; import javax.inject.Inject; import javax.xml.xpath.XPathConstants; +import java.util.Map; import java.util.Optional; import static java.lang.String.format; @@ -34,7 +35,14 @@ public class ExportedComponentsPlugin extends AndroidManifestPlugin { stream(exportedActivities) .filter(this::isExported) .filter(node -> doesNotHavePermission(node, "android:permission")) - .forEach(node -> addIssue(Severity.WARNING, format(".%s.NO_PERMISSION", component.toUpperCase()), null, tagString(node))); + .forEach(node -> addIssue(Severity.WARNING, ".NO_PERMISSION", getModel(node), null, null)); + } + + private Map getModel(Node node) { + return Map.of( + "componentName", node.getAttributes().getNamedItem("android:name").getNodeValue(), + "componentType", node.getNodeName() + ); } private void findExportedProviders(Document xml) { @@ -43,7 +51,7 @@ public class ExportedComponentsPlugin extends AndroidManifestPlugin { .filter(this::isExported) .filter(node -> doesNotHavePermission(node, "android:writePermission") || doesNotHavePermission(node, "android:readPermission")) - .forEach(node -> addIssue(Severity.WARNING, ".PROVIDER.NO_PERMISSION", null, tagString(node))); + .forEach(node -> addIssue(Severity.WARNING, ".NO_PERMISSION", getModel(node), null, null)); } private boolean doesNotHavePermission(Node node, String permissionAttribute) { diff --git a/src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java b/src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java index 237c803..3272674 100644 --- a/src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/IntentFilterPlugin.java @@ -9,6 +9,7 @@ import org.w3c.dom.Node; import org.w3c.dom.NodeList; import javax.inject.Inject; +import java.util.Map; public class IntentFilterPlugin extends AndroidManifestPlugin { @@ -23,7 +24,14 @@ public class IntentFilterPlugin extends AndroidManifestPlugin { stream(filters) .filter(this::isNotMainActivity) .map(Node::getParentNode) - .forEach(n -> addIssue(Severity.INFO, null, tagString(n))); + .forEach(n -> addIssue(Severity.INFO, getModel(n), null, null)); + } + + private Map getModel(Node node) { + return Map.of( + "componentType", node.getNodeName(), + "componentName", node.getAttributes().getNamedItem("android:name").getNodeValue() + ); } private boolean isNotMainActivity(Node filter) { diff --git a/src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java b/src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java index f1e3478..138521c 100644 --- a/src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/PermissionsRaceConditionPlugin.java @@ -10,6 +10,7 @@ import org.w3c.dom.NodeList; import javax.inject.Inject; import javax.xml.xpath.XPathConstants; +import java.util.Map; import static java.lang.Integer.parseInt; @@ -28,8 +29,12 @@ public class PermissionsRaceConditionPlugin extends AndroidManifestPlugin { Node minSdkVersionNode = usesSdkNode.getAttributes().getNamedItem("android:minSdkVersion"); int minSdkVersion = parseInt(minSdkVersionNode.getNodeValue()); if(minSdkVersion < 21) { - addIssue(Severity.VULNERABILITY, null, minSdkVersionNode.toString()); + addIssue(Severity.VULNERABILITY, getModel(minSdkVersion), null, minSdkVersionNode.toString()); } } } + + private Map getModel(int minSdkVersion) { + return Map.of("minSdkVersion", Integer.toString(minSdkVersion)); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java b/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java index 9ac43e9..b65a4f2 100644 --- a/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/UsesSdkPlugin.java @@ -9,6 +9,7 @@ import org.w3c.dom.Node; import javax.inject.Inject; import javax.xml.xpath.XPathConstants; +import java.util.Map; import java.util.Optional; public class UsesSdkPlugin extends AndroidManifestPlugin { @@ -26,7 +27,7 @@ public class UsesSdkPlugin extends AndroidManifestPlugin { } Optional.ofNullable(usesSdkNode.getAttributes().getNamedItem("android:maxSdkVersion")).ifPresent(maxSdkVersion -> - addIssue(Severity.ERROR, ".USES_SDK.MAX_SDK_VERSION", null, maxSdkVersion.toString()) + addIssue(Severity.ERROR, ".USES_SDK.MAX_SDK_VERSION", Map.of("maxSdkVersion", maxSdkVersion.getNodeValue()),null, maxSdkVersion.toString()) ); }, () -> addIssue(Severity.ERROR, ".NO_USES_SDK", null, null)); } diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index bba2b77..5d67f0b 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -30,7 +30,7 @@ This will allow accessing the backups via adb if device has USB debugging enable Consider setting it to 'false'. com.bartek.esa.core.plugin.PermissionsRaceConditionPlugin=Potential permissions race condition vulnerability. \n\ - There are declared custom permissions in AndroidManifest.xml and the minimal API version is set to less than 21.\n\ + There are declared custom permissions in AndroidManifest.xml and the minimal API version is set to ${minSdkVersion} that is less than 21.\n\ It means that declared permissions can be obtained by malicious application installed before and without need of having 1proper signature.\n\ Consider setting minimal API version to 21 at least. @@ -66,7 +66,7 @@ com.bartek.esa.core.plugin.UsesSdkPlugin.USES_SDK.NO_MIN_SDK_VERSION=There is no For example: com.bartek.esa.core.plugin.UsesSdkPlugin.USES_SDK.MAX_SDK_VERSION=Application defines an upper limit for API version.\n\ - The android:maxSdkVersion is defined in AndroidManifest.xml.\n\ + The android:maxSdkVersion is set to ${maxSdkVersion} in AndroidManifest.xml.\n\ There is no need to limit available platforms for application.\n\ Furthermore it can cause unexpected application uninstall\n\ on upgrading Android version (along with API which can exceed defined maximal API version). @@ -88,26 +88,11 @@ com.bartek.esa.core.plugin.SuppressWarningsPlugin=@SuppressWarnings annotation w The @SuppressWarnings annotation might be hiding useful warnings.\n\ Consider removing it. -com.bartek.esa.core.plugin.ExportedComponentsPlugin.ACTIVITY.NO_PERMISSION=Exported activity.\n\ - The activity is exported but not protected by any permission. \n\ +com.bartek.esa.core.plugin.ExportedComponentsPlugin.NO_PERMISSION=Exported activity.\n\ + The ${componentType} with name '${componentName}' is exported but not protected by any permission. \n\ It means any malicious application could make use of the component. \n\ Consider using 'android:permission' tag and adding custom permission to protect it. -com.bartek.esa.core.plugin.ExportedComponentsPlugin.SERVICE.NO_PERMISSION=Exported service.\n\ - The service is exported but not protected by any permission. \n\ - It means any malicious application could make use of the component. \n\ - Consider using 'android:permission' tag and adding custom permission to protect it. - -com.bartek.esa.core.plugin.ExportedComponentsPlugin.RECEIVER.NO_PERMISSION=Exported broadcast receiver.\n\ - The broadcast receiver is exported but not protected by any permission. \n\ - It means any malicious application could make use of the component. \n\ - Consider using 'android:permission' tag and adding custom permission to protect it. - -com.bartek.esa.core.plugin.ExportedComponentsPlugin.PROVIDER.NO_PERMISSION=Exported content provider. Potential data leakage.\n\ - The content provider is exported but not protected by any permissions. \n\ - It means any malicious application could make use of data provided by the component and/or insert some new data. \n\ - Consider using 'android:readPermission' and 'android:writePermission' tags and adding custom permission to protect it. - com.bartek.esa.core.plugin.DangerousPermissionPlugin=Custom permission without description.\n\ Custom permission with 'dangerous' protection level was found and it doesn't have any description.\n\ As long as the permission requires user attention, he should have provided a meaningful description about\n\ @@ -119,7 +104,8 @@ com.bartek.esa.core.plugin.TextInputValidationPlugin=Input type is no selected.\ For example: Date: Thu, 11 Apr 2019 17:42:18 +0200 Subject: [PATCH 31/37] 10: Fix interrupting executing on parse error --- .../bartek/esa/core/archetype/JavaPlugin.java | 32 ++++++++++++++++--- .../esa/core/executor/PluginExecutor.java | 6 ++-- .../bartek/esa/core/model/object/Issue.java | 3 +- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java index 030587f..f7f31a4 100644 --- a/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java +++ b/src/main/java/com/bartek/esa/core/archetype/JavaPlugin.java @@ -3,12 +3,13 @@ package com.bartek.esa.core.archetype; import com.bartek.esa.core.model.enumeration.Severity; import com.bartek.esa.core.model.object.Issue; import com.bartek.esa.core.xml.XmlHelper; -import com.bartek.esa.error.EsaException; import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ParseProblemException; +import com.github.javaparser.Problem; import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.TokenRange; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.expr.Expression; -import io.vavr.control.Try; import org.w3c.dom.Document; import org.w3c.dom.Node; @@ -16,6 +17,8 @@ import javax.xml.xpath.XPathConstants; import java.io.File; import java.util.HashMap; +import static java.lang.String.format; + public abstract class JavaPlugin extends BasePlugin { private final GlobMatcher globMatcher; private final XmlHelper xmlHelper; @@ -36,8 +39,27 @@ public abstract class JavaPlugin extends BasePlugin { return; } - CompilationUnit compilationUnit = Try.of(() -> StaticJavaParser.parse(file)).getOrElseThrow(EsaException::new); - run(compilationUnit); + try { + CompilationUnit compilationUnit = StaticJavaParser.parse(file); + run(compilationUnit); + } catch (ParseProblemException e) { + printParsingErrorToStderr(e, file); + } catch (Exception e) { + e.printStackTrace(); + } + } + + private void printParsingErrorToStderr(ParseProblemException e, File file) { + e.getProblems().stream() + .map(p -> format("%s%s:\n%s\nIgnoring file...\n", file.getAbsolutePath(), getRange(p), p.getMessage())) + .forEach(System.err::println); + } + + private String getRange(Problem problem) { + return problem.getLocation() + .flatMap(TokenRange::toRange) + .map(range -> format(" (line %d, col %d)", range.begin.line, range.begin.column)) + .orElse(""); } private boolean isApplicationPackageFile(File file) { @@ -59,7 +81,7 @@ public abstract class JavaPlugin extends BasePlugin { } String path = packageValue.getNodeValue().replaceAll("\\.", "/"); - return globMatcher.fileMatchesGlobPattern(file, String.format("**/%s/**", path)); + return globMatcher.fileMatchesGlobPattern(file, format("**/%s/**", path)); } protected Integer getLineNumberFromExpression(Expression expression) { diff --git a/src/main/java/com/bartek/esa/core/executor/PluginExecutor.java b/src/main/java/com/bartek/esa/core/executor/PluginExecutor.java index 4c86bf7..d0cd00a 100644 --- a/src/main/java/com/bartek/esa/core/executor/PluginExecutor.java +++ b/src/main/java/com/bartek/esa/core/executor/PluginExecutor.java @@ -21,7 +21,7 @@ public class PluginExecutor { public Set executeForFiles(File manifest, Set files, Set plugins, boolean debug) { return files.stream() - .peek(file -> { if(debug) System.out.printf("File: %s", file.getAbsolutePath()); }) + .peek(file -> { if(debug) System.out.printf("File: %s\n", file.getAbsolutePath()); }) .map(file -> executeForFile(manifest, file, plugins, debug)) .flatMap(Set::stream) .collect(toSet()); @@ -29,8 +29,8 @@ public class PluginExecutor { private Set executeForFile(File manifest, File file, Set plugins, boolean debug) { Document xmlManifest = xmlHelper.parseXml(manifest); - return plugins.parallelStream() - .peek(plugin -> { if(debug) System.out.printf(" Plugin: %s", plugin.getClass().getCanonicalName()); }) + return plugins.stream() + .peek(plugin -> { if(debug) System.out.printf(" Plugin: %s\n", plugin.getClass().getCanonicalName()); }) .peek(plugin -> plugin.update(file, xmlManifest)) .filter(plugin -> plugin.supports(file)) .map(Plugin::runForIssues) diff --git a/src/main/java/com/bartek/esa/core/model/object/Issue.java b/src/main/java/com/bartek/esa/core/model/object/Issue.java index 1fb713b..355dfcb 100644 --- a/src/main/java/com/bartek/esa/core/model/object/Issue.java +++ b/src/main/java/com/bartek/esa/core/model/object/Issue.java @@ -6,6 +6,7 @@ import lombok.Data; import java.io.File; import java.util.Map; +import java.util.Optional; @Data @Builder @@ -27,6 +28,6 @@ public class Issue implements Comparable { return compByFile; } - return lineNumber - another.lineNumber; + return Optional.ofNullable(lineNumber).orElse(0) - Optional.ofNullable(another.lineNumber).orElse(0); } } From ad078edc47c4f5bfa1a546fcdacb9b5138611e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Thu, 11 Apr 2019 18:29:26 +0200 Subject: [PATCH 32/37] 10: Improve ImplicitIntentsPlugin --- .../com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java b/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java index aca4bf6..92b8f4d 100644 --- a/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/ImplicitIntentsPlugin.java @@ -95,8 +95,12 @@ public class ImplicitIntentsPlugin extends JavaPlugin { // Not works for: new Intent(this, ...) if(arguments.size() == 2) { - Expression argument = arguments.get(0); - isImplicit = !argument.isThisExpr(); + Expression firstArg = arguments.get(0); + Expression secondArg = arguments.get(1); + boolean isThisExpr = firstArg.isThisExpr(); + boolean isTryingToGetClass = secondArg.isClassExpr(); + boolean isExplicit = isThisExpr || isTryingToGetClass; + isImplicit = !isExplicit; } return isImplicit; From c0c1577f1ceadb2156a2135112e778626327c451 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 12 Apr 2019 10:51:28 +0200 Subject: [PATCH 33/37] 10: Add WorldAccessPermissionsPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++ .../plugin/WorldAccessPermissionsPlugin.java | 39 +++++++++++++++++++ src/main/resources/description.properties | 7 +++- 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/WorldAccessPermissionsPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index b4d75e3..cb9e906 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -125,4 +125,10 @@ public class PluginModule { public Plugin sqlInjectionPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new SqlInjectionPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin worldAccessPermissionsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new WorldAccessPermissionsPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/WorldAccessPermissionsPlugin.java b/src/main/java/com/bartek/esa/core/plugin/WorldAccessPermissionsPlugin.java new file mode 100644 index 0000000..fb1c8d1 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/WorldAccessPermissionsPlugin.java @@ -0,0 +1,39 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.FieldAccessExpr; +import com.github.javaparser.ast.expr.NameExpr; + +import javax.inject.Inject; +import java.util.Map; + +public class WorldAccessPermissionsPlugin extends JavaPlugin { + + @Inject + public WorldAccessPermissionsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(NameExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("MODE_WORLD_(READABLE|WRITEABLE)")) + .forEach(expr -> addIssue(Severity.ERROR, getModel(expr), getLineNumberFromExpression(expr), expr.toString())); + + compilationUnit.findAll(FieldAccessExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("MODE_WORLD_(READABLE|WRITEABLE)")) + .forEach(expr -> addIssue(Severity.ERROR, getModel(expr), getLineNumberFromExpression(expr), expr.toString())); + } + + private Map getModel(NameExpr expression) { + return Map.of("exprName", expression.getName().getIdentifier()); + } + + private Map getModel(FieldAccessExpr expression) { + return Map.of("exprName", expression.getName().getIdentifier()); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 5d67f0b..a9057cb 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -110,4 +110,9 @@ com.bartek.esa.core.plugin.IntentFilterPlugin=Implemented intent filter.\n\ Also be aware, that intent filter is not a security tool. It can be easily omitted. 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. \ No newline at end of file + 'rawQuery' method should be avoided because of possibility to inject SQL code. + +com.bartek.esa.core.plugin.WorldAccessPermissionsPlugin=World access permissions detected. Potential data leakage.\n\ + The deprecated '${exprName}' constant has been found and it can be risky to use.\n\ + It grants world access permission to selected resource.\n\ + Consider using less permissive mode.a. \ No newline at end of file From 6ecdafac87e63c7a2c2f281364d52b490ccc3804 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Fri, 12 Apr 2019 10:51:49 +0200 Subject: [PATCH 34/37] 10: Add OrderedBroadcastPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++++ .../core/plugin/OrderedBroadcastPlugin.java | 25 +++++++++++++++++++ src/main/resources/description.properties | 5 +++- 3 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/OrderedBroadcastPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index cb9e906..1248dbd 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -131,4 +131,10 @@ public class PluginModule { public Plugin worldAccessPermissionsPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new WorldAccessPermissionsPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin orderedAndStickyBroadcastPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new OrderedBroadcastPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/OrderedBroadcastPlugin.java b/src/main/java/com/bartek/esa/core/plugin/OrderedBroadcastPlugin.java new file mode 100644 index 0000000..89a91fd --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/OrderedBroadcastPlugin.java @@ -0,0 +1,25 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.MethodCallExpr; + +import javax.inject.Inject; + +public class OrderedBroadcastPlugin extends JavaPlugin { + + @Inject + public OrderedBroadcastPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches("sendOrderedBroadcast|sendOrderedBroadcastAsUser|sendStickyOrderedBroadcast|sendStickyOrderedBroadcastAsUser")) + .forEach(expr -> addIssue(Severity.WARNING, getLineNumberFromExpression(expr), expr.toString())); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index a9057cb..073c154 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -115,4 +115,7 @@ com.bartek.esa.core.plugin.SqlInjectionPlugin='rawQuery' method detected. Potent com.bartek.esa.core.plugin.WorldAccessPermissionsPlugin=World access permissions detected. Potential data leakage.\n\ The deprecated '${exprName}' constant has been found and it can be risky to use.\n\ It grants world access permission to selected resource.\n\ - Consider using less permissive mode.a. \ No newline at end of file + Consider using less permissive mode. + +com.bartek.esa.core.plugin.OrderedBroadcastPlugin=Sending ordered broadcast. Potential broadcast theft.\n\ + Malicious applications can intercept ordered broadcasts, stop their propagation and resend with malicious data. \ No newline at end of file From 716a6bc92c02818ab419882232091954fe661e6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Wed, 17 Apr 2019 08:53:54 +0200 Subject: [PATCH 35/37] 10: Add WebViewPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++ .../bartek/esa/core/plugin/WebViewPlugin.java | 51 +++++++++++++++++++ src/main/resources/description.properties | 19 ++++++- 3 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 1248dbd..0ae7717 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -137,4 +137,10 @@ public class PluginModule { public Plugin orderedAndStickyBroadcastPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new OrderedBroadcastPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin webViewPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new WebViewPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java b/src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java new file mode 100644 index 0000000..6753969 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java @@ -0,0 +1,51 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.MethodCallExpr; + +import javax.inject.Inject; + +public class WebViewPlugin extends JavaPlugin { + private static final String SETTINGS_METHODS = "addJavascriptInterface|setJavaScriptEnabled|setWebContentsDebuggingEnabled|setAllowFileAccess|setDomStorageEnabled"; + + @Inject + public WebViewPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(MethodCallExpr.class).stream() + .filter(expr -> expr.getName().getIdentifier().matches(SETTINGS_METHODS)) + .forEach(this::issueMethod); + } + + private void issueMethod(MethodCallExpr methodCall) { + switch (methodCall.getName().getIdentifier()) { + case "addJavascriptInterface": + addIssue(Severity.VULNERABILITY, ".JS_INTERFACE", getLineNumberFromExpression(methodCall), methodCall.toString()); + break; + case "setJavaScriptEnabled": + issueSettingsMethod(methodCall, ".JS_ENABLED"); + break; + case "setWebContentsDebuggingEnabled": + issueSettingsMethod(methodCall, ".DEBUGGING_ENABLED"); + break; + case "setAllowFileAccess": + issueSettingsMethod(methodCall, ".ALLOW_FILE_ACCESS"); + break; + } + } + + private void issueSettingsMethod(MethodCallExpr methodCall, String descriptionCode) { + Expression firstArg = methodCall.getArguments().get(0); + if (firstArg.isBooleanLiteralExpr() && firstArg.asBooleanLiteralExpr().getValue()) { + addIssue(Severity.INFO, descriptionCode, getLineNumberFromExpression(methodCall), methodCall.toString()); + } + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 073c154..5a18692 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -118,4 +118,21 @@ com.bartek.esa.core.plugin.WorldAccessPermissionsPlugin=World access permissions Consider using less permissive mode. com.bartek.esa.core.plugin.OrderedBroadcastPlugin=Sending ordered broadcast. Potential broadcast theft.\n\ - Malicious applications can intercept ordered broadcasts, stop their propagation and resend with malicious data. \ No newline at end of file + Malicious applications can intercept ordered broadcasts, stop their propagation and resend with malicious data. + +com.bartek.esa.core.plugin.WebViewPlugin.JS_INTERFACE=WebView with JavaScript interface. Potential malicious code injection.\n\ + The WebView uses 'addJavascriptInterface' method which exposes public methods to JavaScript code. Loading JavaScript code \n\ + from untrusted sources is a major security violation and should never be used. + +com.bartek.esa.core.plugin.WebViewPlugin.JS_ENABLED=JavaScript enabled in WebView.\n\ + The WebView has enabled JavaScript code execution. This can effect in XSS attack.\n\ + Consider disabling JavaScript in WebView. + +com.bartek.esa.core.plugin.WebViewPlugin.DEBUGGING_ENABLED=JavaScript debugging enabled in WebView.\n\ + The WebView has enabled JavaScript code debugging. This can effect in data leakage from WebView component.\n\ + Consider disabling JavaScript debugging in WebView. + +com.bartek.esa.core.plugin.WebViewPlugin.ALLOW_FILE_ACCESS=Access to file system from WebView.\n\ + The WebView has granted access to private files. Loading content from untrusted source may effect with \n\ + accessing private files by malicious site/application.\n\ + Consider disabling this option. From 4277304e2053003d42e094a82f86c194d73c4ba8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Wed, 17 Apr 2019 11:30:10 +0200 Subject: [PATCH 36/37] 10: Add TelephonyManagerPlugin --- .../com/bartek/esa/core/di/PluginModule.java | 6 +++++ .../core/plugin/TelephonyManagerPlugin.java | 26 +++++++++++++++++++ src/main/resources/description.properties | 4 +++ 3 files changed, 36 insertions(+) create mode 100644 src/main/java/com/bartek/esa/core/plugin/TelephonyManagerPlugin.java diff --git a/src/main/java/com/bartek/esa/core/di/PluginModule.java b/src/main/java/com/bartek/esa/core/di/PluginModule.java index 0ae7717..b218fbc 100644 --- a/src/main/java/com/bartek/esa/core/di/PluginModule.java +++ b/src/main/java/com/bartek/esa/core/di/PluginModule.java @@ -143,4 +143,10 @@ public class PluginModule { public Plugin webViewPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { return new WebViewPlugin(globMatcher, xmlHelper); } + + @Provides + @IntoSet + public Plugin telephonyManagerPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + return new TelephonyManagerPlugin(globMatcher, xmlHelper); + } } diff --git a/src/main/java/com/bartek/esa/core/plugin/TelephonyManagerPlugin.java b/src/main/java/com/bartek/esa/core/plugin/TelephonyManagerPlugin.java new file mode 100644 index 0000000..86b88a1 --- /dev/null +++ b/src/main/java/com/bartek/esa/core/plugin/TelephonyManagerPlugin.java @@ -0,0 +1,26 @@ +package com.bartek.esa.core.plugin; + +import com.bartek.esa.core.archetype.JavaPlugin; +import com.bartek.esa.core.model.enumeration.Severity; +import com.bartek.esa.core.xml.XmlHelper; +import com.bartek.esa.file.matcher.GlobMatcher; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.CastExpr; + +import javax.inject.Inject; + +public class TelephonyManagerPlugin extends JavaPlugin { + + @Inject + public TelephonyManagerPlugin(GlobMatcher globMatcher, XmlHelper xmlHelper) { + super(globMatcher, xmlHelper); + } + + @Override + public void run(CompilationUnit compilationUnit) { + compilationUnit.findAll(CastExpr.class).stream() + .filter(expr -> expr.getType().isClassOrInterfaceType()) + .filter(expr -> expr.getType().asClassOrInterfaceType().getName().getIdentifier().equals("TelephonyManager")) + .forEach(expr -> addIssue(Severity.INFO, getLineNumberFromExpression(expr), expr.toString())); + } +} diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 5a18692..113d4d4 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -136,3 +136,7 @@ com.bartek.esa.core.plugin.WebViewPlugin.ALLOW_FILE_ACCESS=Access to file system The WebView has granted access to private files. Loading content from untrusted source may effect with \n\ accessing private files by malicious site/application.\n\ Consider disabling this option. + +com.bartek.esa.core.plugin.TelephonyManagerPlugin=Usage of TelephonyManager.\n\ + The TelephonyManager service is detected to be used.\n\ + Make sure that no sensitive data (like IMEI, phone number etc.) exits the application. From b3c6c86a281aa98f252287383fe142ab2e7e135e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Pluta?= Date: Wed, 17 Apr 2019 12:07:29 +0200 Subject: [PATCH 37/37] 10: Clean plugins --- .../java/com/bartek/esa/core/plugin/StrictModePlugin.java | 2 +- src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java | 2 +- src/main/resources/description.properties | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java b/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java index e8532f6..273f2af 100644 --- a/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/StrictModePlugin.java @@ -24,6 +24,6 @@ public class StrictModePlugin extends JavaPlugin { compilationUnit.findAll(MethodCallExpr.class).stream() .filter(expr -> expr.getName().getIdentifier().equals("setThreadPolicy")) .filter(staticScopeHelper.isFromScope(compilationUnit, "setThreadPolicy", "StrictMode", "android.os")) - .forEach(expr -> addIssue(Severity.INFO, getLineNumberFromExpression(expr), expr.toString())); + .forEach(expr -> addIssue(Severity.WARNING, getLineNumberFromExpression(expr), expr.toString())); } } diff --git a/src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java b/src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java index 6753969..07de49b 100644 --- a/src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java +++ b/src/main/java/com/bartek/esa/core/plugin/WebViewPlugin.java @@ -45,7 +45,7 @@ public class WebViewPlugin extends JavaPlugin { private void issueSettingsMethod(MethodCallExpr methodCall, String descriptionCode) { Expression firstArg = methodCall.getArguments().get(0); if (firstArg.isBooleanLiteralExpr() && firstArg.asBooleanLiteralExpr().getValue()) { - addIssue(Severity.INFO, descriptionCode, getLineNumberFromExpression(methodCall), methodCall.toString()); + addIssue(Severity.WARNING, descriptionCode, getLineNumberFromExpression(methodCall), methodCall.toString()); } } } diff --git a/src/main/resources/description.properties b/src/main/resources/description.properties index 113d4d4..04612e4 100644 --- a/src/main/resources/description.properties +++ b/src/main/resources/description.properties @@ -31,7 +31,7 @@ Consider setting it to 'false'. com.bartek.esa.core.plugin.PermissionsRaceConditionPlugin=Potential permissions race condition vulnerability. \n\ There are declared custom permissions in AndroidManifest.xml and the minimal API version is set to ${minSdkVersion} that is less than 21.\n\ - It means that declared permissions can be obtained by malicious application installed before and without need of having 1proper signature.\n\ + It means that declared permissions can be obtained by malicious application installed before and without need of having proper signature.\n\ Consider setting minimal API version to 21 at least. com.bartek.esa.core.plugin.SecureRandomPlugin=Initializing SecureRandom object with custom seed. \n\ @@ -88,7 +88,7 @@ com.bartek.esa.core.plugin.SuppressWarningsPlugin=@SuppressWarnings annotation w The @SuppressWarnings annotation might be hiding useful warnings.\n\ Consider removing it. -com.bartek.esa.core.plugin.ExportedComponentsPlugin.NO_PERMISSION=Exported activity.\n\ +com.bartek.esa.core.plugin.ExportedComponentsPlugin.NO_PERMISSION=Exported ${componentType}.\n\ The ${componentType} with name '${componentName}' is exported but not protected by any permission. \n\ It means any malicious application could make use of the component. \n\ Consider using 'android:permission' tag and adding custom permission to protect it.