-
Notifications
You must be signed in to change notification settings - Fork 2
Add sensitive exposure split query #207
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
Open
knewbury01
wants to merge
5
commits into
main
Choose a base branch
from
knewbury01/cap-sensitive-sources
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
2915d6b
Add sensitive exposure split query
knewbury01 d7995ce
Rename sensitive-exposure-likely to heuristic
knewbury01 ad9e4fe
Change query id SensitiveExposureHeuristicSource.ql
knewbury01 c6ad123
Address review comments
knewbury01 0291022
Address review comments second round
knewbury01 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
44 changes: 44 additions & 0 deletions
44
...cript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
# 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 that may expose system information such as full path names, system information, usernames and passwords should not be logged. | ||
|
||
This query is similar to `js/cap-sensitive-log` in that the sinks are CAP logging facilities. The sources however are the same (exclusively) as the out of the box CodeQL query for [clear text logging](https://codeql.github.com/codeql-query-help/javascript/js-clear-text-logging/). | ||
|
||
## Recommendation | ||
|
||
CAP applications should not log sensitive information. Sensitive information can include: full path names, system information, usernames, passwords or any personally identifiable information. Make sure to log only information that is not sensitive, or obfuscate/encrypt sensitive information any time that it is logged. | ||
|
||
## Examples | ||
|
||
This CAP service directly logs the sensitive information. Potential attackers may gain access to this sensitive information when the log output is displayed or when the attacker gains access to the log, and the info is not obfuscated or encrypted. | ||
|
||
``` javascript | ||
import cds from '@sap/cds' | ||
const LOG = cds.log("logger"); | ||
|
||
class SampleVulnService extends cds.ApplicationService { | ||
init() { | ||
LOG.info(`[INFO] Environment: ${JSON.stringify(process.env)}`); // CAP log exposure alert | ||
var obj = { | ||
x: password | ||
}; | ||
|
||
LOG.info(obj); // CAP log exposure alert | ||
|
||
LOG.info(obj.x.replace(/./g, "*")); // NO CAP log exposure alert - replace call acts as sanitizer | ||
|
||
var user = { | ||
password: encryptLib.encryptPassword(password) | ||
}; | ||
LOG.info(user); // NO CAP log exposure alert - the data is encrypted | ||
} | ||
} | ||
``` | ||
|
||
## 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). |
44 changes: 44 additions & 0 deletions
44
javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/** | ||
* @name Insertion of sensitive information into log files | ||
* @description Writing heuristically 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 low | ||
* @id js/cap-sensitive-log-heurisitic-source | ||
* @tags security | ||
* external/cwe/cwe-532 | ||
*/ | ||
|
||
import javascript | ||
import advanced_security.javascript.frameworks.cap.CDS | ||
import advanced_security.javascript.frameworks.cap.CAPLogInjectionQuery | ||
private import semmle.javascript.security.dataflow.CleartextLoggingCustomizations::CleartextLogging as CleartextLogging | ||
|
||
module SensitiveLogExposureConfig implements DataFlow::ConfigSig { | ||
predicate isSource(DataFlow::Node source) { source instanceof CleartextLogging::Source } | ||
|
||
predicate isSink(DataFlow::Node sink) { sink instanceof CdsLogSink } | ||
|
||
predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg) { | ||
CleartextLogging::isAdditionalTaintStep(src, trg) | ||
} | ||
|
||
predicate isBarrier(DataFlow::Node sink) { sink instanceof CleartextLogging::Barrier } | ||
|
||
predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet contents) { | ||
// Assume all properties of a logged object are themselves logged. | ||
contents = DataFlow::ContentSet::anyProperty() and | ||
isSink(node) | ||
} | ||
} | ||
|
||
module SensitiveLogExposureFlow = TaintTracking::Global<SensitiveLogExposureConfig>; | ||
|
||
import SensitiveLogExposureFlow::PathGraph | ||
|
||
from SensitiveLogExposureFlow::PathNode source, SensitiveLogExposureFlow::PathNode sink | ||
where SensitiveLogExposureFlow::flowPath(source, sink) | ||
select sink, source, sink, "This logs sensitive data returned by $@ as clear text.", | ||
source.getNode(), source.getNode().(CleartextLogging::Source).describe() |
18 changes: 18 additions & 0 deletions
18
...ive-exposure/sensitive-exposure-js-all-sinks/sensitive-exposure-heuristic-source.expected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
edges | ||
| sensitive-exposure-heuristic-source.js:6:41:6:67 | JSON.st ... ss.env) | sensitive-exposure-heuristic-source.js:6:18:6:69 | `[INFO] ... .env)}` | provenance | | | ||
| sensitive-exposure-heuristic-source.js:6:56:6:66 | process.env | sensitive-exposure-heuristic-source.js:6:41:6:67 | JSON.st ... ss.env) | provenance | | | ||
| sensitive-exposure-heuristic-source.js:8:13:10:9 | obj [x] | sensitive-exposure-heuristic-source.js:11:18:11:20 | obj | provenance | | | ||
| sensitive-exposure-heuristic-source.js:8:19:10:9 | {\\n ... } [x] | sensitive-exposure-heuristic-source.js:8:13:10:9 | obj [x] | provenance | | | ||
| sensitive-exposure-heuristic-source.js:9:16:9:23 | password | sensitive-exposure-heuristic-source.js:8:19:10:9 | {\\n ... } [x] | provenance | | | ||
nodes | ||
| sensitive-exposure-heuristic-source.js:6:18:6:69 | `[INFO] ... .env)}` | semmle.label | `[INFO] ... .env)}` | | ||
| sensitive-exposure-heuristic-source.js:6:41:6:67 | JSON.st ... ss.env) | semmle.label | JSON.st ... ss.env) | | ||
| sensitive-exposure-heuristic-source.js:6:56:6:66 | process.env | semmle.label | process.env | | ||
| sensitive-exposure-heuristic-source.js:8:13:10:9 | obj [x] | semmle.label | obj [x] | | ||
| sensitive-exposure-heuristic-source.js:8:19:10:9 | {\\n ... } [x] | semmle.label | {\\n ... } [x] | | ||
| sensitive-exposure-heuristic-source.js:9:16:9:23 | password | semmle.label | password | | ||
| sensitive-exposure-heuristic-source.js:11:18:11:20 | obj | semmle.label | obj | | ||
subpaths | ||
#select | ||
| sensitive-exposure-heuristic-source.js:6:18:6:69 | `[INFO] ... .env)}` | sensitive-exposure-heuristic-source.js:6:56:6:66 | process.env | sensitive-exposure-heuristic-source.js:6:18:6:69 | `[INFO] ... .env)}` | This logs sensitive data returned by $@ as clear text. | sensitive-exposure-heuristic-source.js:6:56:6:66 | process.env | process environment | | ||
| sensitive-exposure-heuristic-source.js:11:18:11:20 | obj | sensitive-exposure-heuristic-source.js:9:16:9:23 | password | sensitive-exposure-heuristic-source.js:11:18:11:20 | obj | This logs sensitive data returned by $@ as clear text. | sensitive-exposure-heuristic-source.js:9:16:9:23 | password | an access to password | |
20 changes: 20 additions & 0 deletions
20
...sensitive-exposure/sensitive-exposure-js-all-sinks/sensitive-exposure-heuristic-source.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import cds from '@sap/cds' | ||
const LOG = cds.log("logger"); | ||
|
||
class SampleVulnService extends cds.ApplicationService { | ||
init() { | ||
LOG.info(`[INFO] Environment: ${JSON.stringify(process.env)}`); // CAP log exposure alert | ||
|
||
var obj = { | ||
x: password | ||
}; | ||
LOG.info(obj); // CAP log exposure alert | ||
|
||
LOG.info(obj.x.replace(/./g, "*")); // NO CAP log exposure alert - replace as sanitizer | ||
|
||
var user = { | ||
password: encryptLib.encryptPassword(password) | ||
}; | ||
LOG.info(user); // NO CAP log exposure alert - encrypted data is fine | ||
jeongsoolee09 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
1 change: 1 addition & 0 deletions
1
...sitive-exposure/sensitive-exposure-js-all-sinks/sensitive-exposure-heuristic-source.qlref
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
sensitive-exposure/SensitiveExposureHeuristicSource.ql |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? What kind of code does this intend to capture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its from the out of the box queries, and is very necessary, it means that the definition of a variable being "read" includes any access of its properties (I think at sink locations only, as opposed to overall field sensitivity as an additional flow step would capture)
I dont really see any other form of documentation anywhere talking about this common predicate/to add to the comment but for our own understanding, if you want, searching for "allowImplicitRead" in slack across all channels does find some refs to it and a bit of useful conversation confirming that this is the intended use of that