Merge pull request #3451 from erik-krogh/fstreamWrite

Approved by esbena

change-notes/1.25/analysis-javascript.md

@@ -3,6 +3,7 @@
 ## General improvements
 
 * Support for the following frameworks and libraries has been improved:
+  - [fstream](https://www.npmjs.com/package/fstream)
   - [jGrowl](https://github.com/stanlemon/jGrowl)
   - [jQuery](https://jquery.com/)
 

javascript/ql/src/semmle/javascript/frameworks/Files.qll

@@ -151,10 +151,14 @@ private module FStream {
   /**
    * Gets a reference to a method in the `fstream` library.
    */
-  private DataFlow::SourceNode getAnFStreamProperty() {
+  private DataFlow::SourceNode getAnFStreamProperty(boolean writer) {
     exists(DataFlow::SourceNode mod, string readOrWrite, string subMod |
       mod = DataFlow::moduleImport("fstream") and
-      (readOrWrite = "Reader" or readOrWrite = "Writer") 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
@@ -167,7 +171,9 @@ private module FStream {
    * An invocation of a method defined in the `fstream` library.
    */
   private class FStream extends FileSystemAccess, DataFlow::InvokeNode {
-    FStream() { this = getAnFStreamProperty().getAnInvocation() }
+    boolean writer;
+
+    FStream() { this = getAnFStreamProperty(writer).getAnInvocation() }
 
     override DataFlow::Node getAPathArgument() {
       result = getOptionArgument(0, "path")
@@ -176,6 +182,24 @@ private module FStream {
       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() }
+  }
 }
 
 /**

javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected

@@ -16,6 +16,11 @@ nodes
 | ZipSlipBad.js:7:22:7:31 | entry.path |
 | ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:8:37:8:44 | fileName |
+| ZipSlipBad.js:15:11:15:31 | fileName |
+| ZipSlipBad.js:15:22:15:31 | entry.path |
+| ZipSlipBad.js:15:22:15:31 | entry.path |
+| ZipSlipBad.js:16:30:16:37 | fileName |
+| ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
@@ -33,6 +38,10 @@ edges
 | ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
 | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
 | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
+| ZipSlipBad.js:15:11:15:31 | fileName | ZipSlipBad.js:16:30:16:37 | fileName |
+| ZipSlipBad.js:15:11:15:31 | fileName | ZipSlipBad.js:16:30:16:37 | fileName |
+| ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
+| ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
@@ -42,4 +51,5 @@ edges
 | TarSlipBad.js:6:36:6:46 | header.name | TarSlipBad.js:6:36:6:46 | header.name | TarSlipBad.js:6:36:6:46 | header.name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | TarSlipBad.js:6:36:6:46 | header.name | item path |
 | ZipSlipBad2.js:6:22:6:29 | fileName | ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:6:22:6:29 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad2.js:5:37:5:46 | entry.path | item path |
 | ZipSlipBad.js:8:37:8:44 | fileName | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:7:22:7:31 | entry.path | item path |
+| ZipSlipBad.js:16:30:16:37 | fileName | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:16:30:16:37 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:15:22:15:31 | entry.path | item path |
 | ZipSlipBadUnzipper.js:8:37:8:44 | fileName | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | item path |

javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js

@@ -7,3 +7,11 @@ fs.createReadStream('archive.zip')
     const fileName = entry.path;
     entry.pipe(fs.createWriteStream(fileName));
   });
+
+var Writer = require('fstream').Writer;
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = entry.path;
+    entry.pipe(Writer({path: fileName}));
+  });