Java: Add Guard Classes for checking OS & unify System Property Access #8032
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| import Member | ||
| import semmle.code.java.security.ExternalProcess | ||
| private import semmle.code.java.dataflow.FlowSteps | ||
|
|
||
| // --- Standard types --- | ||
| /** The class `java.lang.Object`. */ | ||
|
|
@@ -37,6 +38,27 @@ class StringLengthMethod extends Method { | |
| StringLengthMethod() { this.hasName("length") and this.getDeclaringType() instanceof TypeString } | ||
| } | ||
|
|
||
| /** | ||
| * The methods on the class `java.lang.String` that are used to perform partial matches with a specified substring or char. | ||
| */ | ||
| class StringPartialMatchMethod extends Method { | ||
| StringPartialMatchMethod() { | ||
| this.hasName([ | ||
| "contains", "startsWith", "endsWith", "matches", "indexOf", "lastIndexOf", "regionMatches" | ||
| ]) and | ||
| this.getDeclaringType() instanceof TypeString | ||
| } | ||
|
|
||
| /** | ||
| * Gets the index of the parameter that is being matched against. | ||
| */ | ||
| int getMatchParameterIndex() { | ||
| if this.hasName("regionMatches") | ||
| then this.getParameterType(result) instanceof TypeString | ||
| else result = 0 | ||
| } | ||
| } | ||
|
|
||
| /** The class `java.lang.StringBuffer`. */ | ||
| class TypeStringBuffer extends Class { | ||
| TypeStringBuffer() { this.hasQualifiedName("java.lang", "StringBuffer") } | ||
|
|
@@ -228,11 +250,13 @@ class MethodSystemGetenv extends Method { | |
| /** | ||
| * Any method named `getProperty` on class `java.lang.System`. | ||
| */ | ||
| class MethodSystemGetProperty extends ValuePreservingMethod { | ||
|
There was a problem hiding this comment. This isn't a value-preserving method. A value-preserving method is Do you mean to say you want There was a problem hiding this comment.
/**
* Searches for the property with the specified key in this property list.
* If the key is not found in this property list, the default property list,
* and its defaults, recursively, are then checked. The method returns the
* default value argument if the property is not found.
*
* @param key the hashtable key.
* @param defaultValue a default value.
*
* @return the value in this property list with the specified key value.
* @see #setProperty
* @see #defaults
*/
public String getProperty(String key, String defaultValue) {
String val = getProperty(key);
return (val == null) ? defaultValue : val;
}There was a problem hiding this comment. Doh right, I'd forgotten those overloads existed. |
||
| MethodSystemGetProperty() { | ||
| this.hasName("getProperty") and | ||
| this.getDeclaringType() instanceof TypeSystem | ||
| } | ||
|
|
||
| override predicate returnsValue(int arg) { arg = 1 } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -244,6 +268,9 @@ class MethodAccessSystemGetProperty extends MethodAccess { | |
| /** | ||
| * Holds if this call has a compile-time constant first argument with the value `propertyName`. | ||
| * For example: `System.getProperty("user.dir")`. | ||
| * | ||
| * Note: Better to use `semmle.code.java.environment.SystemProperty#getSystemProperty` instead | ||
| * as that predicate covers more libraries' and JDK API's ways of accessing the same information | ||
JLLeitschuh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| predicate hasCompileTimeConstantGetPropertyName(string propertyName) { | ||
|
There was a problem hiding this comment. @smowton wondering if I should deprecate this in favor of the new way of finding all expressions that access system properties There was a problem hiding this comment. Don't deprecate since we use it ourselves, but expand your comment a little to point out that the recommended predicate covers more libraries' ways of accessing the same information. There was a problem hiding this comment. As of my changes, it's only ever now used in my code, it isn't used anywhere else in this repository. There was a problem hiding this comment. Also, at your request, I've improved my comment |
||
| this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,249 @@ | ||
| import java | ||
| private import semmle.code.java.dataflow.DataFlow | ||
| private import semmle.code.java.frameworks.Properties | ||
| private import semmle.code.java.frameworks.apache.Lang | ||
|
|
||
| /** | ||
| * Gets an expression that retrieves the value of `propertyName` from `System.getProperty()`. | ||
| */ | ||
| Expr getSystemProperty(string propertyName) { | ||
| result = getSystemPropertyFromSystem(propertyName) or | ||
| result = getSystemPropertyFromSystemGetProperties(propertyName) or | ||
| result = getSystemPropertyFromFile(propertyName) or | ||
| result = getSystemPropertyFromApacheSystemUtils(propertyName) or | ||
| result = getSystemPropertyFromApacheFileUtils(propertyName) or | ||
| result = getSystemPropertyFromGuava(propertyName) or | ||
| result = getSystemPropertyFromOperatingSystemMXBean(propertyName) or | ||
| result = getSystemPropertyFromSpringProperties(propertyName) | ||
| } | ||
|
|
||
| private MethodAccess getSystemPropertyFromSystem(string propertyName) { | ||
| result.(MethodAccessSystemGetProperty).hasCompileTimeConstantGetPropertyName(propertyName) | ||
| or | ||
| exists(Method m | result.getMethod() = m | m.hasName("lineSeparator")) and | ||
JLLeitschuh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| propertyName = "line.separator" | ||
| } | ||
|
|
||
| /** | ||
| * A method access that retrieves the value of `propertyName` from the following methods: | ||
| * - `System.getProperties().getProperty(...)` | ||
| * - `System.getProperties().get(...)` | ||
| */ | ||
| private MethodAccess getSystemPropertyFromSystemGetProperties(string propertyName) { | ||
| exists(Method getMethod | | ||
| getMethod instanceof PropertiesGetMethod | ||
| or | ||
| getMethod instanceof PropertiesGetPropertyMethod and | ||
| result.getMethod() = getMethod | ||
| ) and | ||
| result.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName and | ||
| DataFlow::localExprFlow(any(MethodAccess m | | ||
| m.getMethod().getDeclaringType() instanceof TypeSystem and | ||
| m.getMethod().hasName("getProperties") | ||
| ), result.getQualifier()) | ||
| } | ||
|
|
||
| private FieldAccess getSystemPropertyFromFile(string propertyName) { | ||
| result.getField() instanceof FieldFileSeparator and propertyName = "file.separator" | ||
| or | ||
| result.getField() instanceof FieldFilePathSeparator and propertyName = "path.separator" | ||
| } | ||
|
|
||
| /** The field `java.io.File.separator` or `java.io.File.separatorChar` */ | ||
| private class FieldFileSeparator extends Field { | ||
| FieldFileSeparator() { | ||
| this.getDeclaringType() instanceof TypeFile and this.hasName(["separator", "separatorChar"]) | ||
| } | ||
| } | ||
|
|
||
| /* The field `java.io.File.pathSeparator` or `java.io.File.pathSeparatorChar` */ | ||
| private class FieldFilePathSeparator extends Field { | ||
| FieldFilePathSeparator() { | ||
| this.getDeclaringType() instanceof TypeFile and | ||
| this.hasName(["pathSeparator", "pathSeparatorChar"]) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A field access to the system property. | ||
| * See: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/SystemUtils.html | ||
| */ | ||
| private FieldAccess getSystemPropertyFromApacheSystemUtils(string propertyName) { | ||
| exists(Field f | f = result.getField() and f.getDeclaringType() instanceof ApacheSystemUtils | | ||
| f.hasName("AWT_TOOLKIT") and propertyName = "awt.toolkit" | ||
| or | ||
| f.hasName("FILE_ENCODING") and propertyName = "file.encoding" | ||
| or | ||
| f.hasName("FILE_SEPARATOR") and propertyName = "file.separator" | ||
| or | ||
| f.hasName("JAVA_AWT_FONTS") and propertyName = "java.awt.fonts" | ||
| or | ||
| f.hasName("JAVA_AWT_GRAPHICSENV") and propertyName = "java.awt.graphicsenv" | ||
| or | ||
| f.hasName("JAVA_AWT_HEADLESS") and propertyName = "java.awt.headless" | ||
| or | ||
| f.hasName("JAVA_AWT_PRINTERJOB") and propertyName = "java.awt.printerjob" | ||
| or | ||
| f.hasName("JAVA_CLASS_PATH") and propertyName = "java.class.path" | ||
| or | ||
| f.hasName("JAVA_CLASS_VERSION") and propertyName = "java.class.version" | ||
| or | ||
| f.hasName("JAVA_COMPILER") and propertyName = "java.compiler" | ||
| or | ||
| f.hasName("JAVA_EXT_DIRS") and propertyName = "java.ext.dirs" | ||
| or | ||
| f.hasName("JAVA_HOME") and propertyName = "java.home" | ||
| or | ||
| f.hasName("JAVA_IO_TMPDIR") and propertyName = "java.io.tmpdir" | ||
| or | ||
| f.hasName("JAVA_LIBRARY_PATH") and propertyName = "java.library.path" | ||
| or | ||
| f.hasName("JAVA_RUNTIME_NAME") and propertyName = "java.runtime.name" | ||
| or | ||
| f.hasName("JAVA_RUNTIME_VERSION") and propertyName = "java.runtime.version" | ||
| or | ||
| f.hasName("JAVA_SPECIFICATION_NAME") and propertyName = "java.specification.name" | ||
| or | ||
| f.hasName("JAVA_SPECIFICATION_VENDOR") and propertyName = "java.specification.vendor" | ||
| or | ||
| f.hasName("JAVA_UTIL_PREFS_PREFERENCES_FACTORY") and | ||
| propertyName = "java.util.prefs.PreferencesFactory" // This really does break the lowercase convention obeyed everywhere else | ||
| or | ||
| f.hasName("JAVA_VENDOR") and propertyName = "java.vendor" | ||
| or | ||
| f.hasName("JAVA_VENDOR_URL") and propertyName = "java.vendor.url" | ||
| or | ||
| f.hasName("JAVA_VERSION") and propertyName = "java.version" | ||
| or | ||
| f.hasName("JAVA_VM_INFO") and propertyName = "java.vm.info" | ||
| or | ||
| f.hasName("JAVA_VM_NAME") and propertyName = "java.vm.name" | ||
| or | ||
| f.hasName("JAVA_VM_SPECIFICATION_NAME") and propertyName = "java.vm.specification.name" | ||
| or | ||
| f.hasName("JAVA_VM_SPECIFICATION_VENDOR") and propertyName = "java.vm.specification.vendor" | ||
| or | ||
| f.hasName("JAVA_VM_VENDOR") and propertyName = "java.vm.vendor" | ||
| or | ||
| f.hasName("JAVA_VM_VERSION") and propertyName = "java.vm.version" | ||
| or | ||
| f.hasName("LINE_SEPARATOR") and propertyName = "line.separator" | ||
| or | ||
| f.hasName("OS_ARCH") and propertyName = "os.arch" | ||
| or | ||
| f.hasName("OS_NAME") and propertyName = "os.name" | ||
| or | ||
| f.hasName("OS_VERSION") and propertyName = "os.version" | ||
| or | ||
| f.hasName("PATH_SEPARATOR") and propertyName = "path.separator" | ||
| or | ||
| f.hasName("USER_COUNTRY") and propertyName = "user.country" | ||
| or | ||
| f.hasName("USER_DIR") and propertyName = "user.dir" | ||
| or | ||
| f.hasName("USER_HOME") and propertyName = "user.home" | ||
| or | ||
| f.hasName("USER_LANGUAGE") and propertyName = "user.language" | ||
| or | ||
| f.hasName("USER_NAME") and propertyName = "user.name" | ||
| or | ||
| f.hasName("USER_TIMEZONE") and propertyName = "user.timezone" | ||
| ) | ||
| } | ||
|
|
||
| private MethodAccess getSystemPropertyFromApacheFileUtils(string propertyName) { | ||
| exists(Method m | | ||
| result.getMethod() = m and | ||
| m.getDeclaringType().hasQualifiedName("org.apache.commons.io", "FileUtils") | ||
| | | ||
| m.hasName(["getTempDirectory", "getTempDirectoryPath"]) and propertyName = "java.io.tmpdir" | ||
| or | ||
| m.hasName(["getUserDirectory", "getUserDirectoryPath"]) and propertyName = "user.home" | ||
| ) | ||
| } | ||
|
Comment on lines
+159
to
+168
There was a problem hiding this comment. Both
I figure that this is still fine as it's essentially equivalent to the string variant, but wrapped in a different type There was a problem hiding this comment. Maybe it should be documented for |
||
|
|
||
| private MethodAccess getSystemPropertyFromGuava(string propertyName) { | ||
| exists(EnumConstant ec | | ||
| ec.getDeclaringType().hasQualifiedName("com.google.common.base", "StandardSystemProperty") and | ||
| result.getQualifier() = ec.getAnAccess() and | ||
| result.getMethod().hasName("value") | ||
| | | ||
| ec.hasName("JAVA_VERSION") and propertyName = "java.version" | ||
| or | ||
| ec.hasName("JAVA_VENDOR") and propertyName = "java.vendor" | ||
| or | ||
| ec.hasName("JAVA_VENDOR_URL") and propertyName = "java.vendor.url" | ||
| or | ||
| ec.hasName("JAVA_HOME") and propertyName = "java.home" | ||
| or | ||
| ec.hasName("JAVA_VM_SPECIFICATION_VERSION") and propertyName = "java.vm.specification.version" | ||
| or | ||
| ec.hasName("JAVA_VM_SPECIFICATION_VENDOR") and propertyName = "java.vm.specification.vendor" | ||
| or | ||
| ec.hasName("JAVA_VM_SPECIFICATION_NAME") and propertyName = "java.vm.specification.name" | ||
| or | ||
| ec.hasName("JAVA_VM_VERSION") and propertyName = "java.vm.version" | ||
| or | ||
| ec.hasName("JAVA_VM_VENDOR") and propertyName = "java.vm.vendor" | ||
| or | ||
| ec.hasName("JAVA_VM_NAME") and propertyName = "java.vm.name" | ||
| or | ||
| ec.hasName("JAVA_SPECIFICATION_VERSION") and propertyName = "java.specification.version" | ||
| or | ||
| ec.hasName("JAVA_SPECIFICATION_VENDOR") and propertyName = "java.specification.vendor" | ||
| or | ||
| ec.hasName("JAVA_SPECIFICATION_NAME") and propertyName = "java.specification.name" | ||
| or | ||
| ec.hasName("JAVA_CLASS_VERSION") and propertyName = "java.class.version" | ||
| or | ||
| ec.hasName("JAVA_CLASS_PATH") and propertyName = "java.class.path" | ||
| or | ||
| ec.hasName("JAVA_LIBRARY_PATH") and propertyName = "java.library.path" | ||
| or | ||
| ec.hasName("JAVA_IO_TMPDIR") and propertyName = "java.io.tmpdir" | ||
| or | ||
| ec.hasName("JAVA_COMPILER") and propertyName = "java.compiler" | ||
| or | ||
| ec.hasName("JAVA_EXT_DIRS") and propertyName = "java.ext.dirs" | ||
| or | ||
| ec.hasName("OS_NAME") and propertyName = "os.name" | ||
| or | ||
| ec.hasName("OS_ARCH") and propertyName = "os.arch" | ||
| or | ||
| ec.hasName("OS_VERSION") and propertyName = "os.version" | ||
| or | ||
| ec.hasName("FILE_SEPARATOR") and propertyName = "file.separator" | ||
| or | ||
| ec.hasName("PATH_SEPARATOR") and propertyName = "path.separator" | ||
| or | ||
| ec.hasName("LINE_SEPARATOR") and propertyName = "line.separator" | ||
| or | ||
| ec.hasName("USER_NAME") and propertyName = "user.name" | ||
| or | ||
| ec.hasName("USER_HOME") and propertyName = "user.home" | ||
| or | ||
| ec.hasName("USER_DIR") and propertyName = "user.dir" | ||
| ) | ||
| } | ||
|
|
||
| private MethodAccess getSystemPropertyFromOperatingSystemMXBean(string propertyName) { | ||
| exists(Method m | | ||
| m = result.getMethod() and | ||
| m.getDeclaringType().hasQualifiedName("java.lang.management", "OperatingSystemMXBean") | ||
|
There was a problem hiding this comment. Might be good to consider subtypes as well, because there is at least There was a problem hiding this comment.
There was a problem hiding this comment. Ah you are right; |
||
| | | ||
| m.getName() = "getName" and propertyName = "os.name" | ||
| or | ||
| m.getName() = "getArch" and propertyName = "os.arch" | ||
| or | ||
| m.getName() = "getVersion" and propertyName = "os.version" | ||
| ) | ||
| } | ||
|
|
||
| private MethodAccess getSystemPropertyFromSpringProperties(string propertyName) { | ||
| exists(Method m | | ||
| m = result.getMethod() and | ||
| m.getDeclaringType().hasQualifiedName("org.springframework.core", "SpringProperties") and | ||
| m.hasName("getProperty") | ||
| ) and | ||
| result.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume this is a pretty large import, I hope it's fine and won't break anything. Should I do this differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok -- note that in FlowSteps.qll's
Frameworksmodule you should private-import this file back. This is to ensure all queries using FlowSteps see the same set of standard value-preserving methods etc, and so the related QL can be evaluated once for the whole query suite, not re-evaluated per query as it would need to be if each one defined extra flow steps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks!