Skip to content

Add sensitive information exposure query #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/javascript.sarif.expected

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,17 @@ abstract class CdlElement extends JsonObject {
CdlAnnotation getAnAnnotation() { result = this.getAnnotation(_) }
}

class CdlEntityField extends CdlElement {
string name;
CdlKind kind;

CdlEntityField() { exists(CdlDefinition definition | this = definition.getElement(name)) }

override string getName() { result = name }

override CdlKind getKind() { result = kind }
}

class CdlService extends CdlElement {
string name;
CdlKind kind;
Expand Down Expand Up @@ -127,11 +138,46 @@ class CdlAttribute extends JsonObject {
string getType() { result = this.getPropStringValue("type") }

int getLength() { result = this.getPropValue("length").(JsonPrimitiveValue).getIntValue() }

string getName() { result = name }
}

/**
* any `JsonValue` that has a `PersonalData` like annotation above it
*/
abstract class SensitiveAnnotatedElement extends JsonValue {
abstract string getName();
}

abstract class CdlAnnotation extends JsonValue {
class SensitiveAnnotatedEntity extends SensitiveAnnotatedElement instanceof CdlEntity {
SensitiveAnnotatedEntity() { exists(PersonalDataAnnotation a | a.getQualifiedElement() = this) }

override string getName() { result = this.(CdlEntity).getName() }

string getShortName() { result = this.getName().regexpCapture(".*\\.([^\\.]+$)", 1) }
}

class SensitiveAnnotatedAttribute extends SensitiveAnnotatedElement instanceof CdlAttribute {
SensitiveAnnotatedAttribute() {
exists(PersonalDataAnnotation a | a.getQualifiedElement() = this)
}

override string getName() { result = this.(CdlAttribute).getName() }
}

/**
* CDL annotations for PersonalData
*/
class PersonalDataAnnotation extends CdlAnnotation {
PersonalDataAnnotation() { this.getName().matches("PersonalData%") }
}

/**
* CDL annotations specifically associated to `CdlElement`s
*/
class CdlAnnotation extends JsonValue {
string annotationName;
CdlElement element;
JsonValue element;

CdlAnnotation() {
this = element.getPropValue(annotationName) and
Expand All @@ -146,7 +192,7 @@ abstract class CdlAnnotation extends JsonValue {
/**
* Gets the CDL Element that this annotation is attached to.
*/
CdlElement getQualifiedElement() { result = element }
JsonValue getQualifiedElement() { result = element }
}

class ProtocolAnnotation extends CdlAnnotation {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# CAP Insertion of Sensitive Information into Log File

If sensitive information is written to a log entry using the CAP Node.js logging API, a malicious user may be able to gain access to user data.

Data annotated as `@PersonalData` should not be logged.

## Recommendation

CAP applications should not log sensitive information. Check CDS declarations for annotations before logging certain data types or fields.

## Examples

This CAP service directly logs the sensitive information.

```cds
namespace advanced_security.log_exposure.sample_entities;

entity Sample {
name : String(111);
}

// annotations for Data Privacy
annotate Sample with
@PersonalData : { DataSubjectRole : 'Sample', EntitySemantics : 'DataSubject' }
{
name @PersonalData.IsPotentiallySensitive;
}
```

``` javascript
import cds from '@sap/cds'
const LOG = cds.log("logger");

const { Sample } = cds.entities('advanced_security.log_exposure.sample_entities')

class SampleVulnService extends cds.ApplicationService {
init() {
LOG.info("Received: ", Sample.name); // CAP log exposure alert
}
}
```

## References

- OWASP 2021: [Security Logging and Monitoring Failures](https://owasp.org/Top10/A09_2021-Security_Logging_and_Monitoring_Failures/).
- OWASP: [Logging Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html).
- OWASP: [User Privacy Protection Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/User_Privacy_Protection_Cheat_Sheet.html).
- SAP CAPire Documentation: [PersonalData Annotations](https://cap.cloud.sap/docs/guides/data-privacy/annotations).
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* @name Insertion of sensitive information into log files
* @description Writing sensitive information to log files can allow that
* information to be leaked to an attacker more easily.
* @kind path-problem
* @problem.severity warning
* @security-severity 7.5
* @precision medium
* @id javascript/sensitive-log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* @tags security
* external/cwe/cwe-532
*/

import javascript
import advanced_security.javascript.frameworks.cap.CDS
import advanced_security.javascript.frameworks.cap.CAPLogInjectionQuery
import DataFlow::PathGraph

class SensitiveExposureSource extends DataFlow::Node {
SensitiveExposureSource() {
exists(PropRead p, SensitiveAnnotatedElement c |
Copy link
Contributor

@mbaluda mbaluda Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a problem here as you are only comparing on the property name (name in the test vs Sample.name)
There can be multiple entities with the same names as well as multiple applications in the same repo...
I think you can use getCdsDeclaration()

p.getPropertyName() = c.getName() and
this = p
)
}
}

class SensitiveLogExposureConfig extends TaintTracking::Configuration {
SensitiveLogExposureConfig() { this = "SensitiveLogExposure" }

override predicate isSource(DataFlow::Node source) { source instanceof SensitiveExposureSource }

override predicate isSink(DataFlow::Node sink) { sink instanceof CdsLogSink }
}

from SensitiveLogExposureConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink)
select sink, source, sink, "Log entry depends on a potentially sensitive piece of information."
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
namespace advanced_security.log_exposure.sample_entities;

entity Sample {
name : String(111);
dateOfBirth : Date;
}

// annotations for Data Privacy
annotate Sample with
@PersonalData : { DataSubjectRole : 'Sample', EntitySemantics : 'DataSubject' }
{
name @PersonalData.IsPotentiallySensitive;
dateOfBirth @PersonalData.IsPotentiallyPersonal;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
nodes
| sensitive-exposure.js:10:32:10:42 | Sample.name |
| sensitive-exposure.js:10:32:10:42 | Sample.name |
| sensitive-exposure.js:10:32:10:42 | Sample.name |
edges
| sensitive-exposure.js:10:32:10:42 | Sample.name | sensitive-exposure.js:10:32:10:42 | Sample.name |
#select
| sensitive-exposure.js:10:32:10:42 | Sample.name | sensitive-exposure.js:10:32:10:42 | Sample.name | sensitive-exposure.js:10:32:10:42 | Sample.name | Log entry depends on a potentially sensitive piece of information. |
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import cds from '@sap/cds'
const LOG = cds.log("logger");

const { Sample } = cds.entities('advanced_security.log_exposure.sample_entities')

class SampleVulnService extends cds.ApplicationService {
init() {
/* A sensitive info log sink. */

LOG.info("Received: ", Sample.name); // CAP log exposure alert

Check failure

Code scanning / CodeQL

Insertion of sensitive information into log files High test

Log entry depends on a potentially sensitive piece of information.
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
sensitive-exposure/SensitiveExposure.ql
Empty file modified scripts/compile-all-cds-in-directory.sh
100644 → 100755
Empty file.
Empty file modified scripts/create-db-with-cds.sh
100644 → 100755
Empty file.