From 55c72cebf2b23ba0788a60c3300fd1190ae73545 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 8 Jun 2021 12:43:23 +0100 Subject: [PATCH] Improve StringBuilder append chain tracking Previously this didn't catch the case of constructors chaining directly into appends, like `StringBuilder sb = new StringBuilder("1").append("2")` --- .../code/java/security/RequestForgery.qll | 108 ++++++++++++---- .../security/CWE-918/RequestForgery.expected | 119 ++++++++++-------- .../security/CWE-918/RequestForgery.java | 33 ++++- 3 files changed, 183 insertions(+), 77 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/RequestForgery.qll b/java/ql/src/semmle/code/java/security/RequestForgery.qll index bcbde5d1a78..8a0087bb9fb 100644 --- a/java/ql/src/semmle/code/java/security/RequestForgery.qll +++ b/java/ql/src/semmle/code/java/security/RequestForgery.qll @@ -248,20 +248,85 @@ private Expr getAHostnameSanitizingPrefix() { result.(AddExpr).getAnOperand() = getAHostnameSanitizingPrefix() } -private MethodAccess getNextAppend(MethodAccess append) { - result = any(StringBuilderVar sbv).getNextAppend(append) +private class StringBuilderAppend extends MethodAccess { + StringBuilderAppend() { + this.getMethod().getDeclaringType() instanceof StringBuildingType and + this.getMethod().hasName("append") + } } -private Expr getQualifier(MethodAccess e) { result = e.getQualifier() } +private class StringBuilderConstructorOrAppend extends Call { + StringBuilderConstructorOrAppend() { + this instanceof StringBuilderAppend or + this.(ClassInstanceExpr).getConstructedType() instanceof StringBuildingType + } +} -private MethodAccess getAChainedAppend(Expr e) { - ( - result.getQualifier() = e +private Expr getQualifier(Expr e) { result = e.(MethodAccess).getQualifier() } + +/** + * An extension of `StringBuilderVar` that also accounts for strings appended in StringBuilder/Buffer's constructor + * and in `append` calls chained onto the constructor call. + * + * The original `StringBuilderVar` doesn't care about these because it is designed to model taint, and + * in taint rules terms these are not needed, as the connection between construction, appends and the + * eventual `toString` is more obvious. + */ +private class StringBuilderVarExt extends StringBuilderVar { + /** + * Returns a first assignment after this StringBuilderVar is first assigned. + * + * For example, for `StringBuilder sbv = new StringBuilder("1").append("2"); sbv.append("3").append("4");` + * this returns the append of `"3"`. + */ + private StringBuilderAppend getAFirstAppendAfterAssignment() { + // + result = this.getAnAppend() and not result = this.getNextAppend(_) + } + + /** + * Gets the next `append` after `prev`, where `prev` is, perhaps after some more `append` or other + * chained calls, assigned to this `StringBuilderVar`. + */ + private StringBuilderAppend getNextAssignmentChainedAppend(StringBuilderConstructorOrAppend prev) { + getQualifier*(result) = this.getAnAssignedValue() and + result.getQualifier() = prev + } + + /** + * Get a constructor call or `append` call that contributes a string to this string builder. + */ + StringBuilderConstructorOrAppend getAConstructorOrAppend() { + exists(this.getNextAssignmentChainedAppend(result)) or + result = this.getAnAssignedValue() or + result = this.getAnAppend() + } + + /** + * Like `StringBuilderVar.getNextAppend`, except including appends and constructors directly + * assigned to this `StringBuilderVar`. + */ + private StringBuilderAppend getNextAppendIncludingAssignmentChains( + StringBuilderConstructorOrAppend prev + ) { + result = getNextAssignmentChainedAppend(prev) or - result.getQualifier() = getAChainedAppend(e) - ) and - result.getCallee().getDeclaringType() instanceof StringBuildingType and - result.getCallee().getName() = "append" + prev = this.getAnAssignedValue() and + result = this.getAFirstAppendAfterAssignment() + or + result = this.getNextAppend(prev) + } + + /** + * Implements `StringBuilderVarExt.getNextAppendIncludingAssignmentChains+(prev)`. + */ + StringBuilderAppend getSubsequentAppendIncludingAssignmentChains( + StringBuilderConstructorOrAppend prev + ) { + result = this.getNextAppendIncludingAssignmentChains(prev) or + result = + this.getSubsequentAppendIncludingAssignmentChains(this.getNextAppendIncludingAssignmentChains(prev)) + } } /** @@ -275,25 +340,14 @@ private class HostnameSanitizedExpr extends Expr { this = any(AddExpr add | add.getLeftOperand() = getAHostnameSanitizingPrefix()).getRightOperand() or - // Sanitize all appends to a StringBuilder that is initialized with a sanitizing prefix: - // (note imprecision: if the same StringBuilder/StringBuffer has more than one constructor call, - // this sanitizes all of its append calls, not just those that may follow the constructor). - exists(StringBuilderVar sbv, ConstructorCall constructor, Expr initializer | - initializer = sbv.getAnAssignedValue() and - constructor = getQualifier*(initializer) and - constructor.getArgument(0) = getAHostnameSanitizingPrefix() and - ( - this = sbv.getAnAppend().getArgument(0) - or - this = getAChainedAppend(constructor).getArgument(0) - ) - ) - or // Sanitize expressions that come after a sanitizing prefix in a sequence of StringBuilder operations: - exists(MethodAccess appendSanitizingConstant, MethodAccess subsequentAppend | - appendSanitizingConstant = any(StringBuilderVar v).getAnAppend() and + exists( + StringBuilderConstructorOrAppend appendSanitizingConstant, + StringBuilderAppend subsequentAppend, StringBuilderVarExt v + | + appendSanitizingConstant = v.getAConstructorOrAppend() and appendSanitizingConstant.getArgument(0) = getAHostnameSanitizingPrefix() and - getNextAppend*(appendSanitizingConstant) = subsequentAppend and + v.getSubsequentAppendIncludingAssignmentChains(appendSanitizingConstant) = subsequentAppend and this = subsequentAppend.getArgument(0) ) or diff --git a/java/ql/test/query-tests/security/CWE-918/RequestForgery.expected b/java/ql/test/query-tests/security/CWE-918/RequestForgery.expected index 170ae08cd72..8c50a69c1b7 100644 --- a/java/ql/test/query-tests/security/CWE-918/RequestForgery.expected +++ b/java/ql/test/query-tests/security/CWE-918/RequestForgery.expected @@ -14,27 +14,36 @@ edges | RequestForgery.java:19:23:19:58 | new URI(...) : URI | RequestForgery.java:22:52:22:54 | uri | | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:19:23:19:58 | new URI(...) : URI | | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:22:52:22:54 | uri | -| RequestForgery.java:59:33:59:63 | getParameter(...) : String | RequestForgery.java:60:59:60:77 | new URI(...) | -| RequestForgery.java:59:33:59:63 | getParameter(...) : String | RequestForgery.java:60:67:60:76 | unsafeUri3 : String | -| RequestForgery.java:60:67:60:76 | unsafeUri3 : String | RequestForgery.java:60:59:60:77 | new URI(...) | -| RequestForgery.java:63:49:63:79 | getParameter(...) : String | RequestForgery.java:64:59:64:77 | new URI(...) | -| RequestForgery.java:63:49:63:79 | getParameter(...) : String | RequestForgery.java:64:67:64:76 | unsafeUri4 : String | -| RequestForgery.java:64:67:64:76 | unsafeUri4 : String | RequestForgery.java:64:59:64:77 | new URI(...) | -| RequestForgery.java:68:31:68:61 | getParameter(...) : String | RequestForgery.java:69:59:69:88 | new URI(...) | -| RequestForgery.java:68:31:68:61 | getParameter(...) : String | RequestForgery.java:69:67:69:87 | toString(...) : String | -| RequestForgery.java:69:67:69:87 | toString(...) : String | RequestForgery.java:69:59:69:88 | new URI(...) | -| RequestForgery.java:72:73:72:103 | getParameter(...) : String | RequestForgery.java:73:59:73:77 | new URI(...) | -| RequestForgery.java:72:73:72:103 | getParameter(...) : String | RequestForgery.java:73:67:73:76 | unsafeUri6 : String | -| RequestForgery.java:73:67:73:76 | unsafeUri6 : String | RequestForgery.java:73:59:73:77 | new URI(...) | -| RequestForgery.java:76:56:76:86 | getParameter(...) : String | RequestForgery.java:77:59:77:77 | new URI(...) | -| RequestForgery.java:76:56:76:86 | getParameter(...) : String | RequestForgery.java:77:67:77:76 | unsafeUri7 : String | -| RequestForgery.java:77:67:77:76 | unsafeUri7 : String | RequestForgery.java:77:59:77:77 | new URI(...) | -| RequestForgery.java:80:55:80:85 | getParameter(...) : String | RequestForgery.java:81:59:81:77 | new URI(...) | -| RequestForgery.java:80:55:80:85 | getParameter(...) : String | RequestForgery.java:81:67:81:76 | unsafeUri8 : String | -| RequestForgery.java:81:67:81:76 | unsafeUri8 : String | RequestForgery.java:81:59:81:77 | new URI(...) | -| RequestForgery.java:84:33:84:63 | getParameter(...) : String | RequestForgery.java:85:59:85:77 | new URI(...) | -| RequestForgery.java:84:33:84:63 | getParameter(...) : String | RequestForgery.java:85:67:85:76 | unsafeUri9 : String | -| RequestForgery.java:85:67:85:76 | unsafeUri9 : String | RequestForgery.java:85:59:85:77 | new URI(...) | +| RequestForgery.java:75:33:75:63 | getParameter(...) : String | RequestForgery.java:76:59:76:77 | new URI(...) | +| RequestForgery.java:75:33:75:63 | getParameter(...) : String | RequestForgery.java:76:67:76:76 | unsafeUri3 : String | +| RequestForgery.java:76:67:76:76 | unsafeUri3 : String | RequestForgery.java:76:59:76:77 | new URI(...) | +| RequestForgery.java:79:49:79:79 | getParameter(...) : String | RequestForgery.java:80:59:80:77 | new URI(...) | +| RequestForgery.java:79:49:79:79 | getParameter(...) : String | RequestForgery.java:80:67:80:76 | unsafeUri4 : String | +| RequestForgery.java:80:67:80:76 | unsafeUri4 : String | RequestForgery.java:80:59:80:77 | new URI(...) | +| RequestForgery.java:84:31:84:61 | getParameter(...) : String | RequestForgery.java:85:59:85:88 | new URI(...) | +| RequestForgery.java:84:31:84:61 | getParameter(...) : String | RequestForgery.java:85:67:85:87 | toString(...) : String | +| RequestForgery.java:85:67:85:87 | toString(...) : String | RequestForgery.java:85:59:85:88 | new URI(...) | +| RequestForgery.java:88:58:88:86 | getParameter(...) : String | RequestForgery.java:90:60:90:89 | new URI(...) | +| RequestForgery.java:88:58:88:86 | getParameter(...) : String | RequestForgery.java:90:68:90:88 | toString(...) : String | +| RequestForgery.java:90:68:90:88 | toString(...) : String | RequestForgery.java:90:60:90:89 | new URI(...) | +| RequestForgery.java:93:60:93:88 | getParameter(...) : String | RequestForgery.java:95:60:95:90 | new URI(...) | +| RequestForgery.java:93:60:93:88 | getParameter(...) : String | RequestForgery.java:95:68:95:89 | toString(...) : String | +| RequestForgery.java:95:68:95:89 | toString(...) : String | RequestForgery.java:95:60:95:90 | new URI(...) | +| RequestForgery.java:98:77:98:105 | getParameter(...) : String | RequestForgery.java:100:60:100:90 | new URI(...) | +| RequestForgery.java:98:77:98:105 | getParameter(...) : String | RequestForgery.java:100:68:100:89 | toString(...) : String | +| RequestForgery.java:100:68:100:89 | toString(...) : String | RequestForgery.java:100:60:100:90 | new URI(...) | +| RequestForgery.java:103:73:103:103 | getParameter(...) : String | RequestForgery.java:104:59:104:77 | new URI(...) | +| RequestForgery.java:103:73:103:103 | getParameter(...) : String | RequestForgery.java:104:67:104:76 | unsafeUri6 : String | +| RequestForgery.java:104:67:104:76 | unsafeUri6 : String | RequestForgery.java:104:59:104:77 | new URI(...) | +| RequestForgery.java:107:56:107:86 | getParameter(...) : String | RequestForgery.java:108:59:108:77 | new URI(...) | +| RequestForgery.java:107:56:107:86 | getParameter(...) : String | RequestForgery.java:108:67:108:76 | unsafeUri7 : String | +| RequestForgery.java:108:67:108:76 | unsafeUri7 : String | RequestForgery.java:108:59:108:77 | new URI(...) | +| RequestForgery.java:111:55:111:85 | getParameter(...) : String | RequestForgery.java:112:59:112:77 | new URI(...) | +| RequestForgery.java:111:55:111:85 | getParameter(...) : String | RequestForgery.java:112:67:112:76 | unsafeUri8 : String | +| RequestForgery.java:112:67:112:76 | unsafeUri8 : String | RequestForgery.java:112:59:112:77 | new URI(...) | +| RequestForgery.java:115:33:115:63 | getParameter(...) : String | RequestForgery.java:116:59:116:77 | new URI(...) | +| RequestForgery.java:115:33:115:63 | getParameter(...) : String | RequestForgery.java:116:67:116:76 | unsafeUri9 : String | +| RequestForgery.java:116:67:116:76 | unsafeUri9 : String | RequestForgery.java:116:59:116:77 | new URI(...) | | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:32:47:32:67 | ... + ... | | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:37:43:37:56 | fooResourceUrl | | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:41:42:41:55 | fooResourceUrl | @@ -62,27 +71,36 @@ nodes | RequestForgery.java:19:23:19:58 | new URI(...) : URI | semmle.label | new URI(...) : URI | | RequestForgery.java:19:31:19:57 | getParameter(...) : String | semmle.label | getParameter(...) : String | | RequestForgery.java:22:52:22:54 | uri | semmle.label | uri | -| RequestForgery.java:59:33:59:63 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RequestForgery.java:60:59:60:77 | new URI(...) | semmle.label | new URI(...) | -| RequestForgery.java:60:67:60:76 | unsafeUri3 : String | semmle.label | unsafeUri3 : String | -| RequestForgery.java:63:49:63:79 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RequestForgery.java:64:59:64:77 | new URI(...) | semmle.label | new URI(...) | -| RequestForgery.java:64:67:64:76 | unsafeUri4 : String | semmle.label | unsafeUri4 : String | -| RequestForgery.java:68:31:68:61 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RequestForgery.java:69:59:69:88 | new URI(...) | semmle.label | new URI(...) | -| RequestForgery.java:69:67:69:87 | toString(...) : String | semmle.label | toString(...) : String | -| RequestForgery.java:72:73:72:103 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RequestForgery.java:73:59:73:77 | new URI(...) | semmle.label | new URI(...) | -| RequestForgery.java:73:67:73:76 | unsafeUri6 : String | semmle.label | unsafeUri6 : String | -| RequestForgery.java:76:56:76:86 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RequestForgery.java:77:59:77:77 | new URI(...) | semmle.label | new URI(...) | -| RequestForgery.java:77:67:77:76 | unsafeUri7 : String | semmle.label | unsafeUri7 : String | -| RequestForgery.java:80:55:80:85 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RequestForgery.java:81:59:81:77 | new URI(...) | semmle.label | new URI(...) | -| RequestForgery.java:81:67:81:76 | unsafeUri8 : String | semmle.label | unsafeUri8 : String | -| RequestForgery.java:84:33:84:63 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| RequestForgery.java:85:59:85:77 | new URI(...) | semmle.label | new URI(...) | -| RequestForgery.java:85:67:85:76 | unsafeUri9 : String | semmle.label | unsafeUri9 : String | +| RequestForgery.java:75:33:75:63 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:76:59:76:77 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:76:67:76:76 | unsafeUri3 : String | semmle.label | unsafeUri3 : String | +| RequestForgery.java:79:49:79:79 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:80:59:80:77 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:80:67:80:76 | unsafeUri4 : String | semmle.label | unsafeUri4 : String | +| RequestForgery.java:84:31:84:61 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:85:59:85:88 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:85:67:85:87 | toString(...) : String | semmle.label | toString(...) : String | +| RequestForgery.java:88:58:88:86 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:90:60:90:89 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:90:68:90:88 | toString(...) : String | semmle.label | toString(...) : String | +| RequestForgery.java:93:60:93:88 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:95:60:95:90 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:95:68:95:89 | toString(...) : String | semmle.label | toString(...) : String | +| RequestForgery.java:98:77:98:105 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:100:60:100:90 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:100:68:100:89 | toString(...) : String | semmle.label | toString(...) : String | +| RequestForgery.java:103:73:103:103 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:104:59:104:77 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:104:67:104:76 | unsafeUri6 : String | semmle.label | unsafeUri6 : String | +| RequestForgery.java:107:56:107:86 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:108:59:108:77 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:108:67:108:76 | unsafeUri7 : String | semmle.label | unsafeUri7 : String | +| RequestForgery.java:111:55:111:85 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:112:59:112:77 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:112:67:112:76 | unsafeUri8 : String | semmle.label | unsafeUri8 : String | +| RequestForgery.java:115:33:115:63 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| RequestForgery.java:116:59:116:77 | new URI(...) | semmle.label | new URI(...) | +| RequestForgery.java:116:67:116:76 | unsafeUri9 : String | semmle.label | unsafeUri9 : String | | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | semmle.label | getParameter(...) : String | | SpringSSRF.java:32:47:32:67 | ... + ... | semmle.label | ... + ... | | SpringSSRF.java:37:43:37:56 | fooResourceUrl | semmle.label | fooResourceUrl | @@ -104,13 +122,16 @@ nodes | RequestForgery2.java:67:43:67:45 | uri | RequestForgery2.java:23:27:23:53 | getParameter(...) : String | RequestForgery2.java:67:43:67:45 | uri | Potential server-side request forgery due to $@. | RequestForgery2.java:23:27:23:53 | getParameter(...) | a user-provided value | | RequestForgery2.java:69:29:69:32 | uri2 | RequestForgery2.java:23:27:23:53 | getParameter(...) : String | RequestForgery2.java:69:29:69:32 | uri2 | Potential server-side request forgery due to $@. | RequestForgery2.java:23:27:23:53 | getParameter(...) | a user-provided value | | RequestForgery.java:22:52:22:54 | uri | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:22:52:22:54 | uri | Potential server-side request forgery due to $@. | RequestForgery.java:19:31:19:57 | getParameter(...) | a user-provided value | -| RequestForgery.java:60:59:60:77 | new URI(...) | RequestForgery.java:59:33:59:63 | getParameter(...) : String | RequestForgery.java:60:59:60:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:59:33:59:63 | getParameter(...) | a user-provided value | -| RequestForgery.java:64:59:64:77 | new URI(...) | RequestForgery.java:63:49:63:79 | getParameter(...) : String | RequestForgery.java:64:59:64:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:63:49:63:79 | getParameter(...) | a user-provided value | -| RequestForgery.java:69:59:69:88 | new URI(...) | RequestForgery.java:68:31:68:61 | getParameter(...) : String | RequestForgery.java:69:59:69:88 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:68:31:68:61 | getParameter(...) | a user-provided value | -| RequestForgery.java:73:59:73:77 | new URI(...) | RequestForgery.java:72:73:72:103 | getParameter(...) : String | RequestForgery.java:73:59:73:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:72:73:72:103 | getParameter(...) | a user-provided value | -| RequestForgery.java:77:59:77:77 | new URI(...) | RequestForgery.java:76:56:76:86 | getParameter(...) : String | RequestForgery.java:77:59:77:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:76:56:76:86 | getParameter(...) | a user-provided value | -| RequestForgery.java:81:59:81:77 | new URI(...) | RequestForgery.java:80:55:80:85 | getParameter(...) : String | RequestForgery.java:81:59:81:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:80:55:80:85 | getParameter(...) | a user-provided value | -| RequestForgery.java:85:59:85:77 | new URI(...) | RequestForgery.java:84:33:84:63 | getParameter(...) : String | RequestForgery.java:85:59:85:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:84:33:84:63 | getParameter(...) | a user-provided value | +| RequestForgery.java:76:59:76:77 | new URI(...) | RequestForgery.java:75:33:75:63 | getParameter(...) : String | RequestForgery.java:76:59:76:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:75:33:75:63 | getParameter(...) | a user-provided value | +| RequestForgery.java:80:59:80:77 | new URI(...) | RequestForgery.java:79:49:79:79 | getParameter(...) : String | RequestForgery.java:80:59:80:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:79:49:79:79 | getParameter(...) | a user-provided value | +| RequestForgery.java:85:59:85:88 | new URI(...) | RequestForgery.java:84:31:84:61 | getParameter(...) : String | RequestForgery.java:85:59:85:88 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:84:31:84:61 | getParameter(...) | a user-provided value | +| RequestForgery.java:90:60:90:89 | new URI(...) | RequestForgery.java:88:58:88:86 | getParameter(...) : String | RequestForgery.java:90:60:90:89 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:88:58:88:86 | getParameter(...) | a user-provided value | +| RequestForgery.java:95:60:95:90 | new URI(...) | RequestForgery.java:93:60:93:88 | getParameter(...) : String | RequestForgery.java:95:60:95:90 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:93:60:93:88 | getParameter(...) | a user-provided value | +| RequestForgery.java:100:60:100:90 | new URI(...) | RequestForgery.java:98:77:98:105 | getParameter(...) : String | RequestForgery.java:100:60:100:90 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:98:77:98:105 | getParameter(...) | a user-provided value | +| RequestForgery.java:104:59:104:77 | new URI(...) | RequestForgery.java:103:73:103:103 | getParameter(...) : String | RequestForgery.java:104:59:104:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:103:73:103:103 | getParameter(...) | a user-provided value | +| RequestForgery.java:108:59:108:77 | new URI(...) | RequestForgery.java:107:56:107:86 | getParameter(...) : String | RequestForgery.java:108:59:108:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:107:56:107:86 | getParameter(...) | a user-provided value | +| RequestForgery.java:112:59:112:77 | new URI(...) | RequestForgery.java:111:55:111:85 | getParameter(...) : String | RequestForgery.java:112:59:112:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:111:55:111:85 | getParameter(...) | a user-provided value | +| RequestForgery.java:116:59:116:77 | new URI(...) | RequestForgery.java:115:33:115:63 | getParameter(...) : String | RequestForgery.java:116:59:116:77 | new URI(...) | Potential server-side request forgery due to $@. | RequestForgery.java:115:33:115:63 | getParameter(...) | a user-provided value | | SpringSSRF.java:32:47:32:67 | ... + ... | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:32:47:32:67 | ... + ... | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value | | SpringSSRF.java:37:43:37:56 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:37:43:37:56 | fooResourceUrl | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value | | SpringSSRF.java:41:42:41:55 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:41:42:41:55 | fooResourceUrl | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value | diff --git a/java/ql/test/query-tests/security/CWE-918/RequestForgery.java b/java/ql/test/query-tests/security/CWE-918/RequestForgery.java index d4febb9b94b..f298ab31d1b 100644 --- a/java/ql/test/query-tests/security/CWE-918/RequestForgery.java +++ b/java/ql/test/query-tests/security/CWE-918/RequestForgery.java @@ -24,7 +24,8 @@ public class RequestForgery extends HttpServlet { // GOOD: sanitisation by concatenation with a prefix that prevents targeting an arbitrary host. // We test a few different ways of sanitisation: via string conctentation (perhaps nested), - // via a stringbuilder and via String.format. + // via a stringbuilder (for which we consider appends done in the constructor, chained onto + // the constructor and applied in subsequent statements) and via String.format. String safeUri3 = "https://example.com/" + request.getParameter("uri3"); HttpRequest r3 = HttpRequest.newBuilder(new URI(safeUri3)).build(); client.send(r3, null); @@ -38,6 +39,21 @@ public class RequestForgery extends HttpServlet { HttpRequest r5 = HttpRequest.newBuilder(new URI(safeUri5.toString())).build(); client.send(r5, null); + StringBuilder safeUri5a = new StringBuilder("https://example.com/"); + safeUri5a.append(request.getParameter("uri5a")); + HttpRequest r5a = HttpRequest.newBuilder(new URI(safeUri5a.toString())).build(); + client.send(r5a, null); + + StringBuilder safeUri5b = (new StringBuilder("https://example.com/")).append("dir/"); + safeUri5b.append(request.getParameter("uri5b")); + HttpRequest r5b = HttpRequest.newBuilder(new URI(safeUri5b.toString())).build(); + client.send(r5b, null); + + StringBuilder safeUri5c = (new StringBuilder("prefix")).append("https://example.com/dir/"); + safeUri5c.append(request.getParameter("uri5c")); + HttpRequest r5c = HttpRequest.newBuilder(new URI(safeUri5c.toString())).build(); + client.send(r5c, null); + String safeUri6 = String.format("https://example.com/%s", request.getParameter("uri6")); HttpRequest r6 = HttpRequest.newBuilder(new URI(safeUri6)).build(); client.send(r6, null); @@ -69,6 +85,21 @@ public class RequestForgery extends HttpServlet { HttpRequest unsafer5 = HttpRequest.newBuilder(new URI(unsafeUri5.toString())).build(); client.send(unsafer5, null); + StringBuilder unafeUri5a = new StringBuilder(request.getParameter("uri5a")); + unafeUri5a.append("https://example.com/"); + HttpRequest unsafer5a = HttpRequest.newBuilder(new URI(unafeUri5a.toString())).build(); + client.send(unsafer5a, null); + + StringBuilder unsafeUri5b = (new StringBuilder(request.getParameter("uri5b"))).append("dir/"); + unsafeUri5b.append("https://example.com/"); + HttpRequest unsafer5b = HttpRequest.newBuilder(new URI(unsafeUri5b.toString())).build(); + client.send(unsafer5b, null); + + StringBuilder unsafeUri5c = (new StringBuilder("https")).append(request.getParameter("uri5c")); + unsafeUri5c.append("://example.com/dir/"); + HttpRequest unsafer5c = HttpRequest.newBuilder(new URI(unsafeUri5c.toString())).build(); + client.send(unsafer5c, null); + String unsafeUri6 = String.format("%shttps://example.com/", request.getParameter("baduri6")); HttpRequest unsafer6 = HttpRequest.newBuilder(new URI(unsafeUri6)).build(); client.send(unsafer6, null);