From 5ad69612587a2a2adc6a20355e0f6d111cf61877 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 17 Oct 2023 18:28:49 -0700 Subject: [PATCH 01/36] Add JSON parser --- .../javascript/frameworks/ui5/JsonParser.qll | 501 ++++++++++++++++++ 1 file changed, 501 insertions(+) create mode 100644 javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll new file mode 100644 index 00000000..4d42b028 --- /dev/null +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -0,0 +1,501 @@ + +signature class JsonStringReader { + string read(); +} + +module JsonParser { + private newtype TJsonToken = + MkLeftBracketToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + begin = source.indexOf("{") and + begin = end and + value = "{" + } or + MkRightBracketToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + begin = source.indexOf("}") and + begin = end and + value = "}" + } or + MkLeftSquareBracketToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + begin = source.indexOf("[") and + begin = end and + value = "[" + } or + MkRightSquareBracketToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + begin = source.indexOf("]") and + begin = end and + value = "]" + } or + MkWhiteSpaceToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + value = source.regexpFind("[\\s\\v\\h]", _, begin) and + begin + value.length() - 1 = end + } or + MkCommaToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + begin = source.indexOf(",") and + begin = end and + value = "," + } or + MkColonToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + begin = source.indexOf(":") and + begin = end and + value = ":" + } or + MkNumberToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + value = source.regexpFind("-?[1-9]\\d*(\\.\\d+)?((e|E)?(\\+|-)?\\d+)?", _, begin) and + begin + value.length() - 1 = end + } or + MkStringToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + value = source.regexpFind("\"[^\"]*\"", _, begin) and + begin + value.length() - 1 = end + } or + MkTrueToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + value = source.regexpFind("true", _, begin) and + begin + value.length() - 1 = end + } or + MkFalseToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + value = source.regexpFind("false", _, begin) and + begin + value.length() - 1 = end + } or + MkNullToken(int begin, int end, string value, string source) { + source = any(Reader r).read() and + value = source.regexpFind("null", _, begin) and + begin + value.length() - 1 = end + } + + private class JsonToken extends TJsonToken { + int getBegin() { + this = MkLeftBracketToken(result, _, _, _) + or + this = MkRightBracketToken(result, _, _, _) + or + this = MkLeftSquareBracketToken(result, _, _, _) + or + this = MkRightSquareBracketToken(result, _, _, _) + or + this = MkWhiteSpaceToken(result, _, _, _) + or + this = MkCommaToken(result, _, _, _) + or + this = MkColonToken(result, _, _, _) + or + this = MkNumberToken(result, _, _, _) + or + this = MkStringToken(result, _, _, _) + or + this = MkTrueToken(result, _, _, _) + or + this = MkFalseToken(result, _, _, _) + or + this = MkNullToken(result, _, _, _) + } + + int getEnd() { + this = MkLeftBracketToken(_, result, _, _) + or + this = MkRightBracketToken(_, result, _, _) + or + this = MkLeftSquareBracketToken(_, result, _, _) + or + this = MkRightSquareBracketToken(_, result, _, _) + or + this = MkWhiteSpaceToken(_, result, _, _) + or + this = MkCommaToken(_, result, _, _) + or + this = MkColonToken(_, result, _, _) + or + this = MkNumberToken(_, result, _, _) + or + this = MkStringToken(_, result, _, _) + or + this = MkTrueToken(_, result, _, _) + or + this = MkFalseToken(_, result, _, _) + or + this = MkNullToken(_, result, _, _) + } + + string getValue() { + this = MkLeftBracketToken(_, _, result, _) + or + this = MkRightBracketToken(_, _, result, _) + or + this = MkLeftSquareBracketToken(_, _, result, _) + or + this = MkRightSquareBracketToken(_, _, result, _) + or + this = MkWhiteSpaceToken(_, _, result, _) + or + this = MkCommaToken(_, _, result, _) + or + this = MkColonToken(_, _, result, _) + or + this = MkNumberToken(_, _, result, _) + or + this = MkStringToken(_, _, result, _) + or + this = MkTrueToken(_, _, result, _) + or + this = MkFalseToken(_, _, result, _) + or + this = MkNullToken(_, _, result, _) + } + + string toString() { + this = MkLeftBracketToken(_, _, _, _) and result = "leftbracket" + or + this = MkRightBracketToken(_, _, _, _) and result = "rightbracket" + or + this = MkLeftSquareBracketToken(_, _, _, _) and result = "leftsquarebracket" + or + this = MkRightSquareBracketToken(_, _, _, _) and result = "rightsquarebracket" + or + this = MkWhiteSpaceToken(_, _, _, _) and result = "whitespace" + or + this = MkCommaToken(_, _, _, _) and result = "comma" + or + this = MkColonToken(_, _, _, _) and result = "colon" + or + exists(string val | this = MkNumberToken(_, _, val, _) and result = "integer=" + val) + or + exists(string val | this = MkStringToken(_, _, val, _) and result = "string=" + val) + or + this = MkTrueToken(_, _, _, _) and result = "true" + or + this = MkFalseToken(_, _, _, _) and result = "false" + or + this = MkNullToken(_, _, _, _) and result = "null" + } + + string getSource() { + this = MkLeftBracketToken(_, _, _, result) + or + this = MkRightBracketToken(_, _, _, result) + or + this = MkLeftSquareBracketToken(_, _, _, result) + or + this = MkRightSquareBracketToken(_, _, _, result) + or + this = MkWhiteSpaceToken(_, _, _, result) + or + this = MkCommaToken(_, _, _, result) + or + this = MkColonToken(_, _, _, result) + or + this = MkNumberToken(_, _, _, result) + or + this = MkStringToken(_, _, _, result) + or + this = MkTrueToken(_, _, _, result) + or + this = MkFalseToken(_, _, _, result) + or + this = MkNullToken(_, _, _, result) + } + + JsonToken getNext() { + result.getBegin() = this.getEnd() + 1 and + result.getSource() = this.getSource() + } + } + + private class WhiteSpaceToken extends JsonToken, MkWhiteSpaceToken { } + + private class CommaToken extends JsonToken, MkCommaToken { } + + private class ColonToken extends JsonToken, MkColonToken { } + + private class LeftBracketToken extends JsonToken, MkLeftBracketToken { } + + private class RightBracketToken extends JsonToken, MkRightBracketToken { } + + private class LeftSquareBracketToken extends JsonToken, MkLeftSquareBracketToken { } + + private class RightSquareBracketToken extends JsonToken, MkRightSquareBracketToken { } + + private class StringToken extends JsonToken, MkStringToken { + predicate contains(JsonToken t) { + this.getSource() = t.getSource() and + this.getBegin() < t.getBegin() and + this.getEnd() > t.getEnd() + } + } + + private class NumberToken extends JsonToken, MkNumberToken { } + + private class TrueToken extends JsonToken, MkTrueToken { } + + private class FalseToken extends JsonToken, MkFalseToken { } + + private class NullToken extends JsonToken, MkNullToken { } + + private JsonToken getNextSkippingWhitespace(JsonToken t) { + result = t.getNext() and + not result instanceof WhiteSpaceToken + or + exists(WhiteSpaceToken ws | ws = t.getNext() | result = getNextSkippingWhitespace(ws)) + } + + private newtype TJsonMemberList = + EmtpyMemberList() or + ConsMemberList(JsonMember head, JsonMemberList tail) { + exists(JsonToken first, JsonToken last | + mkJsonMember(first, head, last) and + if getNextSkippingWhitespace(last) instanceof CommaToken + then mkJsonMembers(getNextSkippingWhitespace(getNextSkippingWhitespace(last)), tail, _) + else tail = EmtpyMemberList() + ) + } + + private JsonMember nthMember(JsonMemberList list, int index) { + exists(JsonMember h | list = ConsMemberList(h, _) | index = 0 and result = h) + or + exists(JsonMemberList t | list = ConsMemberList(_, t) | + index > 0 and result = nthMember(t, index - 1) + ) + } + + class JsonMemberList extends TJsonMemberList { + string toString() { + this = EmtpyMemberList() and result = "{}" + or + exists(JsonMember head, JsonMemberList tail, string tailStr | + this = ConsMemberList(head, tail) and + "{" + tailStr + "}" = tail.toString() and + if tailStr != "" + then result = "{" + head.toString() + ", " + tailStr + "}" + else result = "{" + head.toString() + "}" + ) + } + + JsonMember getMember(int index) { result = nthMember(this, index) } + + JsonMember getAMember() { result = getMember(_) } + } + + private newtype TJsonMember = + MkJsonMember(string key, JsonValue value) { + exists(StringToken keyToken, ColonToken colonToken, JsonToken firstValueToken | + colonToken = getNextSkippingWhitespace(keyToken) and + firstValueToken = getNextSkippingWhitespace(colonToken) and + key = keyToken.(JsonToken).getValue() and + mkJsonValue(firstValueToken, value, _) + ) + } + + class JsonMember extends TJsonMember { + string toString() { + exists(string key, JsonValue value | + this = MkJsonMember(key, value) and + result = key + " : " + value.toString() + ) + } + + string getKey() { this = MkJsonMember(result, _) } + + JsonValue getValue() { this = MkJsonMember(_, result) } + } + + private predicate mkJsonMember(StringToken first, JsonMember member, JsonToken last) { + getNextSkippingWhitespace(first) instanceof ColonToken and + exists(JsonValue value | + mkJsonValue(getNextSkippingWhitespace(getNextSkippingWhitespace(first)), value, last) and + member = MkJsonMember(first.getValue(), value) + ) + } + + private predicate mkJsonMembers(JsonToken first, JsonMemberList members, JsonToken last) { + exists(JsonMember h, JsonToken memberLast | mkJsonMember(first, h, memberLast) | + not getNextSkippingWhitespace(memberLast) instanceof CommaToken and + members = ConsMemberList(h, EmtpyMemberList()) and + last = memberLast + ) + or + exists(JsonMember h, JsonToken memberLast, JsonMemberList tail | + mkJsonMember(first, h, memberLast) + | + getNextSkippingWhitespace(memberLast) instanceof CommaToken and + mkJsonMembers(getNextSkippingWhitespace(getNextSkippingWhitespace(memberLast)), tail, last) and + members = ConsMemberList(h, tail) + ) + } + + private newtype TJsonValueList = + EmtpyJsonValueList() or + ConsJsonValueList(JsonValue head, JsonValueList tail) { + exists(JsonToken first, JsonToken last | + mkJsonValue(first, head, last) and + if getNextSkippingWhitespace(last) instanceof CommaToken + then mkJsonValues(getNextSkippingWhitespace(getNextSkippingWhitespace(last)), tail, _) + else tail = EmtpyJsonValueList() + ) + } + + private predicate mkJsonValues(JsonToken first, JsonValueList values, JsonToken last) { + exists(JsonValue h, JsonToken valueLast | mkJsonValue(first, h, valueLast) | + not getNextSkippingWhitespace(valueLast) instanceof CommaToken and + values = ConsJsonValueList(h, EmtpyJsonValueList()) and + last = valueLast + ) + or + exists(JsonValue h, JsonToken valueLast, JsonValueList tail | mkJsonValue(first, h, valueLast) | + getNextSkippingWhitespace(valueLast) instanceof CommaToken and + mkJsonValues(getNextSkippingWhitespace(getNextSkippingWhitespace(valueLast)), tail, last) and + values = ConsJsonValueList(h, tail) + ) + } + + private JsonValue getNthValue(JsonValueList list, int index) { + exists(JsonValue h | list = ConsJsonValueList(h, _) | index = 0 and result = h) + or + exists(JsonValueList t | list = ConsJsonValueList(_, t) | + index > 0 and result = getNthValue(t, index - 1) + ) + } + + class JsonValueList extends TJsonValueList { + string toString() { + this = EmtpyJsonValueList() and result = "[]" + or + exists(JsonValue head, JsonValueList tail, string tailStr | + this = ConsJsonValueList(head, tail) and + "[" + tailStr + "]" = tail.toString() and + if tailStr != "" + then result = "[" + head.toString() + ", " + tailStr + "]" + else result = "[" + head.toString() + "]" + ) + } + + JsonValue getValue(int index) { result = getNthValue(this, index) } + + JsonValue getAValue() { result = getValue(_) } + } + + private newtype TJsonValue = + MkJsonNumber(float n) { + exists(NumberToken t | t.getValue().toFloat() = n | not any(StringToken str).contains(t)) + } or + MkJsonString(string s) { exists(StringToken t | t.(JsonToken).getValue() = s) } or + MkJsonObject(JsonMemberList members) { + exists(LeftBracketToken l, RightBracketToken r, JsonToken last | + getNextSkippingWhitespace(l) != r + | + mkJsonMembers(getNextSkippingWhitespace(l), members, last) and + getNextSkippingWhitespace(last) = r + ) + or + exists(LeftBracketToken l, RightBracketToken r | getNextSkippingWhitespace(l) = r | + members = EmtpyMemberList() + ) + } or + MkJsonArray(JsonValueList values) { + exists(LeftSquareBracketToken l, RightSquareBracketToken r, JsonToken last | + mkJsonValues(getNextSkippingWhitespace(l), values, last) and + getNextSkippingWhitespace(last) = r + ) + or + exists(LeftSquareBracketToken l, RightSquareBracketToken r | + getNextSkippingWhitespace(l) = r + | + values = EmtpyJsonValueList() + ) + } or + MkJsonTrue() { exists(TrueToken t | not any(StringToken str).contains(t)) } or + MkJsonFalse() { exists(FalseToken t | not any(StringToken str).contains(t)) } or + MkJsonNull() { exists(NullToken t | not any(StringToken str).contains(t)) } + + private predicate mkJsonValue(JsonToken first, JsonValue value, JsonToken last) { + first instanceof StringToken and + first = last and + value = MkJsonString(first.getValue()) + or + first instanceof NumberToken and + first = last and + value = MkJsonNumber(first.getValue().toFloat()) + or + first instanceof LeftBracketToken and + exists(JsonMemberList members, JsonToken membersLast | + mkJsonMembers(getNextSkippingWhitespace(first), members, membersLast) and + value = MkJsonObject(members) and + last = getNextSkippingWhitespace(membersLast) + ) and + last instanceof RightBracketToken + or + first instanceof LeftSquareBracketToken and + exists(JsonValueList values, JsonToken valuesLast | + mkJsonValues(getNextSkippingWhitespace(first), values, valuesLast) and + value = MkJsonArray(values) and + last = getNextSkippingWhitespace(valuesLast) + ) and + last instanceof RightSquareBracketToken + or + first instanceof TrueToken and + first = last and + value = MkJsonTrue() + or + first instanceof FalseToken and + first = last and + value = MkJsonFalse() + or + first instanceof NullToken and + first = last and + value = MkJsonNull() + } + + class JsonValue extends TJsonValue { + string toString() { + this = MkJsonString(result) + or + exists(float number | this = MkJsonNumber(number) | result = number.toString()) + or + exists(JsonMemberList members | this = MkJsonObject(members) | result = members.toString()) + or + exists(JsonValueList values | this = MkJsonArray(values) | result = values.toString()) + or + this = MkJsonTrue() and result = "true" + or + this = MkJsonFalse() and result = "false" + or + this = MkJsonNull() and result = "null" + } + } + + class JsonObject extends JsonValue, MkJsonObject { + JsonMemberList getMembers() { this = MkJsonObject(result) } + + JsonMember getMember(int index) { result = getMembers().getMember(index) } + + JsonMember getAMember() { result = getMember(_) } + } + + class JsonString extends JsonValue, MkJsonString { } + + class JsonNumber extends JsonValue, MkJsonNumber { } + + class JsonArray extends JsonValue, MkJsonArray { + JsonValueList getValues() { this = MkJsonArray(result) } + + JsonValue getValue(int index) { result = getValues().getValue(index) } + + JsonValue getAValue() { result = getValue(_) } + } + + JsonValue parse() { + exists(JsonToken firstToken | + not any(JsonToken t).getBegin() < firstToken.getBegin() and + if firstToken instanceof WhiteSpaceToken + then mkJsonValue(getNextSkippingWhitespace(firstToken), result, _) + else mkJsonValue(firstToken, result, _) + ) + } +} \ No newline at end of file From 13839a4400dba6b27781a662cf262ef53d970d1f Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 18 Oct 2023 10:09:10 -0700 Subject: [PATCH 02/36] Add source provenance to Json parser --- .../javascript/frameworks/ui5/JsonParser.qll | 132 +++++++++++------- 1 file changed, 81 insertions(+), 51 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 4d42b028..48f28605 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -1,73 +1,70 @@ +signature string getJsonSig(); -signature class JsonStringReader { - string read(); -} - -module JsonParser { +module JsonParser { private newtype TJsonToken = MkLeftBracketToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and begin = source.indexOf("{") and begin = end and value = "{" } or MkRightBracketToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and begin = source.indexOf("}") and begin = end and value = "}" } or MkLeftSquareBracketToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and begin = source.indexOf("[") and begin = end and value = "[" } or MkRightSquareBracketToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and begin = source.indexOf("]") and begin = end and value = "]" } or MkWhiteSpaceToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and value = source.regexpFind("[\\s\\v\\h]", _, begin) and begin + value.length() - 1 = end } or MkCommaToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and begin = source.indexOf(",") and begin = end and value = "," } or MkColonToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and begin = source.indexOf(":") and begin = end and value = ":" } or MkNumberToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and value = source.regexpFind("-?[1-9]\\d*(\\.\\d+)?((e|E)?(\\+|-)?\\d+)?", _, begin) and begin + value.length() - 1 = end } or MkStringToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and value = source.regexpFind("\"[^\"]*\"", _, begin) and begin + value.length() - 1 = end } or MkTrueToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and value = source.regexpFind("true", _, begin) and begin + value.length() - 1 = end } or MkFalseToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and value = source.regexpFind("false", _, begin) and begin + value.length() - 1 = end } or MkNullToken(int begin, int end, string value, string source) { - source = any(Reader r).read() and + source = getJson() and value = source.regexpFind("null", _, begin) and begin + value.length() - 1 = end } @@ -382,51 +379,63 @@ module JsonParser { } private newtype TJsonValue = - MkJsonNumber(float n) { - exists(NumberToken t | t.getValue().toFloat() = n | not any(StringToken str).contains(t)) + MkJsonNumber(float n, JsonToken source) { + exists(NumberToken t | t.getValue().toFloat() = n and source = t | + not any(StringToken str).contains(t) + ) + } or + MkJsonString(string s, JsonToken source) { + exists(StringToken t | t.(JsonToken).getValue() = s and t = source) } or - MkJsonString(string s) { exists(StringToken t | t.(JsonToken).getValue() = s) } or - MkJsonObject(JsonMemberList members) { + MkJsonObject(JsonMemberList members, JsonToken source) { exists(LeftBracketToken l, RightBracketToken r, JsonToken last | getNextSkippingWhitespace(l) != r | mkJsonMembers(getNextSkippingWhitespace(l), members, last) and - getNextSkippingWhitespace(last) = r + getNextSkippingWhitespace(last) = r and + source = l ) or exists(LeftBracketToken l, RightBracketToken r | getNextSkippingWhitespace(l) = r | - members = EmtpyMemberList() + members = EmtpyMemberList() and source = l ) } or - MkJsonArray(JsonValueList values) { + MkJsonArray(JsonValueList values, JsonToken source) { exists(LeftSquareBracketToken l, RightSquareBracketToken r, JsonToken last | mkJsonValues(getNextSkippingWhitespace(l), values, last) and - getNextSkippingWhitespace(last) = r + getNextSkippingWhitespace(last) = r and + source = l ) or exists(LeftSquareBracketToken l, RightSquareBracketToken r | getNextSkippingWhitespace(l) = r | - values = EmtpyJsonValueList() + values = EmtpyJsonValueList() and source = l ) } or - MkJsonTrue() { exists(TrueToken t | not any(StringToken str).contains(t)) } or - MkJsonFalse() { exists(FalseToken t | not any(StringToken str).contains(t)) } or - MkJsonNull() { exists(NullToken t | not any(StringToken str).contains(t)) } + MkJsonTrue(JsonToken source) { + exists(TrueToken t | not any(StringToken str).contains(t) and source = t) + } or + MkJsonFalse(JsonToken source) { + exists(FalseToken t | not any(StringToken str).contains(t) and source = t) + } or + MkJsonNull(JsonToken source) { + exists(NullToken t | not any(StringToken str).contains(t) and source = t) + } private predicate mkJsonValue(JsonToken first, JsonValue value, JsonToken last) { first instanceof StringToken and first = last and - value = MkJsonString(first.getValue()) + value = MkJsonString(first.getValue(), _) or first instanceof NumberToken and first = last and - value = MkJsonNumber(first.getValue().toFloat()) + value = MkJsonNumber(first.getValue().toFloat(), _) or first instanceof LeftBracketToken and exists(JsonMemberList members, JsonToken membersLast | mkJsonMembers(getNextSkippingWhitespace(first), members, membersLast) and - value = MkJsonObject(members) and + value = MkJsonObject(members, _) and last = getNextSkippingWhitespace(membersLast) ) and last instanceof RightBracketToken @@ -434,44 +443,64 @@ module JsonParser { first instanceof LeftSquareBracketToken and exists(JsonValueList values, JsonToken valuesLast | mkJsonValues(getNextSkippingWhitespace(first), values, valuesLast) and - value = MkJsonArray(values) and + value = MkJsonArray(values, _) and last = getNextSkippingWhitespace(valuesLast) ) and last instanceof RightSquareBracketToken or first instanceof TrueToken and first = last and - value = MkJsonTrue() + value = MkJsonTrue(_) or first instanceof FalseToken and first = last and - value = MkJsonFalse() + value = MkJsonFalse(_) or first instanceof NullToken and first = last and - value = MkJsonNull() + value = MkJsonNull(_) } class JsonValue extends TJsonValue { string toString() { - this = MkJsonString(result) + this = MkJsonString(result, _) or - exists(float number | this = MkJsonNumber(number) | result = number.toString()) + exists(float number | this = MkJsonNumber(number, _) | result = number.toString()) or - exists(JsonMemberList members | this = MkJsonObject(members) | result = members.toString()) + exists(JsonMemberList members | this = MkJsonObject(members, _) | result = members.toString()) or - exists(JsonValueList values | this = MkJsonArray(values) | result = values.toString()) + exists(JsonValueList values | this = MkJsonArray(values, _) | result = values.toString()) or - this = MkJsonTrue() and result = "true" + this = MkJsonTrue(_) and result = "true" or - this = MkJsonFalse() and result = "false" + this = MkJsonFalse(_) and result = "false" or - this = MkJsonNull() and result = "null" + this = MkJsonNull(_) and result = "null" + } + + string getSource() { + exists(JsonToken token | + this = MkJsonString(_, token) + or + this = MkJsonNumber(_, token) + or + this = MkJsonObject(_, token) + or + this = MkJsonArray(_, token) + or + this = MkJsonTrue(token) + or + this = MkJsonFalse(token) + or + this = MkJsonNull(token) + | + result = token.getSource() + ) } } class JsonObject extends JsonValue, MkJsonObject { - JsonMemberList getMembers() { this = MkJsonObject(result) } + JsonMemberList getMembers() { this = MkJsonObject(result, _) } JsonMember getMember(int index) { result = getMembers().getMember(index) } @@ -483,19 +512,20 @@ module JsonParser { class JsonNumber extends JsonValue, MkJsonNumber { } class JsonArray extends JsonValue, MkJsonArray { - JsonValueList getValues() { this = MkJsonArray(result) } + JsonValueList getValues() { this = MkJsonArray(result, _) } JsonValue getValue(int index) { result = getValues().getValue(index) } JsonValue getAValue() { result = getValue(_) } } - JsonValue parse() { + JsonValue parse(string json) { + result.getSource() = json and exists(JsonToken firstToken | - not any(JsonToken t).getBegin() < firstToken.getBegin() and - if firstToken instanceof WhiteSpaceToken - then mkJsonValue(getNextSkippingWhitespace(firstToken), result, _) - else mkJsonValue(firstToken, result, _) + not any(JsonToken t).getBegin() < firstToken.getBegin() and + if firstToken instanceof WhiteSpaceToken + then mkJsonValue(getNextSkippingWhitespace(firstToken), result, _) + else mkJsonValue(firstToken, result, _) ) } -} \ No newline at end of file +} From d57bb93cb45789d38645afd94b2041028b5dd1da Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 18 Oct 2023 10:10:26 -0700 Subject: [PATCH 03/36] Add resource root parsing --- .../javascript/frameworks/ui5/UI5.qll | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 8817d82e..d2fd8e7a 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -1,5 +1,6 @@ -private import javascript +private import javascript private import DataFlow +private import advanced_security.javascript.frameworks.ui5.JsonParser private import semmle.javascript.security.dataflow.DomBasedXssCustomizations private import advanced_security.javascript.frameworks.ui5.UI5View @@ -11,6 +12,48 @@ module UI5 { exists(Project p | p.isInThisProject(f1) and p.isInThisProject(f2)) } + bindingset[this] + private class JsonStringReader extends string { + bindingset[result] + string read() { result = this } + } + + private newtype TResourceRoot = + MkResourceRoot(string name, string path, string source) { + exists( + JsonParser::JsonObject config, + JsonParser::JsonMember configEntry + | + source = config.getSource() and + config.getAMember() = configEntry + | + name = configEntry.getKey() and + path = configEntry.getValue().toString() + ) + } + + class ResourceRoot extends TResourceRoot, MkResourceRoot { + string getName() { this = MkResourceRoot(result, _, _) } + + string getPath() { this = MkResourceRoot(_, result, _) } + + string getSource() { this = MkResourceRoot(_, _, result) } + + string toString() { result = this.getName() + ": " + this.getPath() } + } + + private string getAResourceRootConfig() { + result = any(SapUiCoreScript script).getAttributeByName("data-sap-ui-resourceroots").getValue() + } + + class SapUiCoreScript extends HTML::ScriptElement { + SapUiCoreScript() { this.getSourcePath().matches("%/sap-ui-core.js") } + + ResourceRoot getAResourceRoot() { + result.getSource() = this.getAttributeByName("data-sap-ui-resourceroots").getValue() + } + } + class Project extends Folder { /** * An UI5 project root folder. From a68107fb5319aeb3762d72594af6842f25732387 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 18 Oct 2023 11:39:07 -0700 Subject: [PATCH 04/36] Remove delimiting quotes from string value --- .../javascript/frameworks/ui5/JsonParser.qll | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 48f28605..f62fc10a 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -50,8 +50,11 @@ module JsonParser { } or MkStringToken(int begin, int end, string value, string source) { source = getJson() and - value = source.regexpFind("\"[^\"]*\"", _, begin) and - begin + value.length() - 1 = end + exists(string literal | literal = source.regexpFind("\"[^\"]*\"", _, begin) and + // The string without surrounding quotes. + value = literal.substring(1, literal.length()-1) and + begin + literal.length() - 1 = end + ) } or MkTrueToken(int begin, int end, string value, string source) { source = getJson() and From 66bcc79519d4ebeeb1cd8ef584e07918586eaa46 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 18 Oct 2023 11:39:57 -0700 Subject: [PATCH 05/36] QL formatting --- .../advanced_security/javascript/frameworks/ui5/JsonParser.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index f62fc10a..0db43a8e 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -510,7 +510,8 @@ module JsonParser { JsonMember getAMember() { result = getMember(_) } } - class JsonString extends JsonValue, MkJsonString { } + class JsonString extends JsonValue, MkJsonString { + } class JsonNumber extends JsonValue, MkJsonNumber { } From 10b5ed4200f061b70f7342421cd7660a5e477420 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 18 Oct 2023 11:40:24 -0700 Subject: [PATCH 06/36] Add support for resolved resource roots --- .../javascript/frameworks/ui5/UI5.qll | 43 ++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index d2fd8e7a..33cb162c 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -18,28 +18,57 @@ module UI5 { string read() { result = this } } + private class ResourceRootPathString extends PathString { + SapUiCoreScript coreScript; + + ResourceRootPathString() { this = coreScript.getAResourceRoot().getRoot() } + + override Folder getARootFolder() { result = coreScript.getFile().getParentContainer() } + } + private newtype TResourceRoot = - MkResourceRoot(string name, string path, string source) { + MkResourceRoot(string name, string root, string source) { exists( JsonParser::JsonObject config, - JsonParser::JsonMember configEntry + JsonParser::JsonMember configEntry, SapUiCoreScript coreScript | + source = coreScript.getAttributeByName("data-sap-ui-resourceroots").getValue() and source = config.getSource() and config.getAMember() = configEntry | name = configEntry.getKey() and - path = configEntry.getValue().toString() + root = configEntry.getValue().toString() ) } class ResourceRoot extends TResourceRoot, MkResourceRoot { string getName() { this = MkResourceRoot(result, _, _) } - string getPath() { this = MkResourceRoot(_, result, _) } + string getRoot() { this = MkResourceRoot(_, result, _) } string getSource() { this = MkResourceRoot(_, _, result) } - string toString() { result = this.getName() + ": " + this.getPath() } + string toString() { result = this.getName() + ": " + this.getRoot() } + } + + private newtype TResolvedResourceRoot = + MkResolvedResourceRoot(string name, Folder root, string source) { + exists(ResourceRoot unresolvedRoot, ResourceRootPathString path | + name = unresolvedRoot.getName() and + source = unresolvedRoot.getSource() and + path = unresolvedRoot.getRoot() and + root = path.resolve(path.getARootFolder()).getContainer() + ) + } + + class ResolvedResourceRoot extends TResolvedResourceRoot { + string getName() { this = MkResolvedResourceRoot(result, _, _) } + + Folder getRoot() { this = MkResolvedResourceRoot(_, result, _) } + + string getSource() { this = MkResolvedResourceRoot(_, _, result) } + + string toString() { result = this.getName() + ": " + this.getRoot() } } private string getAResourceRootConfig() { @@ -52,6 +81,10 @@ module UI5 { ResourceRoot getAResourceRoot() { result.getSource() = this.getAttributeByName("data-sap-ui-resourceroots").getValue() } + + ResolvedResourceRoot getAResolvedResourceRoot() { + result.getSource() = this.getAttributeByName("data-sap-ui-resourceroots").getValue() + } } class Project extends Folder { From a5fe10ff61c56a5139d1ab5a3c9d25f0bc8af5d5 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 18 Oct 2023 11:43:04 -0700 Subject: [PATCH 07/36] Address typo in branch type --- .../javascript/frameworks/ui5/JsonParser.qll | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 0db43a8e..3f4a7e27 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -247,13 +247,13 @@ module JsonParser { } private newtype TJsonMemberList = - EmtpyMemberList() or + EmptyMemberList() or ConsMemberList(JsonMember head, JsonMemberList tail) { exists(JsonToken first, JsonToken last | mkJsonMember(first, head, last) and if getNextSkippingWhitespace(last) instanceof CommaToken then mkJsonMembers(getNextSkippingWhitespace(getNextSkippingWhitespace(last)), tail, _) - else tail = EmtpyMemberList() + else tail = EmptyMemberList() ) } @@ -267,7 +267,7 @@ module JsonParser { class JsonMemberList extends TJsonMemberList { string toString() { - this = EmtpyMemberList() and result = "{}" + this = EmptyMemberList() and result = "{}" or exists(JsonMember head, JsonMemberList tail, string tailStr | this = ConsMemberList(head, tail) and @@ -317,7 +317,7 @@ module JsonParser { private predicate mkJsonMembers(JsonToken first, JsonMemberList members, JsonToken last) { exists(JsonMember h, JsonToken memberLast | mkJsonMember(first, h, memberLast) | not getNextSkippingWhitespace(memberLast) instanceof CommaToken and - members = ConsMemberList(h, EmtpyMemberList()) and + members = ConsMemberList(h, EmptyMemberList()) and last = memberLast ) or @@ -331,20 +331,20 @@ module JsonParser { } private newtype TJsonValueList = - EmtpyJsonValueList() or + EmptyJsonValueList() or ConsJsonValueList(JsonValue head, JsonValueList tail) { exists(JsonToken first, JsonToken last | mkJsonValue(first, head, last) and if getNextSkippingWhitespace(last) instanceof CommaToken then mkJsonValues(getNextSkippingWhitespace(getNextSkippingWhitespace(last)), tail, _) - else tail = EmtpyJsonValueList() + else tail = EmptyJsonValueList() ) } private predicate mkJsonValues(JsonToken first, JsonValueList values, JsonToken last) { exists(JsonValue h, JsonToken valueLast | mkJsonValue(first, h, valueLast) | not getNextSkippingWhitespace(valueLast) instanceof CommaToken and - values = ConsJsonValueList(h, EmtpyJsonValueList()) and + values = ConsJsonValueList(h, EmptyJsonValueList()) and last = valueLast ) or @@ -365,7 +365,7 @@ module JsonParser { class JsonValueList extends TJsonValueList { string toString() { - this = EmtpyJsonValueList() and result = "[]" + this = EmptyJsonValueList() and result = "[]" or exists(JsonValue head, JsonValueList tail, string tailStr | this = ConsJsonValueList(head, tail) and @@ -400,7 +400,7 @@ module JsonParser { ) or exists(LeftBracketToken l, RightBracketToken r | getNextSkippingWhitespace(l) = r | - members = EmtpyMemberList() and source = l + members = EmptyMemberList() and source = l ) } or MkJsonArray(JsonValueList values, JsonToken source) { @@ -413,7 +413,7 @@ module JsonParser { exists(LeftSquareBracketToken l, RightSquareBracketToken r | getNextSkippingWhitespace(l) = r | - values = EmtpyJsonValueList() and source = l + values = EmptyJsonValueList() and source = l ) } or MkJsonTrue(JsonToken source) { From c5b584852fbadd5bed3e2d9803b82c45f9e15468 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Fri, 20 Oct 2023 15:59:27 -0700 Subject: [PATCH 08/36] Allow escaped double quotes in strings --- .../advanced_security/javascript/frameworks/ui5/JsonParser.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 3f4a7e27..a7c32ac8 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -50,7 +50,7 @@ module JsonParser { } or MkStringToken(int begin, int end, string value, string source) { source = getJson() and - exists(string literal | literal = source.regexpFind("\"[^\"]*\"", _, begin) and + exists(string literal | literal = source.regexpFind("\"(?:[^\"]|\\\")*\"", _, begin) and // The string without surrounding quotes. value = literal.substring(1, literal.length()-1) and begin + literal.length() - 1 = end From 633aa0fdace1cafd648cfb5ff3a2930712c0da42 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Fri, 20 Oct 2023 16:37:00 -0700 Subject: [PATCH 09/36] Add getType predicate to get the type of a Json value --- .../javascript/frameworks/ui5/JsonParser.qll | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index a7c32ac8..4e07fe40 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -500,6 +500,22 @@ module JsonParser { result = token.getSource() ) } + + string getType() { + this = MkJsonString(_, _) and result = "string" + or + this = MkJsonNumber(_, _) and result = "number" + or + this = MkJsonObject(_, _) and result = "object" + or + this = MkJsonArray(_, _) and result = "array" + or + this = MkJsonTrue(_) and result = "true" + or + this = MkJsonFalse(_) and result = "false" + or + this = MkJsonNull(_) and result = "null" + } } class JsonObject extends JsonValue, MkJsonObject { From 26031984d2fc19afd038ed7f4b6ab4a415d4f8b0 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Fri, 20 Oct 2023 16:37:23 -0700 Subject: [PATCH 10/36] Make sure the first token check uses the same source --- .../advanced_security/javascript/frameworks/ui5/JsonParser.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 4e07fe40..d23f6522 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -542,7 +542,7 @@ module JsonParser { JsonValue parse(string json) { result.getSource() = json and exists(JsonToken firstToken | - not any(JsonToken t).getBegin() < firstToken.getBegin() and + not exists(JsonToken otherToken | otherToken.getBegin() < firstToken.getBegin() and otherToken.getSource() = firstToken.getSource()) and if firstToken instanceof WhiteSpaceToken then mkJsonValue(getNextSkippingWhitespace(firstToken), result, _) else mkJsonValue(firstToken, result, _) From 84f8a4a5dfabbe30c9c097d05ed403be4b8a6d0f Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Fri, 20 Oct 2023 16:38:18 -0700 Subject: [PATCH 11/36] Add failing test case --- .../ui5/test/lib/JsonParser/JsonParser.ql | 12 +++++++++ .../ui5/test/lib/JsonParser/test.js | 25 +++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.ql create mode 100644 javascript/frameworks/ui5/test/lib/JsonParser/test.js diff --git a/javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.ql b/javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.ql new file mode 100644 index 00000000..ac006722 --- /dev/null +++ b/javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.ql @@ -0,0 +1,12 @@ +import javascript +import advanced_security.javascript.frameworks.ui5.JsonParser + +string getStringLiteral() { + result = any(StringLiteral l).getValue() +} + +module StringLiteralJsonParser = JsonParser; + +from StringLiteralJsonParser::JsonValue val, StringLiteral lit +where val = StringLiteralJsonParser::parse(lit.getValue()) +select lit, val, val.getType() \ No newline at end of file diff --git a/javascript/frameworks/ui5/test/lib/JsonParser/test.js b/javascript/frameworks/ui5/test/lib/JsonParser/test.js new file mode 100644 index 00000000..a2ca69e8 --- /dev/null +++ b/javascript/frameworks/ui5/test/lib/JsonParser/test.js @@ -0,0 +1,25 @@ +const test1 = '3'; +const test2 = '-1'; +const test3 = '1.0'; +const test4 = '-1.0'; +const test5 = '1.0e+1'; +const test6 = '-1.0e+1'; +const test7 = '1.0e-1'; + +const test8 = '"hello world"'; +const test9 = '"hello \\"world\\""'; +const test10 = '"hello\nworld\n'; + +const test11 = 'true'; +const test12 = 'false'; +const test13 = 'null'; + +const test14 = '[]'; +const test15 = '[1]'; +const test16 = '[1,2]'; +const test17 = '["1", 2]'; + +const test18 = '{}'; +const test19 = '{"a":1}'; +const test20 = '{"a":1,"b":-1}'; +const test21 = '{"a": {"b": 1, "c": true}, "e": [1, "2", false], "f": [{"g": 1, "h": 2}]}'; From 1b323f39237f3e74594b378f4aa044f55eed247b Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:04:37 -0700 Subject: [PATCH 12/36] Add support for strings with escaped double quotes --- .../javascript/frameworks/ui5/JsonParser.qll | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index d23f6522..6ad70db2 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -50,9 +50,10 @@ module JsonParser { } or MkStringToken(int begin, int end, string value, string source) { source = getJson() and - exists(string literal | literal = source.regexpFind("\"(?:[^\"]|\\\")*\"", _, begin) and + exists(string literal | + literal = source.regexpFind("(?s)\".*?(? Date: Mon, 23 Oct 2023 14:06:24 -0700 Subject: [PATCH 13/36] Address Json value source mixup --- .../javascript/frameworks/ui5/JsonParser.qll | 36 ++++++++++++------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 6ad70db2..c63f2dc7 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -430,39 +430,49 @@ module JsonParser { private predicate mkJsonValue(JsonToken first, JsonValue value, JsonToken last) { first instanceof StringToken and first = last and - value = MkJsonString(first.getValue(), _) + value = MkJsonString(first.getValue(), first) or first instanceof NumberToken and first = last and - value = MkJsonNumber(first.getValue().toFloat(), _) + value = MkJsonNumber(first.getValue().toFloat(), first) or first instanceof LeftBracketToken and - exists(JsonMemberList members, JsonToken membersLast | - mkJsonMembers(getNextSkippingWhitespace(first), members, membersLast) and - value = MkJsonObject(members, _) and - last = getNextSkippingWhitespace(membersLast) + ( + exists(JsonMemberList members, JsonToken membersLast | + mkJsonMembers(getNextSkippingWhitespace(first), members, membersLast) and + value = MkJsonObject(members, first) and + last = getNextSkippingWhitespace(membersLast) + ) + or + last = getNextSkippingWhitespace(first) and + value = MkJsonObject(EmptyMemberList(), first) ) and last instanceof RightBracketToken or first instanceof LeftSquareBracketToken and - exists(JsonValueList values, JsonToken valuesLast | - mkJsonValues(getNextSkippingWhitespace(first), values, valuesLast) and - value = MkJsonArray(values, _) and - last = getNextSkippingWhitespace(valuesLast) + ( + exists(JsonValueList values, JsonToken valuesLast | + mkJsonValues(getNextSkippingWhitespace(first), values, valuesLast) and + value = MkJsonArray(values, first) and + last = getNextSkippingWhitespace(valuesLast) + ) + or + last = getNextSkippingWhitespace(first) and + value = MkJsonArray(EmptyJsonValueList(), first) ) and last instanceof RightSquareBracketToken or first instanceof TrueToken and first = last and - value = MkJsonTrue(_) + value = MkJsonTrue(first) or first instanceof FalseToken and first = last and - value = MkJsonFalse(_) + value = MkJsonFalse(first) or first instanceof NullToken and first = last and - value = MkJsonNull(_) + value = MkJsonNull(first) } class JsonValue extends TJsonValue { From c7d16e69a53008daa3eeaf2fdbe6f9b732568680 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:07:07 -0700 Subject: [PATCH 14/36] Enclose Json string value in double quotes --- .../javascript/frameworks/ui5/JsonParser.qll | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index c63f2dc7..b8e43229 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -477,7 +477,10 @@ module JsonParser { class JsonValue extends TJsonValue { string toString() { - this = MkJsonString(result, _) + exists(string value | + this = MkJsonString(value, _) and + result = "\"" + value + "\"" + ) or exists(float number | this = MkJsonNumber(number, _) | result = number.toString()) or From 962cc18d486575e43b1f609a794de0c79cd288a4 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:07:39 -0700 Subject: [PATCH 15/36] Remove superfluous predicate evaluation --- .../advanced_security/javascript/frameworks/ui5/JsonParser.qll | 2 -- 1 file changed, 2 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index b8e43229..d6b697cd 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -393,8 +393,6 @@ module JsonParser { } or MkJsonObject(JsonMemberList members, JsonToken source) { exists(LeftBracketToken l, RightBracketToken r, JsonToken last | - getNextSkippingWhitespace(l) != r - | mkJsonMembers(getNextSkippingWhitespace(l), members, last) and getNextSkippingWhitespace(last) = r and source = l From 1e2c2408d70f9199c33a4d3b08da6cc2ced6ae49 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:08:22 -0700 Subject: [PATCH 16/36] Add QLdoc to module --- .../advanced_security/javascript/frameworks/ui5/JsonParser.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index d6b697cd..1c42e8c1 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -1,5 +1,8 @@ signature string getJsonSig(); +/** + * A naive Json parser without error recovery. + */ module JsonParser { private newtype TJsonToken = MkLeftBracketToken(int begin, int end, string value, string source) { From 2405ad705836f8da1d00929db55324de4ba36191 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:09:18 -0700 Subject: [PATCH 17/36] Encapsulate first token logic in JsonToken --- .../javascript/frameworks/ui5/JsonParser.qll | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 1c42e8c1..d95d0c27 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -211,6 +211,12 @@ module JsonParser { result.getBegin() = this.getEnd() + 1 and result.getSource() = this.getSource() } + + predicate isFirst() { + not exists(JsonToken other | + other.getBegin() < this.getBegin() and other.getSource() = this.getSource() + ) + } } private class WhiteSpaceToken extends JsonToken, MkWhiteSpaceToken { } @@ -557,7 +563,7 @@ module JsonParser { JsonValue parse(string json) { result.getSource() = json and exists(JsonToken firstToken | - not exists(JsonToken otherToken | otherToken.getBegin() < firstToken.getBegin() and otherToken.getSource() = firstToken.getSource()) and + firstToken.isFirst() and if firstToken instanceof WhiteSpaceToken then mkJsonValue(getNextSkippingWhitespace(firstToken), result, _) else mkJsonValue(firstToken, result, _) From 36bbca70bdbb97b7646fdd8badc8d771ba1734c5 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:10:51 -0700 Subject: [PATCH 18/36] Formatting --- .../advanced_security/javascript/frameworks/ui5/JsonParser.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index d95d0c27..8253e501 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -547,8 +547,7 @@ module JsonParser { JsonMember getAMember() { result = getMember(_) } } - class JsonString extends JsonValue, MkJsonString { - } + class JsonString extends JsonValue, MkJsonString { } class JsonNumber extends JsonValue, MkJsonNumber { } From 94bd9fa66bc1aee5334372f3f89e4057d5d273f1 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:11:06 -0700 Subject: [PATCH 19/36] Address incorrect string --- javascript/frameworks/ui5/test/lib/JsonParser/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/test/lib/JsonParser/test.js b/javascript/frameworks/ui5/test/lib/JsonParser/test.js index a2ca69e8..e73e4a27 100644 --- a/javascript/frameworks/ui5/test/lib/JsonParser/test.js +++ b/javascript/frameworks/ui5/test/lib/JsonParser/test.js @@ -8,7 +8,7 @@ const test7 = '1.0e-1'; const test8 = '"hello world"'; const test9 = '"hello \\"world\\""'; -const test10 = '"hello\nworld\n'; +const test10 = '"hello\nworld\n"'; const test11 = 'true'; const test12 = 'false'; From b2a04db7e79e9bf282ceb82c7c60dfad145fd410 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 14:12:16 -0700 Subject: [PATCH 20/36] Add Json test expected file --- .../test/lib/JsonParser/JsonParser.expected | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.expected diff --git a/javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.expected b/javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.expected new file mode 100644 index 00000000..d9f6e533 --- /dev/null +++ b/javascript/frameworks/ui5/test/lib/JsonParser/JsonParser.expected @@ -0,0 +1,21 @@ +| test.js:1:15:1:17 | '3' | 3 | number | +| test.js:2:15:2:18 | '-1' | -1 | number | +| test.js:3:15:3:19 | '1.0' | 1 | number | +| test.js:4:15:4:20 | '-1.0' | -1 | number | +| test.js:5:15:5:22 | '1.0e+1' | 10 | number | +| test.js:6:15:6:23 | '-1.0e+1' | -10 | number | +| test.js:7:15:7:22 | '1.0e-1' | 0.1 | number | +| test.js:9:15:9:29 | '"hello world"' | "hello world" | string | +| test.js:10:15:10:35 | '"hello ... ld\\\\""' | "hello \\"world\\"" | string | +| test.js:11:16:11:33 | '"hello\\nworld\\n"' | "hello\nworld\n" | string | +| test.js:13:16:13:21 | 'true' | true | true | +| test.js:14:16:14:22 | 'false' | false | false | +| test.js:15:16:15:21 | 'null' | null | null | +| test.js:17:16:17:19 | '[]' | [] | array | +| test.js:18:16:18:20 | '[1]' | [1] | array | +| test.js:19:16:19:22 | '[1,2]' | [1, 2] | array | +| test.js:20:16:20:25 | '["1", 2]' | ["1", 2] | array | +| test.js:22:16:22:19 | '{}' | {} | object | +| test.js:23:16:23:24 | '{"a":1}' | {a : 1} | object | +| test.js:24:16:24:31 | '{"a":1,"b":-1}' | {a : 1, b : -1} | object | +| test.js:25:16:25:90 | '{"a": ... : 2}]}' | {a : {b : 1, c : true}, e : [1, "2", false], f : [{g : 1, h : 2}]} | object | From 000e62c1fec6bcceb59bc7de0013b3c560925f31 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 16 Oct 2023 14:44:03 -0700 Subject: [PATCH 21/36] Broaden the requirements for an XML view We have discovered XML views that have the namespace `sap.ui.core` instead of the documented `sap.ui.core.mvc`. To detect these XML views we now include root elements with: - the name `View`, and - a namespace `sap.ui.core.mvc`, or `sap.ui.core`, but with a namespace declaration for `sap.ui.core.mvc`. --- .../javascript/frameworks/ui5/UI5View.qll | 42 ++++++++++++++++--- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll index 74cd45f3..4716f1e1 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll @@ -399,12 +399,44 @@ class XmlBindingPath extends UI5BindingPath instanceof XmlAttribute { } } +class XmlRootElement extends XmlElement { + XmlRootElement() { any(XmlFile f).getARootElement() = this } + + /** + * Returns a XML namespace declaration scoped to the element. + * + * The predicate relies on location information to determine the scope of the namespace declaration. + * A XML element with the same starting line and column, but a larger ending line and column is considered the + * scope of the namespace declaration. + */ + XmlNamespace getANamespaceDeclaration() { + exists(Location elemLoc, Location nsLoc | + elemLoc = this.getLocation() and + nsLoc = result.getLocation() + | + elemLoc.getStartLine() = nsLoc.getStartLine() and + elemLoc.getStartColumn() = nsLoc.getStartColumn() and + ( + elemLoc.getEndLine() > nsLoc.getEndLine() + or + elemLoc.getEndLine() = nsLoc.getEndLine() and + elemLoc.getEndColumn() > nsLoc.getEndColumn() + ) + ) + } +} + class XmlView extends UI5View, XmlFile { - XmlElement root; + XmlRootElement root; XmlView() { root = this.getARootElement() and - root.getNamespace().getUri() = "sap.ui.core.mvc" and + ( + root.getNamespace().getUri() = "sap.ui.core.mvc" + or + root.getNamespace().getUri() = "sap.ui.core" and + root.getANamespaceDeclaration().getUri() = "sap.ui.core.mvc" + ) and root.hasName("View") } @@ -510,7 +542,7 @@ abstract class UI5Control extends Locatable { CustomController getController() { result = this.getView().getController() } } -class XmlControl extends UI5Control, XmlElement { +class XmlControl extends UI5Control instanceof XmlElement { XmlControl() { this.getParent+() = any(XmlView view) } /** Get the qualified type string, e.g. `sap.m.SearchField` */ @@ -523,11 +555,11 @@ class XmlControl extends UI5Control, XmlElement { result = any(CustomControl control | control.getName() = this.getQualifiedType()) } - override Location getLocation() { result = XmlElement.super.getLocation() } + override Location getLocation() { result = this.(XmlElement).getLocation() } override XmlFile getFile() { result = XmlElement.super.getFile() } - override UI5ControlProperty getAProperty(string name) { result = this.getAttribute(name) } + override UI5ControlProperty getAProperty(string name) { result = this.(XmlElement).getAttribute(name) } override CustomControl getDefinition() { result.getName() = this.getQualifiedType() and From 9920069a6fa12acf50c5447fb9d61b66eef014bf Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 24 Oct 2023 16:10:34 -0700 Subject: [PATCH 22/36] Upgrade CodeQL dependencies and bump pack versions --- javascript/frameworks/ui5/ext/qlpack.yml | 6 +++--- .../frameworks/ui5/lib/codeql-pack.lock.yml | 12 +++++++----- javascript/frameworks/ui5/lib/qlpack.yml | 6 +++--- .../frameworks/ui5/src/codeql-pack.lock.yml | 12 +++++++----- javascript/frameworks/ui5/src/qlpack.yml | 8 ++++---- .../frameworks/ui5/test/codeql-pack.lock.yml | 18 ++++++++++-------- javascript/frameworks/ui5/test/qlpack.yml | 12 ++++++------ scripts/codeql-pack.lock.yml | 14 +++++++------- scripts/qlpack.yml | 2 +- 9 files changed, 48 insertions(+), 42 deletions(-) diff --git a/javascript/frameworks/ui5/ext/qlpack.yml b/javascript/frameworks/ui5/ext/qlpack.yml index 23e18667..41b1e972 100644 --- a/javascript/frameworks/ui5/ext/qlpack.yml +++ b/javascript/frameworks/ui5/ext/qlpack.yml @@ -1,9 +1,9 @@ --- library: true name: advanced-security/javascript-sap-ui5-models -version: 0.3.0 +version: 0.4.0 extensionTargets: - codeql/javascript-all: "^0.6.3" - codeql/javascript-queries: "^0.6.3" + codeql/javascript-all: "^0.8.1" + codeql/javascript-queries: "^0.8.1" dataExtensions: - "*.model.yml" diff --git a/javascript/frameworks/ui5/lib/codeql-pack.lock.yml b/javascript/frameworks/ui5/lib/codeql-pack.lock.yml index f8b36209..7e43634f 100644 --- a/javascript/frameworks/ui5/lib/codeql-pack.lock.yml +++ b/javascript/frameworks/ui5/lib/codeql-pack.lock.yml @@ -2,13 +2,15 @@ lockVersion: 1.0.0 dependencies: codeql/javascript-all: - version: 0.6.4 + version: 0.8.1 + codeql/mad: + version: 0.2.1 codeql/regex: - version: 0.0.15 + version: 0.2.1 codeql/tutorial: - version: 0.0.12 + version: 0.2.1 codeql/util: - version: 0.0.12 + version: 0.2.1 codeql/yaml: - version: 0.0.4 + version: 0.2.1 compiled: false diff --git a/javascript/frameworks/ui5/lib/qlpack.yml b/javascript/frameworks/ui5/lib/qlpack.yml index 44442641..25a31aa3 100644 --- a/javascript/frameworks/ui5/lib/qlpack.yml +++ b/javascript/frameworks/ui5/lib/qlpack.yml @@ -1,9 +1,9 @@ --- library: true name: advanced-security/javascript-sap-ui5-all -version: 0.3.0 +version: 0.4.0 suites: codeql-suites extractor: javascript dependencies: - codeql/javascript-all: "^0.6.3" - advanced-security/javascript-sap-ui5-models: "^0.3.0" \ No newline at end of file + codeql/javascript-all: "^0.8.1" + advanced-security/javascript-sap-ui5-models: "^0.4.0" \ No newline at end of file diff --git a/javascript/frameworks/ui5/src/codeql-pack.lock.yml b/javascript/frameworks/ui5/src/codeql-pack.lock.yml index f8b36209..7e43634f 100644 --- a/javascript/frameworks/ui5/src/codeql-pack.lock.yml +++ b/javascript/frameworks/ui5/src/codeql-pack.lock.yml @@ -2,13 +2,15 @@ lockVersion: 1.0.0 dependencies: codeql/javascript-all: - version: 0.6.4 + version: 0.8.1 + codeql/mad: + version: 0.2.1 codeql/regex: - version: 0.0.15 + version: 0.2.1 codeql/tutorial: - version: 0.0.12 + version: 0.2.1 codeql/util: - version: 0.0.12 + version: 0.2.1 codeql/yaml: - version: 0.0.4 + version: 0.2.1 compiled: false diff --git a/javascript/frameworks/ui5/src/qlpack.yml b/javascript/frameworks/ui5/src/qlpack.yml index 028c9371..831c5c86 100644 --- a/javascript/frameworks/ui5/src/qlpack.yml +++ b/javascript/frameworks/ui5/src/qlpack.yml @@ -1,11 +1,11 @@ --- library: false name: advanced-security/javascript-sap-ui5-queries -version: 0.3.0 +version: 0.4.0 suites: codeql-suites extractor: javascript dependencies: - codeql/javascript-all: "^0.6.3" - advanced-security/javascript-sap-ui5-models: "^0.3.0" - advanced-security/javascript-sap-ui5-all: "^0.3.0" + codeql/javascript-all: "^0.8.1" + advanced-security/javascript-sap-ui5-models: "^0.4.0" + advanced-security/javascript-sap-ui5-all: "^0.4.0" default-suite-file: codeql-suites/sap-ui5-code-scanning.qls diff --git a/javascript/frameworks/ui5/test/codeql-pack.lock.yml b/javascript/frameworks/ui5/test/codeql-pack.lock.yml index 79d3aa8c..516f3e8a 100644 --- a/javascript/frameworks/ui5/test/codeql-pack.lock.yml +++ b/javascript/frameworks/ui5/test/codeql-pack.lock.yml @@ -2,19 +2,21 @@ lockVersion: 1.0.0 dependencies: codeql/javascript-all: - version: 0.6.4 + version: 0.8.1 codeql/javascript-queries: - version: 0.6.4 + version: 0.8.1 + codeql/mad: + version: 0.2.1 codeql/regex: - version: 0.0.15 + version: 0.2.1 codeql/suite-helpers: - version: 0.5.4 + version: 0.7.1 codeql/tutorial: - version: 0.0.12 + version: 0.2.1 codeql/typos: - version: 0.0.19 + version: 0.2.1 codeql/util: - version: 0.0.12 + version: 0.2.1 codeql/yaml: - version: 0.0.4 + version: 0.2.1 compiled: false diff --git a/javascript/frameworks/ui5/test/qlpack.yml b/javascript/frameworks/ui5/test/qlpack.yml index 642854b5..b96d9607 100644 --- a/javascript/frameworks/ui5/test/qlpack.yml +++ b/javascript/frameworks/ui5/test/qlpack.yml @@ -1,9 +1,9 @@ name: advanced-security/javascript-sap-ui5-queries-tests -version: 0.3.0 +version: 0.4.0 extractor: javascript dependencies: - codeql/javascript-all: "^0.6.3" - codeql/javascript-queries: "^0.6.3" - advanced-security/javascript-sap-ui5-queries: "^0.2.0" - advanced-security/javascript-sap-ui5-models: "^0.2.0" - advanced-security/javascript-sap-ui5-all: "^0.2.0" + codeql/javascript-all: "^0.8.1" + codeql/javascript-queries: "^0.8.1" + advanced-security/javascript-sap-ui5-queries: "^0.4.0" + advanced-security/javascript-sap-ui5-models: "^0.4.0" + advanced-security/javascript-sap-ui5-all: "^0.4.0" diff --git a/scripts/codeql-pack.lock.yml b/scripts/codeql-pack.lock.yml index bc9df1cc..7e43634f 100644 --- a/scripts/codeql-pack.lock.yml +++ b/scripts/codeql-pack.lock.yml @@ -2,15 +2,15 @@ lockVersion: 1.0.0 dependencies: codeql/javascript-all: - version: 0.7.0 + version: 0.8.1 codeql/mad: - version: 0.1.0 + version: 0.2.1 codeql/regex: - version: 0.1.0 + version: 0.2.1 codeql/tutorial: - version: 0.1.0 + version: 0.2.1 codeql/util: - version: 0.1.0 + version: 0.2.1 codeql/yaml: - version: 0.1.0 -compiled: false \ No newline at end of file + version: 0.2.1 +compiled: false diff --git a/scripts/qlpack.yml b/scripts/qlpack.yml index a6344877..6993811f 100644 --- a/scripts/qlpack.yml +++ b/scripts/qlpack.yml @@ -4,4 +4,4 @@ warnOnImplicitThis: false name: advanced-security/jsdoc-extraction version: 0.0.1 dependencies: - codeql/javascript-all: "*" \ No newline at end of file + codeql/javascript-all: "^0.8.1" \ No newline at end of file From 7d468a8bb2356b382f0a3be0eba2bf15b1224951 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 25 Oct 2023 11:16:45 -0700 Subject: [PATCH 23/36] Upgrade CLI to 2.15.1 --- qlt.conf.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qlt.conf.json b/qlt.conf.json index 9665476c..1db9ad4a 100644 --- a/qlt.conf.json +++ b/qlt.conf.json @@ -1,5 +1,5 @@ { - "CodeQLCLI": "2.14.1", - "CodeQLStandardLibrary": "codeql-cli/v2.14.1", - "CodeQLCLIBundle": "codeql-bundle-v2.14.1" + "CodeQLCLI": "2.15.1", + "CodeQLStandardLibrary": "codeql-cli/v2.15.1", + "CodeQLCLIBundle": "codeql-bundle-v2.15.1" } \ No newline at end of file From 8ae110d8bbc0223132a393d87232a53376c05224 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 16:20:00 -0700 Subject: [PATCH 24/36] Add member predicate to interpret Json value as a string value --- .../advanced_security/javascript/frameworks/ui5/JsonParser.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll index 8253e501..622ea6bd 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/JsonParser.qll @@ -537,6 +537,8 @@ module JsonParser { or this = MkJsonNull(_) and result = "null" } + + string asString() { this = MkJsonString(result, _) } } class JsonObject extends JsonValue, MkJsonObject { From 4186ebea943cda4565a9879ec8659d5bbeb75274 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 16:21:41 -0700 Subject: [PATCH 25/36] Resolve resource roots using Json string value --- .../ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 33cb162c..866e4e28 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -37,7 +37,7 @@ module UI5 { config.getAMember() = configEntry | name = configEntry.getKey() and - root = configEntry.getValue().toString() + root = configEntry.getValue().asString() ) } From feda8e24e35fa4baec62ef59251120873c7cb3b8 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 16:22:05 -0700 Subject: [PATCH 26/36] Add member predicate to determine if file is part of resource root --- .../lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 866e4e28..48723cc8 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -69,6 +69,10 @@ module UI5 { string getSource() { this = MkResolvedResourceRoot(_, _, result) } string toString() { result = this.getName() + ": " + this.getRoot() } + + predicate contains(File file) { + file.getParentContainer+() = getRoot() + } } private string getAResourceRootConfig() { From a9a885e73091fb182d67d93db1172c8efdf99db4 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 23 Oct 2023 16:22:32 -0700 Subject: [PATCH 27/36] Consider nojQuery Sap Ui core script --- .../ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 48723cc8..9d63cb50 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -80,7 +80,7 @@ module UI5 { } class SapUiCoreScript extends HTML::ScriptElement { - SapUiCoreScript() { this.getSourcePath().matches("%/sap-ui-core.js") } + SapUiCoreScript() { this.getSourcePath().matches(["%/sap-ui-core.js", "%sap-ui-core-nojQuery.js"]) } ResourceRoot getAResourceRoot() { result.getSource() = this.getAttributeByName("data-sap-ui-resourceroots").getValue() From 2dbaf580d3d9e2dcb35721bd151a31bfaa1da448 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 24 Oct 2023 15:56:30 -0700 Subject: [PATCH 28/36] Replace Project with WebApp Project resolution relied on a property that doesn't hold for all SAP UI5 projects. The WebApp approach follows the bootstrapping process to determine the definition of a webapp and uses the resource roots to resolve resources beloning to a webapp. --- .../javascript/frameworks/ui5/UI5.qll | 77 +++++++++++-------- .../javascript/frameworks/ui5/UI5DataFlow.qll | 12 +-- .../javascript/frameworks/ui5/UI5HTML.qll | 4 +- .../javascript/frameworks/ui5/UI5View.qll | 18 ++--- .../src/UI5Clickjacking/UI5Clickjacking.ql | 6 +- 5 files changed, 64 insertions(+), 53 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 9d63cb50..5c2e1eae 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -5,12 +5,6 @@ private import semmle.javascript.security.dataflow.DomBasedXssCustomizations private import advanced_security.javascript.frameworks.ui5.UI5View module UI5 { - /** - * Helper predicate checking if two elements are in the same Project - */ - predicate inSameUI5Project(File f1, File f2) { - exists(Project p | p.isInThisProject(f1) and p.isInThisProject(f2)) - } bindingset[this] private class JsonStringReader extends string { @@ -91,30 +85,49 @@ module UI5 { } } - class Project extends Folder { - /** - * An UI5 project root folder. - */ - Project() { exists(File yamlFile | yamlFile = this.getFile("ui5.yaml")) } + /** A UI5 web application manifest associated with a bootstrapped UI5 web application. */ + class WebAppManifest extends File { + WebApp webapp; + WebAppManifest() { + this.getBaseName() = "manifest.json" and + this.getParentContainer() = webapp.getWebAppFolder() + } - /** - * The `ui5.yaml` file that declares a UI5 application. - */ - File getProjectYaml() { result = this.getFile("ui5.yaml") } + WebApp getWebapp() { result = webapp } + } - predicate isInThisProject(File file) { this = file.getParentContainer*() } + /** A UI5 bootstrapped web application. */ + class WebApp extends HTML::HtmlFile { + SapUiCoreScript coreScript; - private HTML::HtmlFile getSapUICoreScript() { - exists(HTML::ScriptElement script | - result = script.getFile() and - this.isInThisProject(result) and - script.getSourcePath().matches("%/sap-ui-core.js") - ) + WebApp() { + coreScript.getFile() = this } - HTML::HtmlFile getMainHTML() { result = this.getSapUICoreScript() } - } + File getAResource() { + coreScript.getAResolvedResourceRoot().contains(result) + } + + File getResource(string path) { + getWebAppFolder().getAbsolutePath() + "/" + path = result.getAbsolutePath() + } + Folder getWebAppFolder() { + result = this.getParentContainer() + } + + WebAppManifest getManifest() { + result.getWebapp() = this + } + + File getInitialModule() { + exists(string initialModuleResourcePath, string resolvedModulePath, ResolvedResourceRoot resourceRoot | + initialModuleResourcePath = coreScript.getAttributeByName("data-sap-ui-onInit").getValue() and coreScript.getAResolvedResourceRoot() = resourceRoot and + resolvedModulePath = initialModuleResourcePath.regexpReplaceAll("^module\\s*:\\s*", "").replaceAll(resourceRoot.getName(), resourceRoot.getRoot().getAbsolutePath()) and + result.getAbsolutePath() = resolvedModulePath + ".js" + ) + } + } /** * https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.loader%23methods/sap.ui.loader.config */ @@ -156,7 +169,9 @@ module UI5 { ) } - Project getProject() { result = this.getFile().getParentContainer*() } + WebApp getWebApp() { + this.getFile() = result.getAResource() + } SapDefineModule getExtendingDefine() { exists(Extension baseExtension, Extension subclassExtension, SapDefineModule subclassDefine | @@ -371,12 +386,8 @@ module UI5 { */ bindingset[path] JsonObject resolveDirectPath(string path) { - exists(Project project, File jsonFile | - // project contains this file - project.isInThisProject(jsonFile) and - jsonFile.getExtension() = "json" and - jsonFile.getAbsolutePath() = project.getASubFolder().getAbsolutePath() + "/" + path and - result.getJsonFile() = jsonFile + exists(WebApp webApp| + result.getJsonFile() = webApp.getResource(path) ) } @@ -582,14 +593,14 @@ module UI5 { result.getMethodName() = "setProperty" and result.getArgument(0).asExpr().(StringLiteral).getValue() = propName and // TODO: in same controller - inSameUI5Project(this.getFile(), result.getFile()) + exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) } bindingset[propName] MethodCallNode getARead(string propName) { result.getMethodName() = "get" + capitalize(propName) and // TODO: in same controller - inSameUI5Project(this.getFile(), result.getFile()) + exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) } } } diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll index 0d9d1387..409d639d 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll @@ -12,7 +12,7 @@ module UI5DataFlow { private predicate bidiModelControl(DataFlow::Node start, DataFlow::Node end) { exists(DataFlow::SourceNode property, Metadata metadata, UI5BoundNode node | // same project - inSameUI5Project(metadata.getFile(), node.getFile()) and + exists(WebApp webApp | webApp.getAResource() = metadata.getFile() and webApp.getAResource() = node.getFile()) and ( // same control metadata.getControl().getName() = node.getBindingPath().getControlQualifiedType() @@ -87,22 +87,22 @@ module UI5DataFlow { UI5BindingPath getBindingPath() { result = bindingPath } UI5BoundNode() { + exists(WebApp webApp | webApp.getAResource() = this.getFile() and + webApp.getAResource() = bindingPath.getFile() | /* The relevant portion of the content of a JSONModel */ exists(Property p, JsonModel model | // The property bound to an UI5View source this.(DataFlow::PropRef).getPropertyNameExpr() = p.getNameExpr() and // The binding path refers to this model - bindingPath.getAbsolutePath() = model.getPathString(p) and - inSameUI5Project(this.getFile(), bindingPath.getFile()) + bindingPath.getAbsolutePath() = model.getPathString(p) ) or /* The URI string to the JSONModel constructor call */ exists(JsonModel model | this = model.getArgument(0) and this.asExpr() instanceof StringLiteral and - bindingPath.getAbsolutePath() = model.getPathString() and - inSameUI5Project(this.getFile(), bindingPath.getFile()) - ) + bindingPath.getAbsolutePath() = model.getPathString() + )) } } diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll index 4cbebfbd..32fadbbb 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll @@ -87,8 +87,8 @@ class FrameOptions extends TFrameOptions { /** * Holds if the frame options are left untouched as the default value `trusted`. */ -predicate thereIsNoFrameOptionSet(UI5::Project p) { - not exists(FrameOptions frameOptions | p.isInThisProject(frameOptions.getLocation().getFile()) | +predicate thereIsNoFrameOptionSet(UI5::WebApp webapp) { + not exists(FrameOptions frameOptions | webapp.getAResource() = frameOptions.getLocation().getFile() | frameOptions.allowsSharedOriginEmbedding() or frameOptions.deniesEmbedding() or frameOptions.allowsAllOriginEmbedding() diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll index 4716f1e1..a800a3fc 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll @@ -98,8 +98,8 @@ abstract class UI5BindingPath extends Locatable { // The property bound to an UI5View source result.getPropertyNameExpr() = p.getNameExpr() and this.getAbsolutePath() = model.getPathString(p) and - //restrict search inside the same project - inSameUI5Project(this.getFile(), result.getFile()) + //restrict search inside the same webapp + exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) ) // TODO /* @@ -142,8 +142,8 @@ abstract class UI5View extends File { CustomController getController() { // The controller name should match result.getName() = this.getControllerName() and - // The View and the Controller are in a same project - inSameUI5Project(this, result.getFile()) + // The View and the Controller are in a same webapp + exists(WebApp webApp | webApp.getAResource() = this and webApp.getAResource() = result.getFile()) } abstract UI5BindingPath getASource(); @@ -490,10 +490,10 @@ class XmlView extends UI5View, XmlFile { ( builtInControl(element.getNamespace()) or - // or a custom control with implementation code found in the project + // or a custom control with implementation code found in the webapp exists(CustomControl control | control.getName() = element.getNamespace().getUri() + "." + element.getName() and - inSameUI5Project(control.getFile(), element.getFile()) + exists(WebApp webApp | webApp.getAResource() = control.getFile() and webApp.getAResource() = element.getFile()) ) ) ) @@ -563,20 +563,20 @@ class XmlControl extends UI5Control instanceof XmlElement { override CustomControl getDefinition() { result.getName() = this.getQualifiedType() and - inSameUI5Project(this.getFile(), result.getFile()) + exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) } bindingset[propName] override MethodCallNode getARead(string propName) { // TODO: in same view - inSameUI5Project(this.getFile(), result.getFile()) and + exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) and result.getMethodName() = "get" + capitalize(propName) } bindingset[propName] override MethodCallNode getAWrite(string propName) { // TODO: in same view - inSameUI5Project(this.getFile(), result.getFile()) and + exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) and result.getMethodName() = "set" + capitalize(propName) } diff --git a/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql b/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql index a76f967c..69969e5c 100644 --- a/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql +++ b/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql @@ -17,7 +17,7 @@ private import advanced_security.javascript.frameworks.ui5.UI5 class FirstLineOfMainHtml extends HTML::DocumentElement, FirstLineOf { FirstLineOfMainHtml() { - exists(UI5::Project p | this.getFile().(FirstLineOf).getFile() = p.getMainHTML()) + exists(UI5::WebApp app | this.getFile().(FirstLineOf).getFile() = app) } } @@ -51,8 +51,8 @@ where " being set to `allow`." ) or - exists(UI5::Project p | thereIsNoFrameOptionSet(p) | - alert.asFirstLineOfMainHtml().getFile() = p.getMainHTML() and + exists(UI5::WebApp app | thereIsNoFrameOptionSet(app) | + alert.asFirstLineOfMainHtml().getFile() = app and message = "Possible clickjacking vulnerability due to missing frame options." ) select alert, message From 9311c30475848819bc8aa3d58d3611ed800a9e20 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 24 Oct 2023 16:00:50 -0700 Subject: [PATCH 29/36] Format QL modules --- .../javascript/frameworks/ui5/UI5.qll | 58 +++++++++---------- .../javascript/frameworks/ui5/UI5DataFlow.qll | 41 +++++++------ .../javascript/frameworks/ui5/UI5View.qll | 25 ++++++-- 3 files changed, 70 insertions(+), 54 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 5c2e1eae..910f474f 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -5,7 +5,6 @@ private import semmle.javascript.security.dataflow.DomBasedXssCustomizations private import advanced_security.javascript.frameworks.ui5.UI5View module UI5 { - bindingset[this] private class JsonStringReader extends string { bindingset[result] @@ -64,9 +63,7 @@ module UI5 { string toString() { result = this.getName() + ": " + this.getRoot() } - predicate contains(File file) { - file.getParentContainer+() = getRoot() - } + predicate contains(File file) { file.getParentContainer+() = getRoot() } } private string getAResourceRootConfig() { @@ -74,7 +71,9 @@ module UI5 { } class SapUiCoreScript extends HTML::ScriptElement { - SapUiCoreScript() { this.getSourcePath().matches(["%/sap-ui-core.js", "%sap-ui-core-nojQuery.js"]) } + SapUiCoreScript() { + this.getSourcePath().matches(["%/sap-ui-core.js", "%sap-ui-core-nojQuery.js"]) + } ResourceRoot getAResourceRoot() { result.getSource() = this.getAttributeByName("data-sap-ui-resourceroots").getValue() @@ -88,8 +87,9 @@ module UI5 { /** A UI5 web application manifest associated with a bootstrapped UI5 web application. */ class WebAppManifest extends File { WebApp webapp; + WebAppManifest() { - this.getBaseName() = "manifest.json" and + this.getBaseName() = "manifest.json" and this.getParentContainer() = webapp.getWebAppFolder() } @@ -100,34 +100,34 @@ module UI5 { class WebApp extends HTML::HtmlFile { SapUiCoreScript coreScript; - WebApp() { - coreScript.getFile() = this - } + WebApp() { coreScript.getFile() = this } - File getAResource() { - coreScript.getAResolvedResourceRoot().contains(result) - } + File getAResource() { coreScript.getAResolvedResourceRoot().contains(result) } File getResource(string path) { getWebAppFolder().getAbsolutePath() + "/" + path = result.getAbsolutePath() } - Folder getWebAppFolder() { - result = this.getParentContainer() - } + Folder getWebAppFolder() { result = this.getParentContainer() } - WebAppManifest getManifest() { - result.getWebapp() = this - } + WebAppManifest getManifest() { result.getWebapp() = this } File getInitialModule() { - exists(string initialModuleResourcePath, string resolvedModulePath, ResolvedResourceRoot resourceRoot | - initialModuleResourcePath = coreScript.getAttributeByName("data-sap-ui-onInit").getValue() and coreScript.getAResolvedResourceRoot() = resourceRoot and - resolvedModulePath = initialModuleResourcePath.regexpReplaceAll("^module\\s*:\\s*", "").replaceAll(resourceRoot.getName(), resourceRoot.getRoot().getAbsolutePath()) and + exists( + string initialModuleResourcePath, string resolvedModulePath, + ResolvedResourceRoot resourceRoot + | + initialModuleResourcePath = coreScript.getAttributeByName("data-sap-ui-onInit").getValue() and + coreScript.getAResolvedResourceRoot() = resourceRoot and + resolvedModulePath = + initialModuleResourcePath + .regexpReplaceAll("^module\\s*:\\s*", "") + .replaceAll(resourceRoot.getName(), resourceRoot.getRoot().getAbsolutePath()) and result.getAbsolutePath() = resolvedModulePath + ".js" ) } } + /** * https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.loader%23methods/sap.ui.loader.config */ @@ -169,9 +169,7 @@ module UI5 { ) } - WebApp getWebApp() { - this.getFile() = result.getAResource() - } + WebApp getWebApp() { this.getFile() = result.getAResource() } SapDefineModule getExtendingDefine() { exists(Extension baseExtension, Extension subclassExtension, SapDefineModule subclassDefine | @@ -386,9 +384,7 @@ module UI5 { */ bindingset[path] JsonObject resolveDirectPath(string path) { - exists(WebApp webApp| - result.getJsonFile() = webApp.getResource(path) - ) + exists(WebApp webApp | result.getJsonFile() = webApp.getResource(path)) } /** @@ -593,14 +589,18 @@ module UI5 { result.getMethodName() = "setProperty" and result.getArgument(0).asExpr().(StringLiteral).getValue() = propName and // TODO: in same controller - exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) + exists(WebApp webApp | + webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile() + ) } bindingset[propName] MethodCallNode getARead(string propName) { result.getMethodName() = "get" + capitalize(propName) and // TODO: in same controller - exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) + exists(WebApp webApp | + webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile() + ) } } } diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll index 409d639d..2e7a910d 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5DataFlow.qll @@ -12,7 +12,9 @@ module UI5DataFlow { private predicate bidiModelControl(DataFlow::Node start, DataFlow::Node end) { exists(DataFlow::SourceNode property, Metadata metadata, UI5BoundNode node | // same project - exists(WebApp webApp | webApp.getAResource() = metadata.getFile() and webApp.getAResource() = node.getFile()) and + exists(WebApp webApp | + webApp.getAResource() = metadata.getFile() and webApp.getAResource() = node.getFile() + ) and ( // same control metadata.getControl().getName() = node.getBindingPath().getControlQualifiedType() @@ -87,22 +89,25 @@ module UI5DataFlow { UI5BindingPath getBindingPath() { result = bindingPath } UI5BoundNode() { - exists(WebApp webApp | webApp.getAResource() = this.getFile() and - webApp.getAResource() = bindingPath.getFile() | - /* The relevant portion of the content of a JSONModel */ - exists(Property p, JsonModel model | - // The property bound to an UI5View source - this.(DataFlow::PropRef).getPropertyNameExpr() = p.getNameExpr() and - // The binding path refers to this model - bindingPath.getAbsolutePath() = model.getPathString(p) + exists(WebApp webApp | + webApp.getAResource() = this.getFile() and + webApp.getAResource() = bindingPath.getFile() + | + /* The relevant portion of the content of a JSONModel */ + exists(Property p, JsonModel model | + // The property bound to an UI5View source + this.(DataFlow::PropRef).getPropertyNameExpr() = p.getNameExpr() and + // The binding path refers to this model + bindingPath.getAbsolutePath() = model.getPathString(p) + ) + or + /* The URI string to the JSONModel constructor call */ + exists(JsonModel model | + this = model.getArgument(0) and + this.asExpr() instanceof StringLiteral and + bindingPath.getAbsolutePath() = model.getPathString() + ) ) - or - /* The URI string to the JSONModel constructor call */ - exists(JsonModel model | - this = model.getArgument(0) and - this.asExpr() instanceof StringLiteral and - bindingPath.getAbsolutePath() = model.getPathString() - )) } } @@ -112,9 +117,7 @@ module UI5DataFlow { class UI5ModelSource extends UI5DataFlow::UI5BoundNode, RemoteFlowSource { UI5ModelSource() { bindingPath = any(UI5View view).getASource() } - override string getSourceType() { - result = "UI5 model remote flow source" - } + override string getSourceType() { result = "UI5 model remote flow source" } } /** diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll index a800a3fc..664c3a3b 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5View.qll @@ -99,7 +99,9 @@ abstract class UI5BindingPath extends Locatable { result.getPropertyNameExpr() = p.getNameExpr() and this.getAbsolutePath() = model.getPathString(p) and //restrict search inside the same webapp - exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) + exists(WebApp webApp | + webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile() + ) ) // TODO /* @@ -143,7 +145,9 @@ abstract class UI5View extends File { // The controller name should match result.getName() = this.getControllerName() and // The View and the Controller are in a same webapp - exists(WebApp webApp | webApp.getAResource() = this and webApp.getAResource() = result.getFile()) + exists(WebApp webApp | + webApp.getAResource() = this and webApp.getAResource() = result.getFile() + ) } abstract UI5BindingPath getASource(); @@ -493,7 +497,10 @@ class XmlView extends UI5View, XmlFile { // or a custom control with implementation code found in the webapp exists(CustomControl control | control.getName() = element.getNamespace().getUri() + "." + element.getName() and - exists(WebApp webApp | webApp.getAResource() = control.getFile() and webApp.getAResource() = element.getFile()) + exists(WebApp webApp | + webApp.getAResource() = control.getFile() and + webApp.getAResource() = element.getFile() + ) ) ) ) @@ -563,20 +570,26 @@ class XmlControl extends UI5Control instanceof XmlElement { override CustomControl getDefinition() { result.getName() = this.getQualifiedType() and - exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) + exists(WebApp webApp | + webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile() + ) } bindingset[propName] override MethodCallNode getARead(string propName) { // TODO: in same view - exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) and + exists(WebApp webApp | + webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile() + ) and result.getMethodName() = "get" + capitalize(propName) } bindingset[propName] override MethodCallNode getAWrite(string propName) { // TODO: in same view - exists(WebApp webApp | webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile()) and + exists(WebApp webApp | + webApp.getAResource() = this.getFile() and webApp.getAResource() = result.getFile() + ) and result.getMethodName() = "set" + capitalize(propName) } From 3edb117dd12ae19ec5e6703a443cd67d9aeccb58 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Fri, 27 Oct 2023 15:24:05 -0700 Subject: [PATCH 30/36] Address incorrect association between frame options and web app --- .../javascript/frameworks/ui5/UI5.qll | 13 +++++++ .../javascript/frameworks/ui5/UI5HTML.qll | 6 +-- .../src/UI5Clickjacking/UI5Clickjacking.ql | 38 ++++++++++--------- 3 files changed, 36 insertions(+), 21 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 910f474f..a0d28e4b 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -3,6 +3,7 @@ private import DataFlow private import advanced_security.javascript.frameworks.ui5.JsonParser private import semmle.javascript.security.dataflow.DomBasedXssCustomizations private import advanced_security.javascript.frameworks.ui5.UI5View +private import advanced_security.javascript.frameworks.ui5.UI5HTML module UI5 { bindingset[this] @@ -126,6 +127,18 @@ module UI5 { result.getAbsolutePath() = resolvedModulePath + ".js" ) } + + FrameOptions getFrameOptions() { + exists(HTML::DocumentElement doc | doc.getFile() = this | + result.asHtmlFrameOptions() = coreScript.getAnAttribute() + ) + or + result.asJsFrameOptions().getFile() = this + } + + HTML::DocumentElement getDocument() { + result.getFile() = this + } } /** diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll index 32fadbbb..20521bae 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll @@ -85,10 +85,10 @@ class FrameOptions extends TFrameOptions { } /** - * Holds if the frame options are left untouched as the default value `trusted`. + * Holds if the specified frame options do not prevent click jacking. */ -predicate thereIsNoFrameOptionSet(UI5::WebApp webapp) { - not exists(FrameOptions frameOptions | webapp.getAResource() = frameOptions.getLocation().getFile() | +predicate isMissingFrameOptionsToPreventClickjacking(UI5::WebApp webapp) { + not exists(FrameOptions frameOptions | webapp.getFrameOptions() = frameOptions | frameOptions.allowsSharedOriginEmbedding() or frameOptions.deniesEmbedding() or frameOptions.allowsAllOriginEmbedding() diff --git a/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql b/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql index 69969e5c..af556e9e 100644 --- a/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql +++ b/javascript/frameworks/ui5/src/UI5Clickjacking/UI5Clickjacking.ql @@ -15,44 +15,46 @@ import advanced_security.javascript.frameworks.ui5.UI5HTML import semmle.javascript.RestrictedLocations private import advanced_security.javascript.frameworks.ui5.UI5 -class FirstLineOfMainHtml extends HTML::DocumentElement, FirstLineOf { - FirstLineOfMainHtml() { - exists(UI5::WebApp app | this.getFile().(FirstLineOf).getFile() = app) +class FirstLineOfDocumentElementWebApp extends HTML::DocumentElement, FirstLineOf { + FirstLineOfDocumentElementWebApp() { + exists(UI5::WebApp app | app.getDocument() = this) } } newtype TAlertLocation = TFrameOptions(FrameOptions frameOptions) or - TFirstLineOfMainHtml(FirstLineOfMainHtml htmlStartTag) + TFirstLineOfDocumentElementWebApp(FirstLineOfDocumentElementWebApp htmlStartTag) class AlertLocation extends TAlertLocation { FrameOptions asFrameOptions() { this = TFrameOptions(result) } - FirstLineOfMainHtml asFirstLineOfMainHtml() { this = TFirstLineOfMainHtml(result) } + FirstLineOfDocumentElementWebApp asFirstLineOfDocumentElementWebApp() { this = TFirstLineOfDocumentElementWebApp(result) } string toString() { result = this.asFrameOptions().toString() or - result = this.asFirstLineOfMainHtml().toString() + result = this.asFirstLineOfDocumentElementWebApp().toString() } predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) { this.asFrameOptions().getLocation().hasLocationInfo(path, sl, sc, el, ec) or - this.asFirstLineOfMainHtml().hasLocationInfo(path, sl, sc, el, ec) + this.asFirstLineOfDocumentElementWebApp().hasLocationInfo(path, sl, sc, el, ec) } } -from AlertLocation alert, string message +from AlertLocation alertLocation, string message where - exists(FrameOptions frameOptions | frameOptions.allowsAllOriginEmbedding() | - alert.asFrameOptions() = frameOptions and - message = - "Possible clickjacking vulnerability due to " + frameOptions.toString() + - " being set to `allow`." - ) - or - exists(UI5::WebApp app | thereIsNoFrameOptionSet(app) | - alert.asFirstLineOfMainHtml().getFile() = app and + exists(UI5::WebApp app | + exists(FrameOptions frameOptions | app.getFrameOptions() = frameOptions | + frameOptions.allowsAllOriginEmbedding() and + alertLocation.asFrameOptions() = frameOptions and + message = + "Possible clickjacking vulnerability due to " + frameOptions.toString() + + " being set to `allow`." + ) + or + isMissingFrameOptionsToPreventClickjacking(app) and + alertLocation.asFirstLineOfDocumentElementWebApp() = app.getDocument() and message = "Possible clickjacking vulnerability due to missing frame options." ) -select alert, message +select alertLocation, message From 09e656d8e74ad594b606e1a83439aae1ef7248a2 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Mon, 20 Nov 2023 10:12:00 -0800 Subject: [PATCH 31/36] Remove redundant class JsonStringReader --- .../lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 6 ------ 1 file changed, 6 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index a0d28e4b..40c3a580 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -6,12 +6,6 @@ private import advanced_security.javascript.frameworks.ui5.UI5View private import advanced_security.javascript.frameworks.ui5.UI5HTML module UI5 { - bindingset[this] - private class JsonStringReader extends string { - bindingset[result] - string read() { result = this } - } - private class ResourceRootPathString extends PathString { SapUiCoreScript coreScript; From 5243a483d53b896ef6281e83cfdab8ba1bb6f52e Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 22 Nov 2023 14:01:16 -0800 Subject: [PATCH 32/36] Simplify ResolvedResourceRoot class --- .../javascript/frameworks/ui5/UI5.qll | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 40c3a580..e542d5e5 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -39,26 +39,24 @@ module UI5 { string toString() { result = this.getName() + ": " + this.getRoot() } } - private newtype TResolvedResourceRoot = - MkResolvedResourceRoot(string name, Folder root, string source) { - exists(ResourceRoot unresolvedRoot, ResourceRootPathString path | - name = unresolvedRoot.getName() and - source = unresolvedRoot.getSource() and - path = unresolvedRoot.getRoot() and - root = path.resolve(path.getARootFolder()).getContainer() - ) + class ResolvedResourceRoot extends Container { + ResourceRoot unresolvedRoot; + ResolvedResourceRoot() { + exists(ResourceRootPathString resourceRootPathString | unresolvedRoot.getRoot() = resourceRootPathString | + this = resourceRootPathString.resolve(resourceRootPathString.getARootFolder()).getContainer()) } - class ResolvedResourceRoot extends TResolvedResourceRoot { - string getName() { this = MkResolvedResourceRoot(result, _, _) } - - Folder getRoot() { this = MkResolvedResourceRoot(_, result, _) } - - string getSource() { this = MkResolvedResourceRoot(_, _, result) } + string getName() { + result = unresolvedRoot.getName() + } - string toString() { result = this.getName() + ": " + this.getRoot() } + string getSource() { + result = unresolvedRoot.getSource() + } - predicate contains(File file) { file.getParentContainer+() = getRoot() } + predicate contains(File file) { + file.getParentContainer+() = this + } } private string getAResourceRootConfig() { @@ -117,7 +115,7 @@ module UI5 { resolvedModulePath = initialModuleResourcePath .regexpReplaceAll("^module\\s*:\\s*", "") - .replaceAll(resourceRoot.getName(), resourceRoot.getRoot().getAbsolutePath()) and + .replaceAll(resourceRoot.getName(), resourceRoot.getAbsolutePath()) and result.getAbsolutePath() = resolvedModulePath + ".js" ) } From 6289acab98305217c639c51ce68a7b26f7092795 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Wed, 22 Nov 2023 14:02:00 -0800 Subject: [PATCH 33/36] Weaken SAP core script pattern Removing the the forward slash ensures we can find the core script located in the same directoryy as the webapp. --- .../ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index e542d5e5..7faadcfb 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -65,7 +65,7 @@ module UI5 { class SapUiCoreScript extends HTML::ScriptElement { SapUiCoreScript() { - this.getSourcePath().matches(["%/sap-ui-core.js", "%sap-ui-core-nojQuery.js"]) + this.getSourcePath().matches(["%sap-ui-core.js", "%sap-ui-core-nojQuery.js"]) } ResourceRoot getAResourceRoot() { From 796c099bdf6336ce4e2c062ab94036a06c38f399 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 28 Nov 2023 14:35:30 -0800 Subject: [PATCH 34/36] Reneame SapUiCoreScript to SapUiCoreScriptElement --- .../javascript/frameworks/ui5/UI5.qll | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 7faadcfb..1aa50edf 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -7,7 +7,7 @@ private import advanced_security.javascript.frameworks.ui5.UI5HTML module UI5 { private class ResourceRootPathString extends PathString { - SapUiCoreScript coreScript; + SapUiCoreScriptElement coreScript; ResourceRootPathString() { this = coreScript.getAResourceRoot().getRoot() } @@ -18,7 +18,7 @@ module UI5 { MkResourceRoot(string name, string root, string source) { exists( JsonParser::JsonObject config, - JsonParser::JsonMember configEntry, SapUiCoreScript coreScript + JsonParser::JsonMember configEntry, SapUiCoreScriptElement coreScript | source = coreScript.getAttributeByName("data-sap-ui-resourceroots").getValue() and source = config.getSource() and @@ -60,11 +60,11 @@ module UI5 { } private string getAResourceRootConfig() { - result = any(SapUiCoreScript script).getAttributeByName("data-sap-ui-resourceroots").getValue() + result = any(SapUiCoreScriptElement script).getAttributeByName("data-sap-ui-resourceroots").getValue() } - class SapUiCoreScript extends HTML::ScriptElement { - SapUiCoreScript() { + class SapUiCoreScriptElement extends HTML::ScriptElement { + SapUiCoreScriptElement() { this.getSourcePath().matches(["%sap-ui-core.js", "%sap-ui-core-nojQuery.js"]) } @@ -91,7 +91,7 @@ module UI5 { /** A UI5 bootstrapped web application. */ class WebApp extends HTML::HtmlFile { - SapUiCoreScript coreScript; + SapUiCoreScriptElement coreScript; WebApp() { coreScript.getFile() = this } From ceeffa02807c4e644bdfb1581f6c9eacbc5cf886 Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 28 Nov 2023 14:37:14 -0800 Subject: [PATCH 35/36] Add QLdoc to getInitialModule --- .../lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll index 1aa50edf..c0686c0f 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll @@ -105,6 +105,9 @@ module UI5 { WebAppManifest getManifest() { result.getWebapp() = this } + /** + * Gets the JavaScript module that serves as an entrypoint to this webapp. + */ File getInitialModule() { exists( string initialModuleResourcePath, string resolvedModulePath, From 5f949643e85add306f010463ae7b00588ba8093d Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 28 Nov 2023 14:40:14 -0800 Subject: [PATCH 36/36] Clarify QLdoc of isMissingFrameOptionsToPreventClickjacking --- .../lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll index 20521bae..96d9f130 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5HTML.qll @@ -85,7 +85,7 @@ class FrameOptions extends TFrameOptions { } /** - * Holds if the specified frame options do not prevent click jacking. + * Holds if there are no frame options specified to prevent click jacking. */ predicate isMissingFrameOptionsToPreventClickjacking(UI5::WebApp webapp) { not exists(FrameOptions frameOptions | webapp.getFrameOptions() = frameOptions |