-
Notifications
You must be signed in to change notification settings - Fork 1.9k
-
I've benn written a codeql for checking whether a user controlled flow to fs.createWriteStream.
class MyDataFlowConfiguration extends TaintTracking::Configuration {
MyDataFlowConfiguration() { this = "MyDataFlowConfiguration" }
override predicate isSource(DataFlow::Node source) { Express::isRequest(source) }
override predicate isSink(DataFlow::Node sink) {
exists(DataFlow::CallNode call |
call = DataFlow::moduleMember("fs", "createWriteStream").getACall() and
call.getArgument(0) = sink
)
}
}
However , user controlled data has been sanitized by IndexOf, and any doucuments(use google) didn't elaborate this case. Could you please help me to write a Sanitizer to eliminate false positives. I've no ideas about sanitizer
if (req.body.cert_file_name.indexOf('../') >= 0 || req.body.key_file_name.indexOf('../') >= 0) {
return res.send(resResult.error(i18n.__('CertConfig.strFileError')));
}
writeStream = fs.createWriteStream(`D:\\test\\${req.body.cert_file_name}`, 'utf-8');
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 1 comment 2 replies
-
There actually already is a sanitizer for contains("../") -- take a look at ContainsDotDotSanitizer and how it gets used (indirectly) in isBarrierGuard. However note that this query is using a LabeledBarrierGuardNode to track that there are two ways to access paths outside the intended directory: using ../ and similar for traversal, or supplying an absolute path. You'll need to make an is-absolute check against the same cert_file_name property to fix the alert.
Beta Was this translation helpful? Give feedback.
All reactions
-
could you please give me some detail about js FlowLabel?
Beta Was this translation helpful? Give feedback.
All reactions
-
They're arbitrary strings used to represent details about flow. In the tainted path query the relevant strings are defined at https://github.com/github/codeql/blob/main/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll#L58 and represent whether a path has been normalised, checked for absolute-ness, neither or both.
In the dataflow query definition at https://github.com/github/codeql/blob/main/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathQuery.qll#L25 we can see that flow-label variants of the is-source, is-sink, is-barrier and is-additional-flow-step predicates re used. The additional flow steps in particular can be used to switch one label out for another -- for example, to note that adding something to the end of a relative path can't make it absolute, but it can make it non-normalised.
Beta Was this translation helpful? Give feedback.