JS: add query js/cleartext-logging#73
Conversation
ghost
added
the
ghost
self-requested a review
as a 
xiemaisi
left a comment
xiemaisi
left a comment
There was a problem hiding this comment.
Great stuff! Looks like you put a lot of work into fine-tuning the heuristics.
A few suggestions, but overall the results speak for themselves.
|
|
||
| | **Query** | **Tags** | **Purpose** | | ||
| |-----------------------------|-----------|--------------------------------------------------------------------| | ||
| | Clear text logging of sensitive information (`js/cleartext-logging`) | security, external/cwe/cwe-312, external/cwe/cwe-315, external/cwe/cwe-359 | Highlights logging of sensitive information, indicating a violation of [CWE-312](https://cwe.mitre.org/data/definitions/312.html). Results shown on lgtm by default. | |
There was a problem hiding this comment.
Here and below: "clear-text logging" should probably have a dash between "clear" and "text". Also, upper-case LGTM.
| /** | ||
| * Holds if `tl` is used in a browser environment. | ||
| */ | ||
| predicate inBrowserEnvironment(TopLevel tl) { |
There was a problem hiding this comment.
OOI, could this lead to false negatives for Electron apps?
There was a problem hiding this comment.
I do not think that is a false negative.
This predicate is used to exclude alerts for logging that occurs on the user's own computer since that is innocent in practice.
Both browsers and electron apps (that use browser features) are run on the user's own computer.
| /** | ||
| * Holds if `sink` only is reachable in a "test" environment. | ||
| */ | ||
| predicate inTestEnvironment(Sink sink) { |
There was a problem hiding this comment.
Couldn't we leave it to our file classification to hide results in test code?
There was a problem hiding this comment.
This does not prevent results in test code, it prevents results like this:
if (environment.isTestEnv()) {
console.log("Password is: " + password); // OK
}See the test:
https://github.com/Semmle/ql/pull/73/files#diff-48bc5394f61be303fa92728aec0a85a8R92
There was a problem hiding this comment.
I see. To be honest, though, this predicate doesn't make me very happy. It looks very ad-hoc and brittle. Could we come up with something slightly more rigorous?
There was a problem hiding this comment.
I have removed the check for now, it does not make a difference on our default benchmarks.
| } | ||
|
|
||
| private string getAStandardLoggerMethodName() { | ||
| // log level names used in RFC5424, `npm`, `console` |
There was a problem hiding this comment.
Turn this into qldoc, since we have it anyway?
| result = "info" or | ||
| result = "log" or | ||
| result = "notice" or | ||
| result = "silly" or |
There was a problem hiding this comment.
Yep.
https://docs.npmjs.com/misc/config#loglevel: "silent", "error", "warn", "notice", "http", "timing", "info", "verbose", "silly"
There was a problem hiding this comment.
Oh, good. My favourite in this list is actually "http", though. Well done for not including it in this predicate.
| @@ -0,0 +1,224 @@ | |||
| /** | |||
| * Provides a dataflow tracking configuration for reasoning about cleartext logging of sensitive information. | |||
| node instanceof Barrier | ||
| } | ||
|
|
||
| override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg) { |
There was a problem hiding this comment.
This duplicates a fair amount of code from the taint tracking library. Please refactor and reuse.
There was a problem hiding this comment.
Done. I made the concatenation part of StringManipulationTaintStep public.
Performance of js/cleartext-logging is unchanged, but I will start a sanity check for the ordinary taint queries now.
| /** | ||
| * An object with a property that may contain password information | ||
| * | ||
| * This is a source since `toString()` on this object will show the property value. |
There was a problem hiding this comment.
Only if they have defined a custom toString, otherwise it'll show [object Object].
| ( | ||
| this.asExpr().(VarAccess).getName() = name | ||
| or | ||
| exists (DataFlow::PropRead read, DataFlow::Node base | |
There was a problem hiding this comment.
exists (DataFlow::SourceNode base |
this = base.getAPropertyRead(name) and
// avoid (...)
not base.getAPropertyWrite(name).getRhs() instanceof NonCleartextPassword
)
|
|
||
| ObjectPasswordPropertySource() { | ||
| exists (DataFlow::PropWrite write | | ||
| write.getPropertyName() = name and |
There was a problem hiding this comment.
write = this.(DataFlow::SourceNode).getAWrite(name) and
|
@felicity-semmle, who should we request a doc review from? (I can @-mention @Semmle/doc, but can't request a review.) |
|
mc-semmle ;-) |
|
Thanks; unfortunately I cannot request a review from you either (maybe you need to get yourself added to the Semmle organisation or something?). |
|
I believe Pavel added me to the Semmle organization and to the docteam. How strange. |
| @@ -0,0 +1,55 @@ | |||
| /** | |||
| * @name Clear text logging of sensitive information | |||
There was a problem hiding this comment.
Clear-text logging (with an hyphen)?
| @@ -0,0 +1,5 @@ | |||
| <!DOCTYPE qhelp PUBLIC | |||
|
All comments addressed. |
| @@ -0,0 +1,38 @@ | |||
| /** | |||
| * @name Clear-text logging of sensitive information | |||
| * @description Sensitive information logged without encryption or hashing can expose it to an | |||
There was a problem hiding this comment.
This description sounds weird to me. Who or what can expose it to an attacker?
Perhaps rephrase as "Logging sensitive information without encryption or hashing (...)".
| result = "info" or | ||
| result = "log" or | ||
| result = "notice" or | ||
| result = "silly" or |
There was a problem hiding this comment.
Oh, good. My favourite in this list is actually "http", though. Well done for not including it in this predicate.
|
Removed last nit with an amend. |
Update tree-sitter-ruby to pick up improvements to calls
Kotlin: Merge in main
Add "implicit `this`" query
…-query Add "implicit `this`" query
fix(controlcheck): Improve checks for actors
3 participants
This PR adds a taint analysis query that flags logging of sensitive data in clear text. Logged sensitive data may be stored, so this query is really just a sibling of
js/clear-text-storage-of-sensitive-data, they therefore share aqhelpfile.Programmers construct log messages in very different ways, so this query is much more conservative that its sibling. In particular, his query considers "passwords" to be the only source of sensitive data.
Evaluation on our standard benchmarks reveal two true positives, and nothing else.
The performance seems fine for a new security query, here are the numbers for a comparison on the security suite: https://git.semmle.com/gist/esben/3e07f35b8aab1caff35eeab990615d19