Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions java/change-notes/2021-03-05-commons-object-utils.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
lgtm,codescanning
* Added models for `ObjectUtils` methods in the Apache Commons Lang library. This may lead to more results from any dataflow query where traversal of `ObjectUtils` methods means we can now complete a path from a source of tainted data to a corresponding sink.
27 changes: 27 additions & 0 deletions java/ql/src/semmle/code/java/frameworks/apache/Lang.qll
Original file line number Diff line number Diff line change
Expand Up @@ -396,3 +396,30 @@ private class ApacheRegExUtilsModel extends SummaryModelCsv {
]
}
}

/**
* Taint-propagating models for `ObjectUtils`.
*/
private class ApacheObjectUtilsModel extends SummaryModelCsv {
override predicate row(string row) {
row =
[
// Note all the functions annotated with `taint` flow really should have `value` flow,
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment might now be redundant.

// but we don't support value-preserving varargs functions at the moment.
"org.apache.commons.lang3;ObjectUtils;false;clone;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;cloneIfPossible;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;CONST;;;Argument;ReturnValue;value",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also match CONST_BYTE and CONST_SHORT?

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 believe these won't work due to a type mismatch? Not 100% sure though

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean they won't work for dataflow because CodeQL does not support dataflow with different types? Would it at least be good then to model them as taint?

Copy link
Contributor

Choose a reason for hiding this comment

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

They should work just fine. The type system that's used for pruning data flow allows for implicit casts, it effectively does not distinguish between the different numeric primitive types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

"org.apache.commons.lang3;ObjectUtils;false;CONST_BYTE;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;CONST_SHORT;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;defaultIfNull;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;taint",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;value",

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this has to be "taint" a while yet until we support proper specification of array contents. The argument is an array (possibly implicitly created), but the method does not return that exact array, so this is not value-preserving from Argument to ReturnValue. The proper value-preserving spec is from Content[Array] of Argument to ReturnValue, but that isn't supported yet.

"org.apache.commons.lang3;ObjectUtils;false;getIfNull;;;Argument[0];ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;max;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;median;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;min;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;mode;;;Argument;ReturnValue;taint",
Comment on lines +417 to +420
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"org.apache.commons.lang3;ObjectUtils;false;max;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;median;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;min;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;mode;;;Argument;ReturnValue;taint",
"org.apache.commons.lang3;ObjectUtils;false;max;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;median;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;min;;;Argument;ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;mode;;;Argument;ReturnValue;value",

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Same as above.

"org.apache.commons.lang3;ObjectUtils;false;requireNonEmpty;;;Argument[0];ReturnValue;value",
"org.apache.commons.lang3;ObjectUtils;false;toString;(Object,String);;Argument[1];ReturnValue;value"
]
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import org.apache.commons.lang3.ObjectUtils;

public class ObjectUtilsTest {
String taint() { return "tainted"; }

private static class IntSource {
static int taint() { return 0; }
}

void sink(Object o) {}

void test() throws Exception {
sink(ObjectUtils.clone(taint())); // $hasValueFlow
sink(ObjectUtils.cloneIfPossible(taint())); // $hasValueFlow
sink(ObjectUtils.CONST(taint())); // $hasValueFlow
sink(ObjectUtils.CONST_SHORT(IntSource.taint())); // $hasValueFlow
sink(ObjectUtils.CONST_BYTE(IntSource.taint())); // $hasValueFlow
sink(ObjectUtils.defaultIfNull(taint(), null)); // $hasValueFlow
sink(ObjectUtils.defaultIfNull(null, taint())); // $hasValueFlow
sink(ObjectUtils.firstNonNull(taint(), null, null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.firstNonNull(null, taint(), null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.firstNonNull(null, null, taint())); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.getIfNull(taint(), null)); // $hasValueFlow
sink(ObjectUtils.max(taint(), null, null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.max(null, taint(), null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.max(null, null, taint())); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.median(taint(), null, null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.median((String)null, taint(), null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.median((String)null, null, taint())); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.min(taint(), null, null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.min(null, taint(), null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.min(null, null, taint())); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.mode(taint(), null, null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.mode(null, taint(), null)); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.mode(null, null, taint())); // $hasTaintFlow $MISSING:hasValueFlow
sink(ObjectUtils.requireNonEmpty(taint(), "message")); // $hasValueFlow
sink(ObjectUtils.requireNonEmpty("not null", taint())); // GOOD (message doesn't propagate to the return)
sink(ObjectUtils.toString(taint(), "default string")); // GOOD (first argument is stringified)
sink(ObjectUtils.toString(null, taint())); // $hasValueFlow
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,20 @@ import java
import semmle.code.java.dataflow.TaintTracking
import TestUtilities.InlineExpectationsTest

class Conf extends TaintTracking::Configuration {
Conf() { this = "qltest:frameworks:apache-commons-lang3" }
class TaintFlowConf extends TaintTracking::Configuration {
TaintFlowConf() { this = "qltest:frameworks:apache-commons-lang3-taint-flow" }

override predicate isSource(DataFlow::Node n) {
n.asExpr().(MethodAccess).getMethod().hasName("taint")
}

override predicate isSink(DataFlow::Node n) {
exists(MethodAccess ma | ma.getMethod().hasName("sink") | n.asExpr() = ma.getAnArgument())
}
}

class ValueFlowConf extends DataFlow::Configuration {
ValueFlowConf() { this = "qltest:frameworks:apache-commons-lang3-value-flow" }

override predicate isSource(DataFlow::Node n) {
n.asExpr().(MethodAccess).getMethod().hasName("taint")
Expand All @@ -17,11 +29,19 @@ class Conf extends TaintTracking::Configuration {
class HasFlowTest extends InlineExpectationsTest {
HasFlowTest() { this = "HasFlowTest" }

override string getARelevantTag() { result = "hasTaintFlow" }
override string getARelevantTag() { result = ["hasTaintFlow", "hasValueFlow"] }

override predicate hasActualResult(Location location, string element, string tag, string value) {
tag = "hasTaintFlow" and
exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) |
exists(DataFlow::Node src, DataFlow::Node sink, TaintFlowConf conf | conf.hasFlow(src, sink) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well add and not any(ValueFlowConf conf | conf.hasFlow(src, sink)) to make the expected output shorter - there's no need to report taint-flow if we have value-flow as that will always be the case.

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

not any(ValueFlowConf vconf).hasFlow(src, sink) and
sink.getLocation() = location and
element = sink.toString() and
value = ""
)
or
tag = "hasValueFlow" and
exists(DataFlow::Node src, DataFlow::Node sink, ValueFlowConf conf | conf.hasFlow(src, sink) |
sink.getLocation() = location and
element = sink.toString() and
value = ""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.commons.lang3;

import java.io.IOException;
import java.io.Serializable;
import java.lang.reflect.Array;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.time.Duration;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.TreeSet;
import java.util.function.Supplier;

import org.apache.commons.lang3.text.StrBuilder;

@SuppressWarnings("deprecation") // deprecated class StrBuilder is imported
// because it is part of the signature of deprecated methods
public class ObjectUtils {
public static class Null implements Serializable {
}
public static final Null NULL = new Null();

public static boolean allNotNull(final Object... values) {
return false;
}

public static boolean allNull(final Object... values) {
return false;
}

public static boolean anyNotNull(final Object... values) {
return false;
}

public static boolean anyNull(final Object... values) {
return false;
}

public static <T> T clone(final T obj) {
return null;
}

public static <T> T cloneIfPossible(final T obj) {
return null;
}

public static <T extends Comparable<? super T>> int compare(final T c1, final T c2) {
return 0;
}

public static <T extends Comparable<? super T>> int compare(final T c1, final T c2, final boolean nullGreater) {
return 0;
}

public static boolean CONST(final boolean v) {
return false;
}

public static byte CONST(final byte v) {
return 0;
}

public static char CONST(final char v) {
return '\0';
}

public static double CONST(final double v) {
return 0;
}

public static float CONST(final float v) {
return 0;
}

public static int CONST(final int v) {
return 0;
}

public static long CONST(final long v) {
return 0;
}

public static short CONST(final short v) {
return 0;
}

public static <T> T CONST(final T v) {
return null;
}

public static byte CONST_BYTE(final int v) {
return 0;
}

public static short CONST_SHORT(final int v) {
return 0;
}

public static <T> T defaultIfNull(final T object, final T defaultValue) {
return null;
}

public static boolean equals(final Object object1, final Object object2) {
return false;
}

public static <T> T firstNonNull(final T... values) {
return null;
}

public static <T> T getFirstNonNull(final Supplier<T>... suppliers) {
return null;
}

public static <T> T getIfNull(final T object, final Supplier<T> defaultSupplier) {
return null;
}

public static int hashCode(final Object obj) {
return 0;
}

public static int hashCodeMulti(final Object... objects) {
return 0;
}

public static void identityToString(final Appendable appendable, final Object object) throws IOException {
}

public static String identityToString(final Object object) {
return null;
}

public static void identityToString(final StrBuilder builder, final Object object) {
}

public static void identityToString(final StringBuffer buffer, final Object object) {
}

public static void identityToString(final StringBuilder builder, final Object object) {
}

public static boolean isEmpty(final Object object) {
return false;
}

public static boolean isNotEmpty(final Object object) {
return false;
}

public static <T extends Comparable<? super T>> T max(final T... values) {
return null;
}

public static <T> T median(final Comparator<T> comparator, final T... items) {
return null;
}

public static <T extends Comparable<? super T>> T median(final T... items) {
return null;
}

public static <T extends Comparable<? super T>> T min(final T... values) {
return null;
}

public static <T> T mode(final T... items) {
return null;
}

public static boolean notEqual(final Object object1, final Object object2) {
return false;
}

public static <T> T requireNonEmpty(final T obj) {
return null;
}

public static <T> T requireNonEmpty(final T obj, final String message) {
return null;
}

public static String toString(final Object obj) {
return null;
}

public static String toString(final Object obj, final String nullStr) {
return null;
}

public static String toString(final Object obj, final Supplier<String> supplier) {
return null;
}

public static void wait(final Object obj, final Duration duration) throws InterruptedException {
}

public ObjectUtils() {
}

}