From 58d2d5d14e43e2238d56bde79367e31441cf1b57 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 4 Sep 2021 21:09:36 +0200 Subject: [PATCH] Java: Replace incorrect usage of `Literal.getLiteral()` --- java/ql/lib/semmle/code/java/Annotation.qll | 6 +-- java/ql/lib/semmle/code/java/Expr.qll | 21 ++++++++-- .../lib/semmle/code/java/JDKAnnotations.qll | 5 +-- .../code/java/security/RelativePaths.qll | 2 +- .../Collections/WriteOnlyContainer.ql | 2 +- .../CWE/CWE-327/BrokenCryptoAlgorithm.ql | 8 ++-- .../CWE/CWE-327/MaybeBrokenCryptoAlgorithm.ql | 8 ++-- .../Magic Constants/MagicConstants.qll | 42 +++++++------------ .../test/library-tests/Encryption/insecure.ql | 2 +- .../test/library-tests/Encryption/secure.ql | 2 +- 10 files changed, 47 insertions(+), 51 deletions(-) diff --git a/java/ql/lib/semmle/code/java/Annotation.qll b/java/ql/lib/semmle/code/java/Annotation.qll index aa114589a2d..2ee7c6403b4 100755 --- a/java/ql/lib/semmle/code/java/Annotation.qll +++ b/java/ql/lib/semmle/code/java/Annotation.qll @@ -118,11 +118,7 @@ class Annotatable extends Element { * annotation attached to it for the specified `category`. */ predicate suppressesWarningsAbout(string category) { - exists(string withQuotes | - withQuotes = getAnAnnotation().(SuppressWarningsAnnotation).getASuppressedWarning() - | - category = withQuotes.substring(1, withQuotes.length() - 1) - ) + category = getAnAnnotation().(SuppressWarningsAnnotation).getASuppressedWarning() or this.(Member).getDeclaringType().suppressesWarningsAbout(category) or diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 4c6ea652f2a..d4b6ca5d425 100755 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -599,10 +599,23 @@ class AssignURShiftExpr extends AssignOp, @assignurshiftexpr { /** A common super-class to represent constant literals. */ class Literal extends Expr, @literal { - /** Gets a string representation of this literal. */ + /** + * Gets a string representation of this literal as it appeared + * in the source code. + * + * **Important:** Unless a query explicitly wants to check how + * a literal was written in the source code, the predicate + * `getValue()` (or value predicates of subclasses) should be + * used instead. For example for the integer literal `0x7fff_ffff` + * the result of `getLiteral()` would be `0x7fff_ffff`, while + * the result of `getValue()` would be `2147483647`. + */ string getLiteral() { namestrings(result, _, this) } - /** Gets a string representation of the value of this literal. */ + /** + * Gets a string representation of the value this literal + * represents. + */ string getValue() { namestrings(_, result, this) } /** Gets a printable representation of this expression. */ @@ -619,9 +632,9 @@ class Literal extends Expr, @literal { class BooleanLiteral extends Literal, @booleanliteral { /** Gets the boolean representation of this literal. */ boolean getBooleanValue() { - result = true and getLiteral() = "true" + result = true and getValue() = "true" or - result = false and getLiteral() = "false" + result = false and getValue() = "false" } override string getAPrimaryQlClass() { result = "BooleanLiteral" } diff --git a/java/ql/lib/semmle/code/java/JDKAnnotations.qll b/java/ql/lib/semmle/code/java/JDKAnnotations.qll index 652b1b48f14..49776a570f2 100644 --- a/java/ql/lib/semmle/code/java/JDKAnnotations.qll +++ b/java/ql/lib/semmle/code/java/JDKAnnotations.qll @@ -25,10 +25,7 @@ class SuppressWarningsAnnotation extends Annotation { } /** Gets the name of a warning suppressed by this annotation. */ - string getASuppressedWarning() { - result = this.getAValue().(StringLiteral).getLiteral() or - result = this.getAValue().(ArrayInit).getAnInit().(StringLiteral).getLiteral() - } + string getASuppressedWarning() { result = getASuppressedWarningLiteral().getRepresentedString() } } /** A `@Target` annotation. */ diff --git a/java/ql/lib/semmle/code/java/security/RelativePaths.qll b/java/ql/lib/semmle/code/java/security/RelativePaths.qll index 908a02332bc..34ffcc5bb04 100644 --- a/java/ql/lib/semmle/code/java/security/RelativePaths.qll +++ b/java/ql/lib/semmle/code/java/security/RelativePaths.qll @@ -5,7 +5,7 @@ import java * An element that starts with a relative path. */ predicate relativePath(Element tree, string command) { - exists(StringLiteral lit, string text | tree = lit and text = lit.getLiteral() | + exists(StringLiteral lit, string text | tree = lit and text = lit.getRepresentedString() | text != "" and ( text.regexpMatch("[^/\\\\ \t]*") or diff --git a/java/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql b/java/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql index 689d223c72b..8002c8d6841 100644 --- a/java/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql +++ b/java/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql @@ -24,7 +24,7 @@ where // Exclude fields that may be read from reflectively. not reflectivelyRead(v) and // Exclude fields annotated with `@SuppressWarnings("unused")`. - not v.getAnAnnotation().(SuppressWarningsAnnotation).getASuppressedWarning() = "\"unused\"" and + not v.getAnAnnotation().(SuppressWarningsAnnotation).getASuppressedWarning() = "unused" and // Exclude fields with relevant Lombok annotations. not v instanceof LombokGetterAnnotatedField and // Every access to `v` is either... diff --git a/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql b/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql index 5c8a5b51df0..230d33c316b 100644 --- a/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql +++ b/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql @@ -17,14 +17,14 @@ import DataFlow import PathGraph private class ShortStringLiteral extends StringLiteral { - ShortStringLiteral() { getLiteral().length() < 100 } + ShortStringLiteral() { getRepresentedString().length() < 100 } } class BrokenAlgoLiteral extends ShortStringLiteral { BrokenAlgoLiteral() { - getValue().regexpMatch(getInsecureAlgorithmRegex()) and + getRepresentedString().regexpMatch(getInsecureAlgorithmRegex()) and // Exclude German and French sentences. - not getValue().regexpMatch(".*\\p{IsLowercase} des \\p{IsLetter}.*") + not getRepresentedString().regexpMatch(".*\\p{IsLowercase} des \\p{IsLetter}.*") } } @@ -48,4 +48,4 @@ where source.getNode().asExpr() = s and conf.hasFlowPath(source, sink) select c, source, sink, "Cryptographic algorithm $@ is weak and should not be used.", s, - s.getLiteral() + s.getRepresentedString() diff --git a/java/ql/src/Security/CWE/CWE-327/MaybeBrokenCryptoAlgorithm.ql b/java/ql/src/Security/CWE/CWE-327/MaybeBrokenCryptoAlgorithm.ql index 1b99c53561b..c153426e2f4 100644 --- a/java/ql/src/Security/CWE/CWE-327/MaybeBrokenCryptoAlgorithm.ql +++ b/java/ql/src/Security/CWE/CWE-327/MaybeBrokenCryptoAlgorithm.ql @@ -18,14 +18,14 @@ import semmle.code.java.dispatch.VirtualDispatch import PathGraph private class ShortStringLiteral extends StringLiteral { - ShortStringLiteral() { getLiteral().length() < 100 } + ShortStringLiteral() { getRepresentedString().length() < 100 } } class InsecureAlgoLiteral extends ShortStringLiteral { InsecureAlgoLiteral() { // Algorithm identifiers should be at least two characters. - getValue().length() > 1 and - exists(string s | s = getLiteral() | + getRepresentedString().length() > 1 and + exists(string s | s = getRepresentedString() | not s.regexpMatch(getSecureAlgorithmRegex()) and // Exclude results covered by another query. not s.regexpMatch(getInsecureAlgorithmRegex()) @@ -72,4 +72,4 @@ where conf.hasFlowPath(source, sink) select c, source, sink, "Cryptographic algorithm $@ may not be secure, consider using a different algorithm.", s, - s.getLiteral() + s.getRepresentedString() diff --git a/java/ql/src/Violations of Best Practice/Magic Constants/MagicConstants.qll b/java/ql/src/Violations of Best Practice/Magic Constants/MagicConstants.qll index 4b5f265754f..4ae435cd2eb 100644 --- a/java/ql/src/Violations of Best Practice/Magic Constants/MagicConstants.qll +++ b/java/ql/src/Violations of Best Practice/Magic Constants/MagicConstants.qll @@ -186,24 +186,21 @@ private predicate trivialIntValue(string s) { exists(string pos | trivialPositiveIntValue(pos) and s = "-" + pos) } -private predicate intTrivial(Literal lit) { - exists(string v | trivialIntValue(v) and v = lit.getLiteral()) +private predicate intTrivial(IntegerLiteral lit) { + // Remove all `_` from literal, if any (e.g. `1_000_000`) + exists(string v | trivialIntValue(v) and v = lit.getLiteral().replaceAll("_", "")) } -private predicate longTrivial(Literal lit) { - exists(string v | trivialIntValue(v) and v + "L" = lit.getLiteral()) +private predicate longTrivial(LongLiteral lit) { + exists(string v | + trivialIntValue(v) and + // Remove all `_` from literal, if any (e.g. `1_000_000L`) + v + ["l", "L"] = lit.getLiteral().replaceAll("_", "") + ) } private predicate powerOfTen(float f) { - f = 10 or - f = 100 or - f = 1000 or - f = 10000 or - f = 100000 or - f = 1000000 or - f = 10000000 or - f = 100000000 or - f = 1000000000 + f = [10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000] } private predicate floatTrivial(Literal lit) { @@ -244,7 +241,7 @@ private predicate literalIsConstantInitializer(Literal literal, Field f) { } private predicate nonTrivialValue(string value, Literal literal, string context) { - value = literal.getLiteral() and + value = literal.getValue() and not trivial(literal) and not literalIsConstantInitializer(literal, _) and not literal.getParent*() instanceof ArrayInit and @@ -259,7 +256,7 @@ private predicate valueOccurrenceCount(string value, int n, string context) { private predicate occurenceCount(Literal lit, string value, int n, string context) { valueOccurrenceCount(value, n, context) and - value = lit.getLiteral() and + value = lit.getValue() and nonTrivialValue(_, lit, context) } @@ -296,14 +293,7 @@ private predicate firstOccurrence(Literal lit, string value, string context, int ) } -predicate isNumber(Literal lit) { - lit.getType().getName() = "char" or - lit.getType().getName() = "short" or - lit.getType().getName() = "int" or - lit.getType().getName() = "long" or - lit.getType().getName() = "float" or - lit.getType().getName() = "double" -} +predicate isNumber(Literal lit) { lit.getType() instanceof NumericOrCharType } predicate magicConstant(Literal e, string msg) { exists(string value, int n, string context | @@ -320,7 +310,7 @@ predicate magicConstant(Literal e, string msg) { private predicate relevantField(Field f, string value) { exists(Literal lit | - not trivial(lit) and value = lit.getLiteral() and literalIsConstantInitializer(lit, f) + not trivial(lit) and value = lit.getValue() and literalIsConstantInitializer(lit, f) ) } @@ -344,7 +334,7 @@ private predicate candidateConstantForLiteral( exists(Literal initLiteral | literalIsConstantInitializer(initLiteral, constField) and exists(string value | - value = initLiteral.getLiteral() and + value = initLiteral.getValue() and nonTrivialValue(value, magicLiteral, context) and fieldUsedInContext(constField, context) ) and @@ -401,7 +391,7 @@ predicate literalInsteadOfConstant( exists(string context | canUseFieldInsteadOfLiteral(constField, magicLiteral, context) and message = - "Literal value '" + magicLiteral.getLiteral() + "' used " + " in a call to " + context + + "Literal value '" + magicLiteral.getValue() + "' used " + " in a call to " + context + "; consider using the defined constant $@." and linkText = constField.getName() and ( diff --git a/java/ql/test/library-tests/Encryption/insecure.ql b/java/ql/test/library-tests/Encryption/insecure.ql index 86e7adcfba0..549997ad804 100644 --- a/java/ql/test/library-tests/Encryption/insecure.ql +++ b/java/ql/test/library-tests/Encryption/insecure.ql @@ -2,5 +2,5 @@ import default import semmle.code.java.security.Encryption from StringLiteral s -where s.getLiteral().regexpMatch(getInsecureAlgorithmRegex()) +where s.getRepresentedString().regexpMatch(getInsecureAlgorithmRegex()) select s diff --git a/java/ql/test/library-tests/Encryption/secure.ql b/java/ql/test/library-tests/Encryption/secure.ql index 16b752713a4..009a99b82f7 100644 --- a/java/ql/test/library-tests/Encryption/secure.ql +++ b/java/ql/test/library-tests/Encryption/secure.ql @@ -2,5 +2,5 @@ import default import semmle.code.java.security.Encryption from StringLiteral s -where s.getLiteral().regexpMatch(getSecureAlgorithmRegex()) +where s.getRepresentedString().regexpMatch(getSecureAlgorithmRegex()) select s