Skip to content

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

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

Merged
smowton 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 1 commit
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
Prev Previous commit
Next Next commit
Add OS Checks based upon separator or path separator
  • Loading branch information
@JLLeitschuh
JLLeitschuh committed Mar 2, 2022
commit 3c53a05e1622de7c599bf3eb61c661f600031cc0
15 changes: 15 additions & 0 deletions 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 @@ -202,6 +202,21 @@ class TypeFile extends Class {
TypeFile() { this.hasQualifiedName("java.io", "File") }
}

/** The field `java.io.File.separator` or `java.io.File.separatorChar` */
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` */
class FieldFilePathSeparator extends Field {
FieldFilePathSeparator() {
this.getDeclaringType() instanceof TypeFile and
this.hasName(["pathSeparator", "pathSeparatorChar"])
}
}

// --- Standard methods ---
/**
* Any constructor of class `java.lang.ProcessBuilder`.
Expand Down
7 changes: 2 additions & 5 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 @@ -325,13 +325,10 @@ private predicate formatStringValue(Expr e, string fmtvalue) {
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")
f instanceof FieldFileSeparator or
f instanceof FieldFilePathSeperator
)
)
}
Expand Down
36 changes: 36 additions & 0 deletions java/ql/lib/semmle/code/java/os/OSCheck.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,42 @@ private class IsWindowsFromSystemProp extends IsWindowsGuard instanceof MethodAc
IsWindowsFromSystemProp() { isOsFromSystemProp(this, "window%") }
Copy link
Copy Markdown
Contributor

@Marcono1234 Marcono1234 Mar 7, 2022

Choose a reason for hiding this comment

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

Maybe a bit difficult to make a clear distinction, but this might rather be IsSpecificWindowsVariant due to the %?

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 7, 2022

Choose a reason for hiding this comment

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

🤔 Technically, it would be a regular expression of windows? since I want to support matching both window and windows

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 7, 2022

Choose a reason for hiding this comment

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

I've updated the code to use a regular expression instead

}

/**
* Holds when the Guard is an equality check between the Field `f` and the string or char constant `compareToLiteral`.
*/
private Guard isOsFromFieldEqualityCheck(Field f, string compareToLiteral) {
result
.isEquality(any(FieldAccess fa | fa.getField() = f),
any(Literal literal |
(literal instanceof CharacterLiteral or literal instanceof StringLiteral) and
literal.getValue() = compareToLiteral
), _)
Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 3, 2022

Choose a reason for hiding this comment

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

@smowton I could probably simplify this logic with the following diff to CompileTimeConstantExpr. Do I have your approval to make the following change?

diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll
index 5a991d8814..d98cdbb6b0 100755
--- a/java/ql/lib/semmle/code/java/Expr.qll
+++ b/java/ql/lib/semmle/code/java/Expr.qll
@@ -168,6 +168,8 @@ class CompileTimeConstantExpr extends Expr {
   string getStringValue() {
     result = this.(StringLiteral).getValue()
     or
+    result = this.(CharacterLiteral).getValue()
+    or
     result =
       this.(AddExpr).getLeftOperand().(CompileTimeConstantExpr).getStringValue() +
         this.(AddExpr).getRightOperand().(CompileTimeConstantExpr).getStringValue()

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

To make it easier to review this PR which already has scope-creep, I broke this out into an independent PR: #8325

Copy link
Copy Markdown
Contributor

@Marcono1234 Marcono1234 Mar 7, 2022

Choose a reason for hiding this comment

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

Should probably consider the polarity (currently specified as _ here), e.g. == '/' is not the same as != '/'.
Possibly this parameter could have two parameters, compareEqual and compareNotEqual and then use the respective boolean for the polarity. Maybe there is a cleaner or more efficient solution though.

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 7, 2022

Choose a reason for hiding this comment

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

This is my understanding of guards from someone who's recently been introduced to them. So, this is my lay-person's explanation:

This predicate is is establishing that a guard exists, not whether or not the guard guarantees that a specific case is true or false.

IE. Both of these are 'windows guards':

  • File.separatorChar == '\\'
  • `File.separatorChar != '\'

However, their end-use utility as guards is only in the larger context of code being guarded is only possible when they are used with DataFlow::BarrierGuard.

TL;DR: Guard merely establishes the existence of a guard. BarrierGuard establishes whether or not the Guard is relevant with respect to the true/false state of it's evaluation.

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 7, 2022

Choose a reason for hiding this comment

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

Looks like my assessment here was kinda wrong. 🤔

}

private class IsWindowsFromCharPathSeperator extends IsWindowsGuard {
IsWindowsFromCharPathSeperator() {
this = isOsFromFieldEqualityCheck(any(FieldFilePathSeparator f), ";")
}
}

private class IsWindowsFromCharSeperator extends IsWindowsGuard {
IsWindowsFromCharSeperator() {
this = isOsFromFieldEqualityCheck(any(FieldFileSeparator f), "\\")
}
}

private class IsUnixFromCharPathSeperator extends IsUnixGuard {
IsUnixFromCharPathSeperator() {
this = isOsFromFieldEqualityCheck(any(FieldFilePathSeparator f), ":")
}
}

private class IsUnixFromCharSeperator extends IsUnixGuard {
IsUnixFromCharSeperator() {
this = isOsFromFieldEqualityCheck(any(FieldFileSeparator f), "/")
}
}

private class IsUnixFromSystemProp extends IsAnyUnixGuard instanceof MethodAccess {
IsUnixFromSystemProp() { isOsFromSystemProp(this, ["mac%", "linux%"]) }
Comment thread
JLLeitschuh marked this conversation as resolved.
Outdated
}
Expand Down
34 changes: 34 additions & 0 deletions java/ql/test/library-tests/os/Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@

import java.io.File;
import java.nio.file.FileSystems;
import java.nio.file.Path;

Expand Down Expand Up @@ -38,6 +40,22 @@ void testWindows() {
} else {
// Might be another version of windows
}

if (File.pathSeparatorChar == ';') {
onlyOnWindows();
}

if (File.pathSeparator == ";") {
onlyOnWindows();
}

if (File.separatorChar == '\\') {
onlyOnWindows();
}

if (File.separator == "\\") {
onlyOnWindows();
}
}

void testUnix() {
Expand All @@ -55,6 +73,22 @@ void testUnix() {
// Reasonable assumption, maybe not 100% accurate, but it's 'good enough'
onlyOnWindows();
}

if (File.pathSeparatorChar == ':') {
onlyOnUnix();
}

if (File.pathSeparator == ":") {
onlyOnUnix();
}

if (File.separatorChar == '/') {
onlyOnUnix();
}

if (File.separator == "/") {
onlyOnUnix();
}
}

void testLinux() {
Expand Down
16 changes: 8 additions & 8 deletions java/ql/test/library-tests/os/any-unix-test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1415:5:1415:84 | IS_OS_SOLARIS |
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1427:5:1427:83 | IS_OS_SUN_OS |
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1625:5:1625:80 | IS_OS_ZOS |
| Test.java:61:13:61:73 | contains(...) |
| Test.java:65:13:65:59 | contains(...) |
| Test.java:69:13:69:35 | SystemUtils.IS_OS_LINUX |
| Test.java:75:14:75:36 | SystemUtils.IS_OS_LINUX |
| Test.java:83:13:83:62 | contains(...) |
| Test.java:87:14:87:72 | contains(...) |
| Test.java:91:14:91:34 | SystemUtils.IS_OS_MAC |
| Test.java:97:14:97:45 | SystemUtils.IS_OS_MAC_OSX_MOJAVE |
| Test.java:95:13:95:73 | contains(...) |
| Test.java:99:13:99:59 | contains(...) |
| Test.java:103:13:103:35 | SystemUtils.IS_OS_LINUX |
| Test.java:109:14:109:36 | SystemUtils.IS_OS_LINUX |
| Test.java:117:13:117:62 | contains(...) |
| Test.java:121:14:121:72 | contains(...) |
| Test.java:125:14:125:34 | SystemUtils.IS_OS_MAC |
| Test.java:131:14:131:45 | SystemUtils.IS_OS_MAC_OSX_MOJAVE |
2 changes: 1 addition & 1 deletion java/ql/test/library-tests/os/any-windows-test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1584:5:1584:86 | IS_OS_WINDOWS_7 |
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1596:5:1596:86 | IS_OS_WINDOWS_8 |
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1608:5:1608:87 | IS_OS_WINDOWS_10 |
| Test.java:36:13:36:40 | SystemUtils.IS_OS_WINDOWS_XP |
| Test.java:38:13:38:40 | SystemUtils.IS_OS_WINDOWS_XP |
6 changes: 3 additions & 3 deletions java/ql/test/library-tests/os/unix-test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1439:5:1439:81 | IS_OS_UNIX |
| Test.java:44:13:44:95 | contains(...) |
| Test.java:48:13:48:84 | contains(...) |
| Test.java:52:13:52:34 | SystemUtils.IS_OS_UNIX |
| Test.java:62:13:62:95 | contains(...) |
| Test.java:66:13:66:84 | contains(...) |
| Test.java:70:13:70:34 | SystemUtils.IS_OS_UNIX |
8 changes: 4 additions & 4 deletions java/ql/test/library-tests/os/windows-test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
| ../../stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/SystemUtils.java:1451:5:1451:84 | IS_OS_WINDOWS |
| Test.java:18:13:18:61 | contains(...) |
| Test.java:22:13:22:75 | contains(...) |
| Test.java:26:13:26:75 | contains(...) |
| Test.java:30:13:30:37 | SystemUtils.IS_OS_WINDOWS |
| Test.java:20:13:20:61 | contains(...) |
| Test.java:24:13:24:75 | contains(...) |
| Test.java:28:13:28:75 | contains(...) |
| Test.java:32:13:32:37 | SystemUtils.IS_OS_WINDOWS |