diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll index e1519892caf..c79639a1d82 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll @@ -861,28 +861,6 @@ module Express { override string getCredentialsKind() { result = kind } } - /** A call to `response.sendFile`, considered as a file system access. */ - private class ResponseSendFileAsFileSystemAccess extends FileSystemReadAccess, - DataFlow::MethodCallNode { - ResponseSendFileAsFileSystemAccess() { - exists(string name | name = "sendFile" or name = "sendfile" | - calls(any(ResponseExpr res).flow(), name) - ) - } - - override DataFlow::Node getADataNode() { none() } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getRootPathArgument() { - result = this.(DataFlow::CallNode).getOptionArgument(1, "root") - } - - override predicate isUpwardNavigationRejected(DataFlow::Node argument) { - argument = getAPathArgument() - } - } - /** * A function that flows to a route setup. */ diff --git a/javascript/ql/src/semmle/javascript/frameworks/Files.qll b/javascript/ql/src/semmle/javascript/frameworks/Files.qll index 3f8f4ee7419..1bcc4549931 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Files.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Files.qll @@ -4,23 +4,6 @@ import javascript -/** - * A call that can produce a file name. - */ -abstract private class FileNameProducer extends DataFlow::Node { - /** - * Gets a file name produced by this producer. - */ - abstract DataFlow::Node getAFileName(); -} - -/** - * A node that contains a file name, and is produced by a `ProducesFileNames`. - */ -private class ProducedFileName extends FileNameSource { - ProducedFileName() { this = any(FileNameProducer producer).getAFileName() } -} - /** * A file name from the `walk-sync` library. */ @@ -144,311 +127,3 @@ private class FastGlobFileNameSource extends FileNameSource { FastGlobFileNameSource() { this = fastGlobFileNameSource(DataFlow::TypeTracker::end()) } } -/** - * Classes and predicates for modelling the `fstream` library (https://www.npmjs.com/package/fstream). - */ -private module FStream { - /** - * Gets a reference to a method in the `fstream` library. - */ - private DataFlow::SourceNode getAnFStreamProperty(boolean writer) { - exists(DataFlow::SourceNode mod, string readOrWrite, string subMod | - mod = DataFlow::moduleImport("fstream") and - ( - readOrWrite = "Reader" and writer = false - or - readOrWrite = "Writer" and writer = true - ) and - (subMod = "File" or subMod = "Dir" or subMod = "Link" or subMod = "Proxy") - | - result = mod.getAPropertyRead(readOrWrite) or - result = mod.getAPropertyRead(readOrWrite).getAPropertyRead(subMod) or - result = mod.getAPropertyRead(subMod).getAPropertyRead(readOrWrite) - ) - } - - /** - * An invocation of a method defined in the `fstream` library. - */ - private class FStream extends FileSystemAccess, DataFlow::InvokeNode { - boolean writer; - - FStream() { this = getAnFStreamProperty(writer).getAnInvocation() } - - override DataFlow::Node getAPathArgument() { - result = getOptionArgument(0, "path") - or - not exists(getOptionArgument(0, "path")) and - result = getArgument(0) - } - } - - /** - * An invocation of an `fstream` method that writes to a file. - */ - private class FStreamWriter extends FileSystemWriteAccess, FStream { - FStreamWriter() { writer = true } - - override DataFlow::Node getADataNode() { none() } - } - - /** - * An invocation of an `fstream` method that reads a file. - */ - private class FStreamReader extends FileSystemReadAccess, FStream { - FStreamReader() { writer = false } - - override DataFlow::Node getADataNode() { none() } - } -} - -/** - * A call to the library `write-file-atomic`. - */ -private class WriteFileAtomic extends FileSystemWriteAccess, DataFlow::CallNode { - WriteFileAtomic() { - this = DataFlow::moduleImport("write-file-atomic").getACall() - or - this = DataFlow::moduleMember("write-file-atomic", "sync").getACall() - } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getADataNode() { result = getArgument(1) } -} - -/** - * A call to the library `recursive-readdir`. - */ -private class RecursiveReadDir extends FileSystemAccess, FileNameProducer, DataFlow::CallNode { - RecursiveReadDir() { this = DataFlow::moduleImport("recursive-readdir").getACall() } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getAFileName() { result = trackFileSource(DataFlow::TypeTracker::end()) } - - private DataFlow::SourceNode trackFileSource(DataFlow::TypeTracker t) { - t.start() and result = getCallback([1 .. 2]).getParameter(1) - or - t.startInPromise() and not exists(getCallback([1 .. 2])) and result = this - or - // Tracking out of a promise - exists(DataFlow::TypeTracker t2 | - result = PromiseTypeTracking::promiseStep(trackFileSource(t2), t, t2) - ) - } -} - -/** - * Classes and predicates for modelling the `jsonfile` library (https://www.npmjs.com/package/jsonfile). - */ -private module JSONFile { - /** - * A reader for JSON files. - */ - class JSONFileReader extends FileSystemReadAccess, DataFlow::CallNode { - JSONFileReader() { - this = - DataFlow::moduleMember("jsonfile", any(string s | s = "readFile" or s = "readFileSync")) - .getACall() - } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getADataNode() { result = trackRead(DataFlow::TypeTracker::end()) } - - private DataFlow::SourceNode trackRead(DataFlow::TypeTracker t) { - this.getCalleeName() = "readFile" and - ( - t.start() and result = getCallback([1 .. 2]).getParameter(1) - or - t.startInPromise() and not exists(getCallback([1 .. 2])) and result = this - ) - or - t.start() and - this.getCalleeName() = "readFileSync" and - result = this - or - // Tracking out of a promise - exists(DataFlow::TypeTracker t2 | - result = PromiseTypeTracking::promiseStep(trackRead(t2), t, t2) - ) - } - } - - /** - * A writer for JSON files. - */ - class JSONFileWriter extends FileSystemWriteAccess, DataFlow::CallNode { - JSONFileWriter() { - this = - DataFlow::moduleMember("jsonfile", any(string s | s = "writeFile" or s = "writeFileSync")) - .getACall() - } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getADataNode() { result = getArgument(1) } - } -} - -/** - * A call to the library `load-json-file`. - */ -private class LoadJsonFile extends FileSystemReadAccess, DataFlow::CallNode { - LoadJsonFile() { - this = DataFlow::moduleImport("load-json-file").getACall() - or - this = DataFlow::moduleMember("load-json-file", "sync").getACall() - } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getADataNode() { result = trackRead(DataFlow::TypeTracker::end()) } - - private DataFlow::SourceNode trackRead(DataFlow::TypeTracker t) { - this.getCalleeName() = "sync" and t.start() and result = this - or - not this.getCalleeName() = "sync" and t.startInPromise() and result = this - or - // Tracking out of a promise - exists(DataFlow::TypeTracker t2 | - result = PromiseTypeTracking::promiseStep(trackRead(t2), t, t2) - ) - } -} - -/** - * A call to the library `write-json-file`. - */ -private class WriteJsonFile extends FileSystemWriteAccess, DataFlow::CallNode { - WriteJsonFile() { - this = DataFlow::moduleImport("write-json-file").getACall() - or - this = DataFlow::moduleMember("write-json-file", "sync").getACall() - } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getADataNode() { result = getArgument(1) } -} - -/** - * A call to the library `walkdir`. - */ -private class WalkDir extends FileNameProducer, FileSystemAccess, DataFlow::CallNode { - WalkDir() { - this = DataFlow::moduleImport("walkdir").getACall() - or - this = DataFlow::moduleMember("walkdir", "sync").getACall() - or - this = DataFlow::moduleMember("walkdir", "async").getACall() - } - - override DataFlow::Node getAPathArgument() { result = getArgument(0) } - - override DataFlow::Node getAFileName() { result = trackFileSource(DataFlow::TypeTracker::end()) } - - private DataFlow::SourceNode trackFileSource(DataFlow::TypeTracker t) { - not this.getCalleeName() = any(string s | s = "sync" or s = "async") and - t.start() and - ( - result = getCallback(getNumArgument() - 1).getParameter(0) - or - result = getAMethodCall(EventEmitter::on()).getCallback(1).getParameter(0) - ) - or - t.start() and this.getCalleeName() = "sync" and result = this - or - t.startInPromise() and this.getCalleeName() = "async" and result = this - or - // Tracking out of a promise - exists(DataFlow::TypeTracker t2 | - result = PromiseTypeTracking::promiseStep(trackFileSource(t2), t, t2) - ) - } -} - -/** - * A call to the library `globule`. - */ -private class Globule extends FileNameProducer, FileSystemAccess, DataFlow::CallNode { - Globule() { - this = DataFlow::moduleMember("globule", "find").getACall() - or - this = DataFlow::moduleMember("globule", "match").getACall() - or - this = DataFlow::moduleMember("globule", "isMatch").getACall() - or - this = DataFlow::moduleMember("globule", "mapping").getACall() - or - this = DataFlow::moduleMember("globule", "findMapping").getACall() - } - - override DataFlow::Node getAPathArgument() { - (this.getCalleeName() = "match" or this.getCalleeName() = "isMatch") and - result = getArgument(1) - or - this.getCalleeName() = "mapping" and - ( - result = getAnArgument() and not exists(result.getALocalSource().getAPropertyWrite("src")) - or - result = getAnArgument().getALocalSource().getAPropertyWrite("src").getRhs() - ) - } - - override DataFlow::Node getAFileName() { - result = this and - ( - this.getCalleeName() = "find" or - this.getCalleeName() = "match" or - this.getCalleeName() = "findMapping" or - this.getCalleeName() = "mapping" - ) - } -} - -/** - * A file system access made by a NodeJS library. - * This class models multiple NodeJS libraries that access files. - */ -private class LibraryAccess extends FileSystemAccess, DataFlow::InvokeNode { - int pathArgument; // The index of the path argument. - - LibraryAccess() { - pathArgument = 0 and - ( - this = DataFlow::moduleImport("path-exists").getACall() - or - this = DataFlow::moduleImport("rimraf").getACall() - or - this = DataFlow::moduleImport("readdirp").getACall() - or - this = DataFlow::moduleImport("walker").getACall() - or - this = - DataFlow::moduleMember("node-dir", - any(string s | - s = "readFiles" or - s = "readFilesStream" or - s = "files" or - s = "promiseFiles" or - s = "subdirs" or - s = "paths" - )).getACall() - ) - or - pathArgument = 0 and - this = - DataFlow::moduleMember("vinyl-fs", any(string s | s = "src" or s = "dest" or s = "symlink")) - .getACall() - or - pathArgument = [0 .. 1] and - ( - this = DataFlow::moduleImport("ncp").getACall() or - this = DataFlow::moduleMember("ncp", "ncp").getACall() - ) - } - - override DataFlow::Node getAPathArgument() { result = getArgument(pathArgument) } -} diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll index a472b33aa8a..e7e1fce4302 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll @@ -450,33 +450,8 @@ module NodeJSLib { * A member `member` from module `fs` or its drop-in replacements `graceful-fs`, `fs-extra`, `original-fs`. */ DataFlow::SourceNode moduleMember(string member) { - result = fsModule(DataFlow::TypeTracker::end()).getAPropertyRead(member) - } - - private DataFlow::SourceNode fsModule(DataFlow::TypeTracker t) { - exists(string moduleName | - moduleName = ["mz/fs", "original-fs", "fs-extra", "graceful-fs", "fs"] - | - result = DataFlow::moduleImport(moduleName) - or - // extra support for flexible names - result.asExpr().(Require).getArgument(0).mayHaveStringValue(moduleName) - ) and - t.start() - or - exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | pred = fsModule(t2) | - result = pred.track(t2, t) - or - t.continue() = t2 and - exists(DataFlow::CallNode promisifyAllCall | - result = promisifyAllCall and - pred.flowsTo(promisifyAllCall.getArgument(0)) and - promisifyAllCall = - [ - DataFlow::moduleMember("bluebird", "promisifyAll"), - DataFlow::moduleImport("util-promisifyall") - ].getACall() - ) + exists(string moduleName | moduleName = ["fs-extra", "graceful-fs", "fs"] | + result = DataFlow::moduleMember(moduleName, member) ) } } @@ -487,7 +462,7 @@ module NodeJSLib { private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::CallNode { string methodName; - NodeJSFileSystemAccess() { this = maybePromisified(FS::moduleMember(methodName)).getACall() } + NodeJSFileSystemAccess() { this = FS::moduleMember(methodName).getACall() } /** * Gets the name of the called method. diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll index 095a2e072c1..7e59a199d99 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll @@ -55,7 +55,7 @@ module TaintedPath { * There are currently four flow labels, representing the different combinations of * normalization and absoluteness. */ - abstract class PosixPath extends DataFlow::FlowLabel { + class PosixPath extends DataFlow::FlowLabel { Normalization normalization; Relativeness relativeness; @@ -113,7 +113,7 @@ module TaintedPath { /** * A flow label representing an array of path elements that may include "..". */ - abstract class SplitPath extends DataFlow::FlowLabel { + class SplitPath extends DataFlow::FlowLabel { SplitPath() { this = "splitPath" } } } @@ -218,12 +218,12 @@ module TaintedPath { output = this or // non-global replace or replace of something other than /\.\./g, /[/]/g, or /[\.]/g. - this instanceof StringReplaceCall and + this.getCalleeName() = "replace" and input = getReceiver() and output = this and not exists(RegExpLiteral literal, RegExpTerm term | - this.(StringReplaceCall).getRegExp().asExpr() = literal and - this.(StringReplaceCall).isGlobal() and + getArgument(0).getALocalSource().asExpr() = literal and + literal.isGlobal() and literal.getRoot() = term | term.getAMatchedString() = "/" or @@ -247,15 +247,16 @@ module TaintedPath { /** * A call that removes all instances of "../" in the prefix of the string. */ - class DotDotSlashPrefixRemovingReplace extends StringReplaceCall { + class DotDotSlashPrefixRemovingReplace extends DataFlow::CallNode { DataFlow::Node input; DataFlow::Node output; DotDotSlashPrefixRemovingReplace() { + this.getCalleeName() = "replace" and input = getReceiver() and output = this and exists(RegExpLiteral literal, RegExpTerm term | - getRegExp().asExpr() = literal and + getArgument(0).getALocalSource().asExpr() = literal and (term instanceof RegExpStar or term instanceof RegExpPlus) and term.getChild(0) = getADotDotSlashMatcher() | @@ -297,16 +298,17 @@ module TaintedPath { /** * A call that removes all "." or ".." from a path, without also removing all forward slashes. */ - class DotRemovingReplaceCall extends StringReplaceCall { + class DotRemovingReplaceCall extends DataFlow::CallNode { DataFlow::Node input; DataFlow::Node output; DotRemovingReplaceCall() { + this.getCalleeName() = "replace" and input = getReceiver() and output = this and - isGlobal() and exists(RegExpLiteral literal, RegExpTerm term | - getRegExp().asExpr() = literal and + getArgument(0).getALocalSource().asExpr() = literal and + literal.isGlobal() and literal.getRoot() = term and not term.getAMatchedString() = "/" | @@ -624,13 +626,6 @@ module TaintedPath { AngularJSTemplateUrlSink() { this = any(AngularJS::CustomDirective d).getMember("templateUrl") } } - /** - * The path argument of a [send](https://www.npmjs.com/package/send) call, viewed as a sink. - */ - class SendPathSink extends Sink, DataFlow::ValueNode { - SendPathSink() { this = DataFlow::moduleImport("send").getACall().getArgument(1) } - } - /** * Holds if there is a step `src -> dst` mapping `srclabel` to `dstlabel` relevant for path traversal vulnerabilities. */