CodeQL 2.26.2, codeql/java-queries security-extended, JDK 8 source.
A command-injection flow that passes through a single String.valueOf call
on a String argument is not reported:
public class Repro { public void handle(javax.servlet.http.HttpServletRequest req) throws java.io.IOException { String data = req.getParameter("cmd"); // source String s1 = String.valueOf(data); // taint lost here Runtime.getRuntime().exec(s1); // sink, not reported }}Passing data directly to exec is reported as expected, so both the source
and the sink are recognised.
java.lang.model.yml in the shipped codeql/java-all pack appears to
explain this precisely. The summaryModel section propagates taint for
four valueOf signatures:
["java.lang","String",False,"valueOf","(char)", "","Argument[0]","ReturnValue","taint","manual"]
["java.lang","String",False,"valueOf","(char[])", "","Argument[0]","ReturnValue","taint","manual"]
["java.lang","String",False,"valueOf","(char[],int,int)", "","Argument[0]","ReturnValue","taint","manual"]
["java.lang","String",False,"valueOf","(CharSequence)", "","Argument[0]","ReturnValue","taint","manual"]
and the neutralModel section declares one of the remaining signatures to
have no flow at all:
["java.lang","String","valueOf","(Object)","summary","manual"]
Two things follow. First, java.lang.String has no valueOf(String)
overload, so a String-typed argument selects valueOf(Object) — the
neutral one. Second, java.lang.String has no valueOf(CharSequence)
overload either. javap java.lang.String on both JDK 8 and JDK 21 lists
exactly nine: boolean, char, char[], char[]+int+int, double, float, int,
long, Object. So the (CharSequence) row matches no method, and no
propagating valueOf model applies to a string-typed argument at all.
This is not an inference from the model file; it is measured. The six cases
below were compiled and analysed together, and javap -c was used to confirm
what each call resolves to:
| case |
call site resolves to |
result |
| exec(data) |
— |
detected |
| exec(new StringBuilder(data).toString()) |
— |
detected |
| exec(String.valueOf(data)) |
valueOf(Object) |
silent |
| exec(String.valueOf(sb)), sb a StringBuilder |
valueOf(Object) |
silent |
| CharSequence cs = data; exec(String.valueOf(cs)) |
valueOf(Object) |
silent |
| exec(String.valueOf(data.toCharArray())) |
valueOf(char[]) |
detected |
The two controls confirm the source and sink are recognised and that other
java.lang summary rows fire normally. valueOf(char[]) — a row whose
signature does exist — propagates. Every attempt to reach the
(CharSequence) row, including declaring the local as CharSequence,
compiles to invokestatic String.valueOf:(Ljava/lang/Object;) instead:
Java overload resolution has no (CharSequence) candidate to select.
I am not asking for the neutral model to be removed. The reason for it is
on the record and I think it is sound: valueOf(Object) calls toString()
on an arbitrary receiver, a blanket taint summary would be wrong for objects
whose toString() does not expose the tainted state, and #6382 flagged the
FP-rate risk explicitly. The test cases do not measure that risk and so
have nothing to say about it.
The narrower question is whether the (CharSequence) row was intended to do
the work described in #15280. If it was, it is not doing it, and one of these
would close the gap without a blanket summary:
a row conditioned on the argument's static type, so that
valueOf(Object) propagates when the argument is a CharSequence
(where the call is the identity or a documented toString()), or
removing the (CharSequence) row, so the model file no longer suggests a
coverage that does not exist.
One thing I could not check. The codeql/java-all pack serves Kotlin as
well as Java, and I have only tested from Java. If the (CharSequence) row
exists to cover a Kotlin call shape rather than a Java one, that would
explain why it is written the way it is — though I am not aware of a Kotlin
declaration it would match either, since Kotlin calls the same nine
java.lang.String statics. Either way the Java-side conclusion is unchanged,
and if the row is indeed Kotlin-only then Java has no compensating
propagating model for String.valueOf at all, which makes the gap wider
rather than narrower. Happy to be corrected on this point.
Scale of the observation: on a generated suite of 60 programs chaining
String.valueOf 1 to 30 times, CodeQL produces zero results of any rule
anywhere in that package — while the package is present in the SARIF
artifact list, so it was extracted and analysed. The same suite built from
new String(s.toCharArray()) is handled correctly at all 30 lengths, and a
control suite replacing the library call with a user-defined
static String pass(String s) { return s; } is also correct at all 30
lengths. That isolates the behaviour to this one overload rather than to
chain length or to the number of dataflow steps.
CodeQL 2.26.2,
codeql/java-queriessecurity-extended, JDK 8 source.A command-injection flow that passes through a single
String.valueOfcallon a
Stringargument is not reported:Passing
datadirectly toexecis reported as expected, so both the sourceand the sink are recognised.
java.lang.model.ymlin the shippedcodeql/java-allpack appears toexplain this precisely. The
summaryModelsection propagates taint forfour
valueOfsignatures:and the
neutralModelsection declares one of the remaining signatures tohave no flow at all:
["java.lang","String","valueOf","(Object)","summary","manual"]Two things follow. First,
java.lang.Stringhas novalueOf(String)overload, so a
String-typed argument selectsvalueOf(Object)— theneutral one. Second,
java.lang.Stringhas novalueOf(CharSequence)overload either.
javap java.lang.Stringon both JDK 8 and JDK 21 listsexactly nine:
boolean, char, char[], char[]+int+int, double, float, int,. So thelong, Object
(CharSequence)row matches no method, and nopropagating
valueOfmodel applies to a string-typed argument at all.This is not an inference from the model file; it is measured. The six cases
below were compiled and analysed together, and
javap -cwas used to confirmwhat each call resolves to:
The two controls confirm the source and sink are recognised and that other
java.langsummary rows fire normally.valueOf(char[])— a row whose signature does exist — propagates. Every attempt to reach the(CharSequence)row, including declaring the local asCharSequence, compiles toinvokestatic String.valueOf:(Ljava/lang/Object;)instead: Java overload resolution has no(CharSequence)candidate to select.I am not asking for the neutral model to be removed. The reason for it is on the record and I think it is sound:
valueOf(Object)callstoString()on an arbitrary receiver, a blanket taint summary would be wrong for objects whosetoString()does not expose the tainted state, and #6382 flagged the FP-rate risk explicitly. The test cases do not measure that risk and so have nothing to say about it.The narrower question is whether the
(CharSequence)row was intended to do the work described in #15280. If it was, it is not doing it, and one of these would close the gap without a blanket summary:a row conditioned on the argument's static type, so that
valueOf(Object)propagates when the argument is aCharSequence(where the call is the identity or a documentedtoString()), orremoving the
(CharSequence)row, so the model file no longer suggests a coverage that does not exist.One thing I could not check. The
codeql/java-allpack serves Kotlin as well as Java, and I have only tested from Java. If the(CharSequence)row exists to cover a Kotlin call shape rather than a Java one, that would explain why it is written the way it is — though I am not aware of a Kotlin declaration it would match either, since Kotlin calls the same ninejava.lang.Stringstatics. Either way the Java-side conclusion is unchanged, and if the row is indeed Kotlin-only then Java has no compensating propagating model forString.valueOfat all, which makes the gap wider rather than narrower. Happy to be corrected on this point.Scale of the observation: on a generated suite of 60 programs chaining
String.valueOf1 to 30 times, CodeQL produces zero results of any rule anywhere in that package — while the package is present in the SARIF artifact list, so it was extracted and analysed. The same suite built fromnew String(s.toCharArray())is handled correctly at all 30 lengths, and a control suite replacing the library call with a user-definedstatic String pass(String s) { return s; }is also correct at all 30 lengths. That isolates the behaviour to this one overload rather than to chain length or to the number of dataflow steps.