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

[JLLeitschuh]

This is useful when you have a bit of logic that is only vulnerable on specific OSes. Additionally, it unifies the access of all system properties (from all libraries and JDK APIs) into one method call getSystemProperty(_).

[JLLeitschuh]

Without the OS guard changes, this gets flagged as vulnerable

[JLLeitschuh]

Without the OS guard changes, this also gets flagged as vulnerable

[JLLeitschuh]

Simple fix because this wouldn't have compiled earlier

[JLLeitschuh]

Thanks @intrigus-lgtm for the feedback! 🙂

[Marcono1234]

Would it make sense to also consider OperatingSystemMXBean.getName()?

Sometimes the OS path separator (File.pathSeparator, File.pathSeparatorChar) and OS file path separator (File.separator, File.separatorChar) are used for OS detection; examples:

• https://github.com/openjdk/jdk/blob/d7f31d0d53bfec627edc83ceb75fc6202891e186/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java#L673-L674
• https://github.com/openjdk/jdk/blob/d7f31d0d53bfec627edc83ceb75fc6202891e186/src/jdk.compiler/share/classes/com/sun/tools/sjavac/JavacState.java#L913

Would it make sense to consider these as well? (That is, comparison in EqualityTest, and maybe EqualsMethod call (qualifier or argument), where the compared value is a compile time constant)

(Feel free to consider this only as suggestion; I am not a member of this project)

[atorralba]

This has some QLDoc errors:

The following CodeQL elements are lacking documentation:
semmle/code/java/environment/SystemProperty.qll  SystemProperty                   file
semmle/code/java/frameworks/Properties.qll       Properties::PropertiesGetMethod  class

Also, a bunch of autoformatting required:

Error: ql/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql would change by autoformatting.
Error: ql/java/ql/test/library-tests/os/unix-test.ql would change by autoformatting.
Error: ql/java/ql/test/library-tests/os/specific-unix-variant-test.ql would change by autoformatting.
Error: ql/java/ql/test/library-tests/os/windows-test.ql would change by autoformatting.
Error: ql/java/ql/test/library-tests/os/specific-windows-variant-test.ql would change by autoformatting.

[smowton]

Having to revert #8360 because of apparent performance problems. Remind me why you needed concatenation of string + char again?

If you still want it, a few possibilities to investigate:

1. Using test project https://lgtm.com/projects/g/geogebra/geogebra, why do we end up performing millions of string concatenations at considerable cost? Are there really millions of string + non-string-constant operations in the codebase?
2. If there really are some long concatenation sequences in the codebase causing this to cost a lot, consider a recursion that finds the parts of the concatenation and assigns them integer indices, before finally concatenating them in one go using QL's concat aggregate.

[JLLeitschuh]

Remind me why you needed concatenation of string + char again?

For this particular change, I don't really, I wrote that new predicate to be more extensive than I needed so that it was generally more useful. In this particular PR, I just need to do equality checking between both string and characters because the new system property predicate sometimes returns a expression that has the type char.

Would adding the cached annotation to getStrigifiedValue and getString resolve the performance issues?

[smowton]

No, cached is only useful if we're recomputing a particular predicate repeatedly. Here one evaluation of it is expensive.

[JLLeitschuh]

If there really are some long concatenation sequences in the codebase causing this to cost a lot, consider a recursion that finds the parts of the concatenation and assigns them integer indices, before finally concatenating them in one go using QL's concat aggregate.

I have no idea what this would look like unfortunately. I'd need some pointers on what the code would look like here.

[smowton]

Probably not worth the difficulty just for this. I've made #8407 to revert the original addition of CharacterLiteral to getStringValue per Marcono's objections in #8325, and recommend you simply special-case CharacterLiteral in this PR as you did originally for the time being. We can investigate how to do this in a performant manner as and when the need arises.

[JLLeitschuh]

I'm ready for a final PR review or a merge if everyone is happy with this in it's current state. 😃

[smowton]

I believe we don't have tests for the Guava cases yet?

[smowton]

I also made a minor commit above to improve some class names.

[JLLeitschuh]

I believe we don't have tests for the Guava cases yet?

Done

[JLLeitschuh]

Anything else required here?

[smowton]

Just running a new performance evaluation as I realised the first one only considered queries with precision >= high, whereas the temp-dir-local-info query has precision = medium

[JLLeitschuh]

Yeay! Very happy this is finally merged!

[aschackmull]

These three barrier-guards don't really make sense. Using Guard.controls in the checks predicate of a barrier-guard is almost guaranteed to be completely wrong, as the barrier-guard abstraction is explicitly designed to handle the .controls-aspect of guards.
The expression e should be the variable access being checked by the guard, and the branch should be the result of the guard that indicates that e is safe. So the basic implementation hint for the checks predicate should just be to use the AST and nothing more. If you really want to use Guard.controls to pick out the nodes that are sanitised then you're really just defining a regular sanitizer and not a sanitizer-guard.

[JLLeitschuh]

So, I partially understand what you're saying. That being said, I don't know how to fix it.

Also, could you suggest a test case that this doesn't work for that I could use as a unit test

[aschackmull]

A barrierguard is just syntactic sugar for a sanitizer with a specific pattern. To see how a barrier-guard gets desugared into a sanitizer, look at the getAGuardedNode() predicate:

  final Node getAGuardedNode() {
    exists(SsaVariable v, boolean branch, RValue use |
      this.checks(v.getAUse(), branch) and
      use = v.getAUse() and
      this.controls(use.getBasicBlock(), branch) and
      result.asExpr() = use
    )
  }

You end up just joining this.controls with itself, which semantically is mostly harmless. But you can simplify things by just defining the sanitizers directly. E.g. replace IsSpecificWindowsBarrierGuard with the following:

abstract private class WindowsOsSanitizer extends DataFlow::Node { }

private class IsSpecificWindowsSanitizer extends WindowsOsSanitizer {
  IsSpecificWindowsSanitizer() {
    any(IsSpecificWindowsVariant guard).control(this.asExpr().getBasicBlock(), true)
  }
}

...

 override predicate isSanitizer(DataFlow::Node node) {
    node instanceof WindowsOsSanitizer
  }

This is mostly equivalent to your IsSpecificWindowsBarrierGuard except it drops the restriction that only uses of ssa variables are sanitized.
The other two, IsNotUnixBarrierGuard and IsWindowsBarrierGuard, can be similarly rewritten, but note that they are somewhat broken as-is, since they currently guard both of their branches. In

if (<IsWindowsBarrierGuard>) {
  foo
} else {
  bar
}

you're sanitizing both foo and bar.

[aschackmull]

I guess it is unlikely that this will give spurious matches, but do note that _ is a wildcard for the .matches predicate, so all of the underscores in the names below could be matched with anything.