From 1c1ca7002743653d99e9248a6f6c5c9be438dbba Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 5 Mar 2021 17:59:05 +0000 Subject: [PATCH 1/5] Add models for flow- and taint-preserving functions in Commons ObjectUtils. These should all be value-preserving, but we don't support value-preserving varargs methods yet. --- .../2021-03-05-commons-object-utils.md | 2 + .../code/java/frameworks/apache/Lang.qll | 25 ++ .../apache-commons-lang3/ObjectUtilsTest.java | 35 +++ .../frameworks/apache-commons-lang3/flow.ql | 27 ++- .../org/apache/commons/lang3/ObjectUtils.java | 221 ++++++++++++++++++ 5 files changed, 306 insertions(+), 4 deletions(-) create mode 100644 java/change-notes/2021-03-05-commons-object-utils.md create mode 100644 java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java create mode 100644 java/ql/test/stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/ObjectUtils.java diff --git a/java/change-notes/2021-03-05-commons-object-utils.md b/java/change-notes/2021-03-05-commons-object-utils.md new file mode 100644 index 000000000000..96cf29d799cf --- /dev/null +++ b/java/change-notes/2021-03-05-commons-object-utils.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Add models for ObjectUtils functions in the Apache Commons-Lang library. This may lead to more results from any dataflow query where traversal of ObjectUtils functions means we can now complete a path from a source of tainted data to a corresponding sink. diff --git a/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll b/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll index b2fb782899ca..44f769a28858 100644 --- a/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll +++ b/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll @@ -396,3 +396,28 @@ 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, + // 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", + "org.apache.commons.lang3;ObjectUtils;false;defaultIfNull;;;Argument;ReturnValue;value", + "org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;taint", + "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", + "org.apache.commons.lang3;ObjectUtils;false;requireNonEmpty;;;Argument[0];ReturnValue;value", + "org.apache.commons.lang3;ObjectUtils;false;toString;(Object,String);;Argument[1];ReturnValue;value" + ] + } +} diff --git a/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java new file mode 100644 index 000000000000..c19d819154f0 --- /dev/null +++ b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java @@ -0,0 +1,35 @@ +import org.apache.commons.lang3.ObjectUtils; + +public class ObjectUtilsTest { + String taint() { return "tainted"; } + + void sink(Object o) {} + + void test() throws Exception { + sink(ObjectUtils.clone(taint())); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.cloneIfPossible(taint())); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.CONST(taint())); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.defaultIfNull(taint(), null)); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.defaultIfNull(null, taint())); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.firstNonNull(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.firstNonNull(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.firstNonNull(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.getIfNull(taint(), null)); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.max(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.max(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.max(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.median(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.median((String)null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.median((String)null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.min(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.min(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.min(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.mode(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.mode(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.mode(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y + sink(ObjectUtils.requireNonEmpty(taint(), "message")); // $hasTaintFlow=y $hasValueFlow=y + 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())); // $hasTaintFlow=y $hasValueFlow=y + } +} diff --git a/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql b/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql index be46920c0882..957e4dd2f740 100644 --- a/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql +++ b/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql @@ -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") @@ -17,11 +29,18 @@ 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) | + 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 = "" diff --git a/java/ql/test/stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/ObjectUtils.java b/java/ql/test/stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/ObjectUtils.java new file mode 100644 index 000000000000..fdd2281cfc61 --- /dev/null +++ b/java/ql/test/stubs/apache-commons-lang3-3.7/org/apache/commons/lang3/ObjectUtils.java @@ -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 clone(final T obj) { + return null; + } + + public static T cloneIfPossible(final T obj) { + return null; + } + + public static > int compare(final T c1, final T c2) { + return 0; + } + + public static > 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 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 defaultIfNull(final T object, final T defaultValue) { + return null; + } + + public static boolean equals(final Object object1, final Object object2) { + return false; + } + + public static T firstNonNull(final T... values) { + return null; + } + + public static T getFirstNonNull(final Supplier... suppliers) { + return null; + } + + public static T getIfNull(final T object, final Supplier 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 max(final T... values) { + return null; + } + + public static T median(final Comparator comparator, final T... items) { + return null; + } + + public static > T median(final T... items) { + return null; + } + + public static > T min(final T... values) { + return null; + } + + public static T mode(final T... items) { + return null; + } + + public static boolean notEqual(final Object object1, final Object object2) { + return false; + } + + public static T requireNonEmpty(final T obj) { + return null; + } + + public static 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 supplier) { + return null; + } + + public static void wait(final Object obj, final Duration duration) throws InterruptedException { + } + + public ObjectUtils() { + } + +} From b5268def1631d9f8a52b8d33871025afe9e681ad Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 9 Mar 2021 15:52:10 +0000 Subject: [PATCH 2/5] Add models for CONST_BYTE and CONST_SHORT --- java/ql/src/semmle/code/java/frameworks/apache/Lang.qll | 2 ++ .../frameworks/apache-commons-lang3/ObjectUtilsTest.java | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll b/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll index 44f769a28858..0b64361c0183 100644 --- a/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll +++ b/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll @@ -409,6 +409,8 @@ private class ApacheObjectUtilsModel extends SummaryModelCsv { "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", + "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", "org.apache.commons.lang3;ObjectUtils;false;getIfNull;;;Argument[0];ReturnValue;value", diff --git a/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java index c19d819154f0..1f258bae2b1c 100644 --- a/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java +++ b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java @@ -3,12 +3,18 @@ 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())); // $hasTaintFlow=y $hasValueFlow=y sink(ObjectUtils.cloneIfPossible(taint())); // $hasTaintFlow=y $hasValueFlow=y sink(ObjectUtils.CONST(taint())); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.CONST_SHORT(IntSource.taint())); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.CONST_BYTE(IntSource.taint())); // $hasTaintFlow=y $hasValueFlow=y sink(ObjectUtils.defaultIfNull(taint(), null)); // $hasTaintFlow=y $hasValueFlow=y sink(ObjectUtils.defaultIfNull(null, taint())); // $hasTaintFlow=y $hasValueFlow=y sink(ObjectUtils.firstNonNull(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y From 6508a223c33271c1a06e653b5fa3f02bf19faafa Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 9 Mar 2021 15:54:31 +0000 Subject: [PATCH 3/5] Remove useless =y value specification from inline test expectations --- .../apache-commons-lang3/ObjectUtilsTest.java | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java index 1f258bae2b1c..d8067a992708 100644 --- a/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java +++ b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java @@ -10,32 +10,32 @@ private static class IntSource { void sink(Object o) {} void test() throws Exception { - sink(ObjectUtils.clone(taint())); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.cloneIfPossible(taint())); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.CONST(taint())); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.CONST_SHORT(IntSource.taint())); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.CONST_BYTE(IntSource.taint())); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.defaultIfNull(taint(), null)); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.defaultIfNull(null, taint())); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.firstNonNull(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.firstNonNull(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.firstNonNull(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.getIfNull(taint(), null)); // $hasTaintFlow=y $hasValueFlow=y - sink(ObjectUtils.max(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.max(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.max(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.median(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.median((String)null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.median((String)null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.min(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.min(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.min(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.mode(taint(), null, null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.mode(null, taint(), null)); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.mode(null, null, taint())); // $hasTaintFlow=y $MISSING:hasValueFlow=y - sink(ObjectUtils.requireNonEmpty(taint(), "message")); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.clone(taint())); // $hasTaintFlow $hasValueFlow + sink(ObjectUtils.cloneIfPossible(taint())); // $hasTaintFlow $hasValueFlow + sink(ObjectUtils.CONST(taint())); // $hasTaintFlow $hasValueFlow + sink(ObjectUtils.CONST_SHORT(IntSource.taint())); // $hasTaintFlow $hasValueFlow + sink(ObjectUtils.CONST_BYTE(IntSource.taint())); // $hasTaintFlow $hasValueFlow + sink(ObjectUtils.defaultIfNull(taint(), null)); // $hasTaintFlow $hasValueFlow + sink(ObjectUtils.defaultIfNull(null, taint())); // $hasTaintFlow $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)); // $hasTaintFlow $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")); // $hasTaintFlow $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())); // $hasTaintFlow=y $hasValueFlow=y + sink(ObjectUtils.toString(null, taint())); // $hasTaintFlow $hasValueFlow } } From 82a000bcca17739d6792c32db5d6781aae5fe20b Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 10 Mar 2021 09:38:12 +0000 Subject: [PATCH 4/5] Improve change note Co-authored-by: Marcono1234 --- java/change-notes/2021-03-05-commons-object-utils.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/change-notes/2021-03-05-commons-object-utils.md b/java/change-notes/2021-03-05-commons-object-utils.md index 96cf29d799cf..c219fb8f5bde 100644 --- a/java/change-notes/2021-03-05-commons-object-utils.md +++ b/java/change-notes/2021-03-05-commons-object-utils.md @@ -1,2 +1,2 @@ lgtm,codescanning -* Add models for ObjectUtils functions in the Apache Commons-Lang library. This may lead to more results from any dataflow query where traversal of ObjectUtils functions means we can now complete a path from a source of tainted data to a corresponding sink. +* 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. From 58d5c2c32d4456fc58236ffc7577e64f19f5db8b Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 12 Mar 2021 10:53:27 +0000 Subject: [PATCH 5/5] Abbreviate redundant value-flow / taint-flow tests --- .../apache-commons-lang3/ObjectUtilsTest.java | 20 +++++++++---------- .../frameworks/apache-commons-lang3/flow.ql | 1 + 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java index d8067a992708..d586b887290c 100644 --- a/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java +++ b/java/ql/test/library-tests/frameworks/apache-commons-lang3/ObjectUtilsTest.java @@ -10,17 +10,17 @@ private static class IntSource { void sink(Object o) {} void test() throws Exception { - sink(ObjectUtils.clone(taint())); // $hasTaintFlow $hasValueFlow - sink(ObjectUtils.cloneIfPossible(taint())); // $hasTaintFlow $hasValueFlow - sink(ObjectUtils.CONST(taint())); // $hasTaintFlow $hasValueFlow - sink(ObjectUtils.CONST_SHORT(IntSource.taint())); // $hasTaintFlow $hasValueFlow - sink(ObjectUtils.CONST_BYTE(IntSource.taint())); // $hasTaintFlow $hasValueFlow - sink(ObjectUtils.defaultIfNull(taint(), null)); // $hasTaintFlow $hasValueFlow - sink(ObjectUtils.defaultIfNull(null, taint())); // $hasTaintFlow $hasValueFlow + 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)); // $hasTaintFlow $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 @@ -33,9 +33,9 @@ void test() throws Exception { 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")); // $hasTaintFlow $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())); // $hasTaintFlow $hasValueFlow + sink(ObjectUtils.toString(null, taint())); // $hasValueFlow } } diff --git a/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql b/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql index 957e4dd2f740..b86d7b2611bc 100644 --- a/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql +++ b/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql @@ -34,6 +34,7 @@ class HasFlowTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasTaintFlow" and exists(DataFlow::Node src, DataFlow::Node sink, TaintFlowConf conf | conf.hasFlow(src, sink) | + not any(ValueFlowConf vconf).hasFlow(src, sink) and sink.getLocation() = location and element = sink.toString() and value = ""