From d0a16d23cbcbf4b835467fbc6eff0be50948971d Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Fri, 11 Sep 2020 15:07:33 -0400
Subject: [PATCH] security: Fix authentication bypass vulnerability

Before, anyone who could create a socket.io connection to Etherpad
could read, modify, and create pads at will without authenticating
first.

The `checkAccess` middleware in `webaccess.js` normally handles
authentication and authorization, but it does not run for `/socket.io`
requests. This means that the connection handler in `socketio.js` must
handle authentication and authorization. However, before this change:
  * The handler did not require a signed `express_sid` cookie.
  * After loading the express-session state, the handler did not check
    to see if the user had authenticated.

Now the handler requires a signed `express_sid` cookie, and it ensures
that `socket.request.session.user` is non-null if authentication is
required. (`socket.request.session.user` is non-null if and only if
the user has authenticated.)
---
 src/node/hooks/express/socketio.js | 57 +++++++++++++-----------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js
index 03fa7bbe6..3eef7a92b 100644
--- a/src/node/hooks/express/socketio.js
+++ b/src/node/hooks/express/socketio.js
@@ -39,43 +39,36 @@ exports.expressCreateServer = function (hook_name, args, cb) {
     cookie: false,
   });
 
-  /* Require an express session cookie to be present, and load the
-   * session. See http://www.danielbaulig.de/socket-ioexpress for more
-   * info */
+  // REQUIRE a signed express-session cookie to be present, then load the session. See
+  // http://www.danielbaulig.de/socket-ioexpress for more info. After the session is loaded, ensure
+  // that the user has authenticated (if authentication is required).
+  //
+  // !!!WARNING!!! Requests to /socket.io are NOT subject to the checkAccess middleware in
+  // webaccess.js. If this handler fails to check for a signed express-session cookie or fails to
+  // check whether the user has authenticated, then any random person on the Internet can read,
+  // modify, or create any pad (unless the pad is password protected or an HTTP API session is
+  // required).
   var cookieParserFn = cookieParser(webaccess.secret, {});
-
   io.use(function(socket, accept) {
     var data = socket.request;
-    // Use a setting if we want to allow load Testing
-
-    // Sometimes browsers might not have cookies at all, for example Safari in iFrames Cross domain
-    // https://github.com/ether/etherpad-lite/issues/4031
-    // if requireSession is false we can allow them to still get on the pad.
-    // Note that this does make security less tight because any socketIO connection can be established without
-    // any logic on the client to do any handshaking..  I am not concerned about this though, the real solution
-    // here is to implement rateLimiting on SocketIO ACCEPT_COMMIT messages.
-
-    if(!data.headers.cookie && (settings.loadTest || !settings.requireSession)){
-      accept(null, true);
-    }else{
-      if (!data.headers.cookie) return accept('No session cookie transmitted.', false);
-    }
-    if(data.headers.cookie){
-      cookieParserFn(data, {}, function(err){
-        if(err) {
-          console.error(err);
-          accept("Couldn't parse request cookies. ", false);
-          return;
+    if (!data.headers.cookie && settings.loadTest) return accept(null, true);
+    cookieParserFn(data, {}, function(err) {
+      if (err) {
+        console.error(err);
+        accept("Couldn't parse request cookies.", false);
+        return;
+      }
+      data.sessionID = data.signedCookies.express_sid;
+      if (!data.sessionID) return accept('Signed express_sid cookie is required', false);
+      args.app.sessionStore.get(data.sessionID, function(err, session) {
+        if (err || !session) return accept('Bad session / session has expired', false);
+        data.session = new sessionModule.Session(data, session);
+        if (settings.requireAuthentication && data.session.user == null) {
+          return accept('Authentication required', false);
         }
-
-        data.sessionID = data.signedCookies.express_sid;
-        args.app.sessionStore.get(data.sessionID, function (err, session) {
-          if (err || !session) return accept('Bad session / session has expired', false);
-          data.session = new sessionModule.Session(data, session);
-          accept(null, true);
-        });
+        accept(null, true);
       });
-    }
+    });
   });
 
   // var socketIOLogger = log4js.getLogger("socket.io");