Merge pull request OWASP#107 from lirantal/feat/session-fixation

feat(session): improper session management
francoatmega · Feb 12, 2018 · fa5b1f9 · fa5b1f9
2 parents b4b8355 + f3b22ae
commit fa5b1f9
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 10 deletions.
diff --git a/app/routes/session.js b/app/routes/session.js
@@ -81,17 +81,25 @@ function SessionHandler(db) {
                     return next(err);
                 }
             }
-            // Regenerating in each login
-            // TODO: Add another vulnerability related with not to do it
-            req.session.regenerate(function() {
-                req.session.userId = user._id;
 
-                if (user.isAdmin) {
-                    return res.redirect("/benefits");
-                } else {
-                    return res.redirect("/dashboard");
-                }
-            });
+            // A2-Broken Authentication and Session Management
+            // Upon login, a security best practice with regards to cookies session management
+            // would be to regenerate the session id so that if an id was already created for
+            // a user on an insecure medium (i.e: non-HTTPS website or otherwise), or if an
+            // attacker was able to get their hands on the cookie id before the user logged-in,
+            // then the old session id will render useless as the logged-in user with new privileges
+            // holds a new session id now.
+
+            // Fix the problem by regenerating a session in each login
+            // by wrapping the below code as a function callback for the method req.session.regenerate()
+            // i.e:
+            // `req.session.regenerate(function() {})`
+            req.session.userId = user._id;
+            if (user.isAdmin) {
+              return res.redirect("/benefits");
+            } else {
+              return res.redirect("/dashboard");
+            }
         });
     };
 

diff --git a/app/views/tutorial/a2.html b/app/views/tutorial/a2.html
@@ -166,6 +166,35 @@ <h3>2. Session timeout and protecting cookies in transit</h3>
 });
                         </pre> Note: The example code uses
                             <code>MemoryStore</code>to manage session data, which is not designed for production environment, as it will leak memory, and will not scale past a single process. Use database based storage MongoStore or RedisStore for production. Alternatively, sessions can be managed using popular passport module.
+                        <br/>
+                        <br/>
+                        <h3>3. Session hijacking</h3>
+
+                        <p>The insecure demo application does not regenerate a new session id upon user's login, therefore rendering a vulnerability of session hijacking if an attacker is able to somehow steal the cookie with the session id and use it.
+
+                        <p>Upon login, a security best practice with regards to cookies session management would be to regenerate the session id so that if an id was already created for a user on an insecure medium (i.e: non-HTTPS website or otherwise), or if an attacker was able to get their hands on the cookie id before the user logged-in, then the old session id will render useless as the logged-in user with new privileges holds a new session id now.
+                    </p>
+
+                        <p>To secure the application:</p>
+                        <p>1. Re-generate a new session id upon login (and best practice is to keep regenerating them
+upon requests or at least upon sensitive actions like a user's password reset.
+
+                            Re-generate a session id as follows:
+                            By wrapping the below code as a function callback for the method req.session.regenerate()
+                            <pre>
+req.session.regenerate(function() {
+
+  req.session.userId = user._id;
+
+  if (user.isAdmin) {
+    return res.redirect("/benefits");
+  } else {
+    return res.redirect("/dashboard");
+  }
+
+})
+                        </pre>
+                        </p>
                     </div>
                 </div>
                 <div class="panel panel-default">