Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Java: Add Guard Classes for checking OS & unify System Property Access #8032

Merged
merged 28 commits into from
Mar 18, 2022
Merged

Java: Add Guard Classes for checking OS & unify System Property Access #8032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
cd073a2
Java: Add Guard Classes for checking OS
JLLeitschuh Feb 14, 2022
39828fd
Apply OS guard checks to TempDirLocalInformationDisclosure
JLLeitschuh Feb 14, 2022
3cdfc00
Cleanup from review feedback
JLLeitschuh Feb 15, 2022
4951344
Update java/ql/lib/semmle/code/java/os/OSCheck.qll
JLLeitschuh Feb 23, 2022
9f5022e
Review fixup and add test for apache SystemUtils
JLLeitschuh Feb 23, 2022
fd63107
Update OS Check from Review Feedback
JLLeitschuh Mar 1, 2022
5913c9a
Refactor OS Guard Checks
JLLeitschuh Mar 1, 2022
dad9a02
Update TempDirInfoDisclosure with new OS Guards
JLLeitschuh Mar 1, 2022
82d3cd8
Improve system property lookup
JLLeitschuh Mar 2, 2022
3c53a05
Add OS Checks based upon separator or path separator
JLLeitschuh Mar 2, 2022
a7adbb7
Refactor more system property access logic
JLLeitschuh Mar 3, 2022
85de9f3
Fix naming of OSCheck method
JLLeitschuh Mar 3, 2022
fea5006
Fix duplicated comment
JLLeitschuh Mar 3, 2022
103c770
Apply suggestions from code review
JLLeitschuh Mar 3, 2022
31527a6
Refactor OS Checks & SystemProperty logic from review feedback
JLLeitschuh Mar 3, 2022
7ab193d
Add System.getProperties().getProperty support
JLLeitschuh Mar 4, 2022
5243fe3
Apply suggestions from code review
JLLeitschuh Mar 4, 2022
523ddb7
Cleanup after code review feedback
JLLeitschuh Mar 4, 2022
b282c7f
Apply suggestions from code review
JLLeitschuh Mar 7, 2022
5b651f2
Fix insufficient tests and add documentation
JLLeitschuh Mar 7, 2022
a21992a
Minor refactoring to improve tests and documentation
JLLeitschuh Mar 7, 2022
2a6c4e9
Add localFlowPlusInitializers
JLLeitschuh Mar 9, 2022
ecb8911
Apply suggestions from code review
JLLeitschuh Mar 10, 2022
1c98642
Remove SystemProperty from FlowSources
JLLeitschuh Mar 10, 2022
50ff2c2
Code cleanup from code review
JLLeitschuh Mar 11, 2022
451661d
Improve guard class names
smowton Mar 15, 2022
09cc8ee
Add tests for StandardSystemProperty
JLLeitschuh Mar 15, 2022
b11340c
Change note tense and detail level
smowton Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion java/ql/lib/semmle/code/java/JDK.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import Member
import semmle.code.java.security.ExternalProcess
private import semmle.code.java.dataflow.FlowSteps
Copy link
Contributor Author

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?

Copy link
Contributor

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 Frameworks module 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!


// --- Standard types ---
/** The class `java.lang.Object`. */
Expand Down Expand Up @@ -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") }
Expand Down Expand Up @@ -228,11 +250,13 @@ class MethodSystemGetenv extends Method {
/**
* Any method named `getProperty` on class `java.lang.System`.
*/
class MethodSystemGetProperty extends Method {
class MethodSystemGetProperty extends ValuePreservingMethod {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a value-preserving method. A value-preserving method is return this; or return param1; or something else that conveys an exact reference across a function.

Do you mean to say you want System.getProperty(x) to generally conserve taint, such that if x is user-controlled then so is the result of System.getProperty? If so I'm not sure we'd want that as a general rule, since in order to be exploited it needs the user to know the name of a property containing sensitive information, or to have a way to get an arbitrary string into that environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.getProperty(propertyName, defaultValue) is a value preserving method. Here's the code of this method:

    /**
     * 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;
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh right, I'd forgotten those overloads existed.

MethodSystemGetProperty() {
this.hasName("getProperty") and
this.getDeclaringType() instanceof TypeSystem
}

override predicate returnsValue(int arg) { arg = 1 }
}

/**
Expand All @@ -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 ways of accessing the same information via various libraries.
*/
predicate hasCompileTimeConstantGetPropertyName(string propertyName) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smowton wondering if I should deprecate this in favor of the new way of finding all expressions that access system properties

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of my changes, it's only ever now used in my code, it isn't used anywhere else in this repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, at your request, I've improved my comment

this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName
Expand Down
23 changes: 2 additions & 21 deletions java/ql/lib/semmle/code/java/StringFormat.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import java
import dataflow.DefUse
private import semmle.code.java.environment.SystemProperty

/**
* A library method that formats a number of its arguments according to a
Expand Down Expand Up @@ -312,27 +313,7 @@ private predicate formatStringValue(Expr e, string fmtvalue) {
or
formatStringValue(e.(ChooseExpr).getAResultExpr(), fmtvalue)
or
exists(Method getprop, MethodAccess ma, string prop |
e = ma and
ma.getMethod() = getprop and
getprop.hasName("getProperty") and
getprop.getDeclaringType().hasQualifiedName("java.lang", "System") and
getprop.getNumberOfParameters() = 1 and
ma.getAnArgument().(StringLiteral).getValue() = prop and
(prop = "line.separator" or prop = "file.separator" or prop = "path.separator") and
fmtvalue = "x" // dummy value
)
or
exists(Field f |
e = f.getAnAccess() and
f.getDeclaringType() instanceof TypeFile and
fmtvalue = "x" // dummy value
|
f.hasName("pathSeparator") or
f.hasName("pathSeparatorChar") or
f.hasName("separator") or
f.hasName("separatorChar")
)
e = getSystemProperty(["line.separator", "file.separator", "path.separator"]) and fmtvalue = "x" // dummy value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simpler, and more comprehensive! 😄

)
}

Expand Down
1 change: 1 addition & 0 deletions java/ql/lib/semmle/code/java/dataflow/FlowSources.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ class EnvReadMethod extends Method {
EnvReadMethod() {
this instanceof MethodSystemGetenv or
this instanceof PropertiesGetPropertyMethod or
this instanceof PropertiesGetMethod or
this instanceof MethodSystemGetProperty
}
}
Expand Down
2 changes: 2 additions & 0 deletions java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ private import semmle.code.java.dataflow.DataFlow
* ensuring that they are visible to the taint tracking library.
*/
private module Frameworks {
private import semmle.code.java.JDK
private import semmle.code.java.frameworks.jackson.JacksonSerializability
private import semmle.code.java.frameworks.android.AsyncTask
private import semmle.code.java.frameworks.android.Intent
private import semmle.code.java.frameworks.android.SQLite
private import semmle.code.java.frameworks.Guice
private import semmle.code.java.frameworks.Properties
private import semmle.code.java.frameworks.Protobuf
private import semmle.code.java.frameworks.guava.Guava
private import semmle.code.java.frameworks.apache.Lang
Expand Down
Loading