From 0f554bd546c975ec7eb7a9f2434fda8da206150c Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 2 Apr 2025 10:33:15 +0100 Subject: [PATCH 1/3] UI5: Address join order issues with inSameWebApp Applying inSameWebApp too early in the pipeline was causing a cross-product on all data flow nodes in a WebApp. It's typically better to apply the other conditions (e.g. name matching), then filter on the same web-app as a final step. --- .../ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll | 2 ++ 1 file changed, 2 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 8c1e9482e..80cc92724 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 @@ -78,6 +78,8 @@ class WebAppManifest extends File { WebApp getWebapp() { result = webapp } } +bindingset[f1, f2] +pragma[inline_late] predicate inSameWebApp(File f1, File f2) { exists(WebApp webApp | webApp.getAResource() = f1 and webApp.getAResource() = f2) } From ed87aa9b03f7a58c83edac8e816c91936c6775af Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 2 Apr 2025 11:57:09 +0100 Subject: [PATCH 2/3] BindingStringParser: Address perf issues in Token operations Avoid creating a cross-product of tokens identified in a string. Instead, create an ordering of tokens based on begin/end locations and process those linearly to find the first/next/contains tokens. --- .../frameworks/ui5/BindingStringParser.qll | 81 ++++++++++++++++--- 1 file changed, 70 insertions(+), 11 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll index 629d76365..b516da1a3 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll @@ -372,26 +372,85 @@ module BindingStringParser { this = MkDoubleQuoteToken(_, _, _, result) } - Token getNext() { - result.getBegin() = this.getEnd() + 1 and - result.getReader() = this.getReader() + /** + * Gets the next contiguous token after the end of this token. + */ + Token getNextAfterEnd() { + // Find the last contained token, then get the next token after that + result = getNextAfterContained() and + // Ensure contiguous tokens + consecutiveToken(this, result) } - predicate isFirst() { - not exists(Token other | - other.getBegin() < this.getBegin() and other.getReader() = this.getReader() + bindingset[t1, t2] + pragma[inline_late] + private predicate consecutiveToken(Token t1, Token t2) { t1.getEnd() + 1 = t2.getBegin() } + + /** + * Gets the next token that occurs after the start of this token. + */ + private Token getNext() { + exists(int pos | + tokenOrdering(this.getReader(), this, pos) and + tokenOrdering(this.getReader(), result, pos + uniqueTokensAtPosition(this.getReader(), pos)) ) } + /** + * Get the last token contained by this token. + */ + pragma[noinline] + private Token getNextAfterContained() { + exists(Token lastToken | + contains(lastToken) and + result = lastToken.getNext() and + not contains(result) + ) + } + + predicate isFirst() { tokenOrdering(this.getReader(), this, 1) } + predicate contains(Token t) { - this.getReader() = t.getReader() and - this.getBegin() < t.getBegin() and - this.getEnd() > t.getEnd() + // base case, every token contains itself + t = this + or + // Recursive case, find an existing token contained by this token, and determine whether the next token is + // contained + exists(Token prev | + this.contains(prev) and + prev.getNext() = t and + // In the case of overlapping ranges, report tokens that begin inside this token as "contained" + this.getEnd() >= t.getBegin() + ) } stdlib::Location getLocation() { result = getReader().getLocation() } } + /** + * Holds if `t` is ordered at `position` according to the location of the beginning of the token. + * + * Note: `position` is not contiguous as certain strings may be matched by multiple tokens. In this + * case those tokens will all have the same `position`, and the subsequent token will have + * `position + count(tokens_with_current_position)`. + */ + private predicate tokenOrdering(BindingStringReader reader, Token t, int position) { + t = + rank[position](Token token | + token.getReader() = reader + | + token order by token.getBegin(), token.getEnd() + ) + } + + /** + * Identify how many tokens are at a given position in the ordering, i.e. have the same beginning and end. + */ + private int uniqueTokensAtPosition(BindingStringReader reader, int position) { + tokenOrdering(reader, _, position) and + result = count(Token t | tokenOrdering(reader, t, position)) + } + private class WhiteSpaceToken extends Token, MkWhiteSpaceToken { } private class CommaToken extends Token, MkCommaToken { } @@ -431,10 +490,10 @@ module BindingStringParser { private class DotToken extends Token, MkDot { } private Token getNextSkippingWhitespace(Token t) { - result = t.getNext() and + result = t.getNextAfterEnd() and not result instanceof WhiteSpaceToken or - exists(WhiteSpaceToken ws | ws = t.getNext() | result = getNextSkippingWhitespace(ws)) + exists(WhiteSpaceToken ws | ws = t.getNextAfterEnd() | result = getNextSkippingWhitespace(ws)) } private newtype TMemberList = From d13ef5e146f47da41b7be23e1548f6916cb55e68 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 2 Apr 2025 20:11:10 +0100 Subject: [PATCH 3/3] BindingStringParser: Match previous behaviour - Consider all tokens beginning at the same location as eligible to be the next token. - Implement strictContains to reflect previous behaviour. --- .../frameworks/ui5/BindingStringParser.qll | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll index b516da1a3..9c265dd2d 100644 --- a/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll +++ b/javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/BindingStringParser.qll @@ -424,6 +424,13 @@ module BindingStringParser { ) } + /** + * The token `t` is completely contained within this outer token. + */ + predicate strictContains(Token t) { + this.contains(t) and t.getBegin() > this.getBegin() and t.getEnd() < this.getEnd() + } + stdlib::Location getLocation() { result = getReader().getLocation() } } @@ -435,12 +442,7 @@ module BindingStringParser { * `position + count(tokens_with_current_position)`. */ private predicate tokenOrdering(BindingStringReader reader, Token t, int position) { - t = - rank[position](Token token | - token.getReader() = reader - | - token order by token.getBegin(), token.getEnd() - ) + t = rank[position](Token token | token.getReader() = reader | token order by token.getBegin()) } /** @@ -684,15 +686,15 @@ module BindingStringParser { private newtype TValue = MkNumber(float n, Token source) { exists(NumberToken t | t.getValue().toFloat() = n and source = t | - not any(StringToken str).contains(t) and - not any(NameToken name).contains(t) + not any(StringToken str).strictContains(t) and + not any(NameToken name).strictContains(t) ) } or MkString(string s, Token source) { exists(StringToken t | t.(Token).getValue() = s and t = source and - not any(NameToken nameToken).contains(t) + not any(NameToken nameToken).strictContains(t) ) } or MkObject(MemberList members, Token source) { @@ -721,27 +723,27 @@ module BindingStringParser { } or MkTrue(Token source) { exists(TrueToken t | - not any(StringToken str).contains(t) and - not any(NameToken nameToken).contains(t) and + not any(StringToken str).strictContains(t) and + not any(NameToken nameToken).strictContains(t) and source = t ) } or MkFalse(Token source) { exists(FalseToken t | - not any(StringToken str).contains(t) and - not any(NameToken nameToken).contains(t) and + not any(StringToken str).strictContains(t) and + not any(NameToken nameToken).strictContains(t) and source = t ) } or MkNull(Token source) { exists(NullToken t | - not any(StringToken str).contains(t) and - not any(NameToken nameToken).contains(t) and + not any(StringToken str).strictContains(t) and + not any(NameToken nameToken).strictContains(t) and source = t ) } or MkName(Token source) { - exists(NameToken t | not any(StringToken str).contains(t) and source = t) + exists(NameToken t | not any(StringToken str).strictContains(t) and source = t) } or MkIdent(Token source) { exists(IdentToken t | source = t and getNextSkippingWhitespace(t) instanceof ColonToken)