From 489e23c2724dfd8c8a5f99a3306ced6a4f25b7be Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Thu, 14 Nov 2024 16:25:51 +0000 Subject: [PATCH 1/6] Refine locations in the CDS file - Exclude objects without a $locations property - Use the known name to deduce the end column --- .../javascript/frameworks/cap/CDL.qll | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll index 934371008..0510a3bd5 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll @@ -5,23 +5,7 @@ import javascript import advanced_security.javascript.frameworks.cap.CDS -abstract class CdlObject extends JsonObject { - predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) { - exists(Location loc, JsonValue locValue | - loc = this.getLocation() and - locValue = this.getPropValue("$location") and - path = - any(File f | - f.getAbsolutePath() - .matches("%" + locValue.getPropValue("file").getStringValue() + ".json") - ).getAbsolutePath().regexpReplaceAll("\\.json$", "") and - sl = locValue.getPropValue("line").getIntValue() and - sc = locValue.getPropValue("col").getIntValue() and - el = sl + 1 and - ec = 1 - ) - } -} +abstract class CdlObject extends JsonObject { } private newtype CdlKind = CdlServiceKind(string value) { value = "service" } or @@ -47,6 +31,29 @@ abstract class CdlElement extends CdlObject { CdlElement() { exists(CdlDefinition definition | this = definition.getElement(name)) } + predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) { + // If the cds.json file has a $location property, then use that, + // otherwise fall back to the cds.json file itself + if exists(this.getPropValue("$location")) + then + exists(Location loc, JsonValue locValue | + loc = this.getLocation() and + locValue = this.getPropValue("$location") and + path = + any(File f | + f.getAbsolutePath() + .matches("%" + locValue.getPropValue("file").getStringValue() + ".json") + ).getAbsolutePath().regexpReplaceAll("\\.json$", "") and + sl = locValue.getPropValue("line").getIntValue() and + sc = locValue.getPropValue("col").getIntValue() and + el = sl and + // Currently $locations does not provide an end location. However, we can + // automatically deduce the end location from the length of the name. + ec = sc + getUnqualifiedName().length() - 1 + ) + else super.getLocation().hasLocationInfo(path, sl, sc, el, ec) + } + /** * Gets the name of this CDL element. */ From c2b47167175957c636741f63c8ce18dd8cfda499 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 18 Nov 2024 13:41:50 +0000 Subject: [PATCH 2/6] Rename CDLDefinition to CDLDefinitions for accuracy --- .../javascript/frameworks/cap/CDL.qll | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll index 0510a3bd5..8e7829dd3 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll @@ -15,21 +15,24 @@ private newtype CdlKind = CdlFunctionKind(string value) { value = "function" } /** - * Any CDL element, including entities, event, actions, and more. + * A list of CDL definitions, which can include entities, events, actions and more. */ -class CdlDefinition extends CdlObject { - CdlDefinition() { exists(JsonObject root | this = root.getPropValue("definitions")) } +class CdlDefinitions extends CdlObject { + CdlDefinitions() { exists(JsonObject root | this = root.getPropValue("definitions")) } JsonObject getElement(string elementName) { result = this.getPropValue(elementName) } JsonObject getAnElement() { result = this.getElement(_) } } +/** + * A CDL definition element. + */ abstract class CdlElement extends CdlObject { CdlKind kind; string name; - CdlElement() { exists(CdlDefinition definition | this = definition.getElement(name)) } + CdlElement() { exists(CdlDefinitions definitions | this = definitions.getElement(name)) } predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) { // If the cds.json file has a $location property, then use that, From f503dcda11333583a590b99208388552fc4962f5 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 18 Nov 2024 14:44:51 +0000 Subject: [PATCH 3/6] Move the use of CDL locations back to CdlObject CdlAttributes extend CdlObject, not CdlElement, but still require location sniffing. --- .../javascript/frameworks/cap/CDL.qll | 64 ++++++++++++------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll index 8e7829dd3..0f47c5bd9 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll @@ -5,7 +5,44 @@ import javascript import advanced_security.javascript.frameworks.cap.CDS -abstract class CdlObject extends JsonObject { } +abstract class CdlObject extends JsonObject { + predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) { + // If the cds.json file has a $location property, then use that, + // otherwise fall back to the cds.json file itself + if exists(this.getPropValue("$location")) + then + exists(Location loc, JsonValue locValue | + loc = this.getLocation() and + locValue = this.getPropValue("$location") and + path = + any(File f | + f.getAbsolutePath() + .matches("%" + locValue.getPropValue("file").getStringValue() + ".json") + ).getAbsolutePath().regexpReplaceAll("\\.json$", "") and + sl = locValue.getPropValue("line").getIntValue() and + sc = locValue.getPropValue("col").getIntValue() and + if exists(getObjectLocationName()) + then + // Currently $locations does not provide an end location. However, we can + // automatically deduce the end location from the length of the name. + el = sl and + ec = sc + getObjectLocationName().length() - 1 + else ( + // We don't know where this entity ends, so mark the whole line + el = sl + 1 and + ec = 1 + ) + ) + else super.getLocation().hasLocationInfo(path, sl, sc, el, ec) + } + + /** + * The name of the object that should be highlighted as the location. + * + * This is used to deduce the length of the location. + */ + string getObjectLocationName() { none() } +} private newtype CdlKind = CdlServiceKind(string value) { value = "service" } or @@ -34,28 +71,7 @@ abstract class CdlElement extends CdlObject { CdlElement() { exists(CdlDefinitions definitions | this = definitions.getElement(name)) } - predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) { - // If the cds.json file has a $location property, then use that, - // otherwise fall back to the cds.json file itself - if exists(this.getPropValue("$location")) - then - exists(Location loc, JsonValue locValue | - loc = this.getLocation() and - locValue = this.getPropValue("$location") and - path = - any(File f | - f.getAbsolutePath() - .matches("%" + locValue.getPropValue("file").getStringValue() + ".json") - ).getAbsolutePath().regexpReplaceAll("\\.json$", "") and - sl = locValue.getPropValue("line").getIntValue() and - sc = locValue.getPropValue("col").getIntValue() and - el = sl and - // Currently $locations does not provide an end location. However, we can - // automatically deduce the end location from the length of the name. - ec = sc + getUnqualifiedName().length() - 1 - ) - else super.getLocation().hasLocationInfo(path, sl, sc, el, ec) - } + override string getObjectLocationName() { result = getUnqualifiedName() } /** * Gets the name of this CDL element. @@ -225,6 +241,8 @@ class CdlAttribute extends CdlObject { exists(CdlElement entity | this = entity.getPropValue("elements").getPropValue(name)) } + override string getObjectLocationName() { result = getName() } + string getType() { result = this.getPropStringValue("type") } int getLength() { result = this.getPropValue("length").(JsonPrimitiveValue).getIntValue() } From 3b12cdf7e5be1218eed048967639a135567b54f1 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 18 Nov 2024 15:23:35 +0000 Subject: [PATCH 4/6] Handle missing location data * Use the file location only if no line/col data * Use the col start line if available, otherwise choose col 0 * Use the end line deduction if available, otherwise use 1 past the start --- .../javascript/frameworks/cap/CDL.qll | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll index 0f47c5bd9..f4811c9ff 100644 --- a/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll +++ b/javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll @@ -19,18 +19,35 @@ abstract class CdlObject extends JsonObject { f.getAbsolutePath() .matches("%" + locValue.getPropValue("file").getStringValue() + ".json") ).getAbsolutePath().regexpReplaceAll("\\.json$", "") and - sl = locValue.getPropValue("line").getIntValue() and - sc = locValue.getPropValue("col").getIntValue() and - if exists(getObjectLocationName()) + if + not exists(locValue.getPropValue("line")) and + not exists(locValue.getPropValue("col")) then - // Currently $locations does not provide an end location. However, we can - // automatically deduce the end location from the length of the name. - el = sl and - ec = sc + getObjectLocationName().length() - 1 + // We don't know where this entity starts, so mark the whole file + sl = 0 and + sc = 0 and + el = 0 and + ec = 0 else ( - // We don't know where this entity ends, so mark the whole line - el = sl + 1 and - ec = 1 + sl = locValue.getPropValue("line").getIntValue() and + ( + if exists(locValue.getPropValue("col")) + then sc = locValue.getPropValue("col").getIntValue() + else + // We don't know where this entity starts, so mark the start of the line + sc = 0 + ) and + el = sl and + ( + if exists(getObjectLocationName()) + then + // Currently $locations does not provide an end location. However, we can + // automatically deduce the end location from the length of the name. + ec = sc + getObjectLocationName().length() - 1 + else + // Mark a single character if we cannot predicate the length + ec = sc + 1 + ) ) ) else super.getLocation().hasLocationInfo(path, sl, sc, el, ec) From 5ef04fa26bcc83bea5c68ead81315dbcb0576240 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 18 Nov 2024 15:26:24 +0000 Subject: [PATCH 5/6] Update expected results --- .../entities-exposed-with-no-authz.expected | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/javascript/frameworks/cap/test/queries/bad-authn-authz/entities-with-no-authz/entities-exposed-with-no-authz/entities-exposed-with-no-authz.expected b/javascript/frameworks/cap/test/queries/bad-authn-authz/entities-with-no-authz/entities-exposed-with-no-authz/entities-exposed-with-no-authz.expected index 0a226b09d..b1b701a87 100644 --- a/javascript/frameworks/cap/test/queries/bad-authn-authz/entities-with-no-authz/entities-exposed-with-no-authz/entities-exposed-with-no-authz.expected +++ b/javascript/frameworks/cap/test/queries/bad-authn-authz/entities-with-no-authz/entities-exposed-with-no-authz/entities-exposed-with-no-authz.expected @@ -1,6 +1,6 @@ -| srv/service1.cds:3:9:4:1 | {\\n ... }\\n } | The CDS service `Service1` is exposed without any authentication. | -| srv/service1.cds:5:10:6:1 | {\\n ... }\\n } | The CDS entity `Service1.Service1Entity` is exposed without any authentication. | -| srv/service1.cds:8:10:9:1 | {\\n ... }\\n } | The CDS action `Service1.send1` is exposed without any authentication. | -| srv/service2.cds:3:9:4:1 | {\\n ... }\\n } | The CDS service `Service2` is exposed without any authentication. | -| srv/service2.cds:5:10:6:1 | {\\n ... }\\n } | The CDS entity `Service2.Service2Entity` is exposed without any authentication. | -| srv/service2.cds:8:10:9:1 | {\\n ... }\\n } | The CDS action `Service2.send2` is exposed without any authentication. | +| srv/service1.cds:3:9:3:16 | {\\n ... }\\n } | The CDS service `Service1` is exposed without any authentication. | +| srv/service1.cds:5:10:5:23 | {\\n ... }\\n } | The CDS entity `Service1.Service1Entity` is exposed without any authentication. | +| srv/service1.cds:8:10:8:14 | {\\n ... }\\n } | The CDS action `Service1.send1` is exposed without any authentication. | +| srv/service2.cds:3:9:3:16 | {\\n ... }\\n } | The CDS service `Service2` is exposed without any authentication. | +| srv/service2.cds:5:10:5:23 | {\\n ... }\\n } | The CDS entity `Service2.Service2Entity` is exposed without any authentication. | +| srv/service2.cds:8:10:8:14 | {\\n ... }\\n } | The CDS action `Service2.send2` is exposed without any authentication. | \ No newline at end of file From 0084839e9a649a7c515abf0c6332e569500124e0 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 18 Nov 2024 23:26:02 +0000 Subject: [PATCH 6/6] Fixup expected results --- .../test/queries/sensitive-exposure/sensitive-exposure.expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected b/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected index c1efa56fc..85d27ac4f 100644 --- a/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected +++ b/javascript/frameworks/cap/test/queries/sensitive-exposure/sensitive-exposure.expected @@ -5,4 +5,4 @@ nodes edges | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | #select -| sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | Log entry depends on the $@ field which is annotated as potentially sensitive. | sensitive-exposure.cds:4:5:5:1 | {\\n ... } | name | +| sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | sensitive-exposure.js:9:32:9:42 | Sample.name | Log entry depends on the $@ field which is annotated as potentially sensitive. | sensitive-exposure.cds:4:5:4:8 | {\\n ... } | name | \ No newline at end of file