Security: FEATURE REMOVAL: Remove all plain text password logic and ui (#4178)

This will be a breaking change for some people.  

We removed all internal password control logic.  If this affects you, you have two options:

1. Use a plugin for authentication and use session based pad access (recommended).
1. Use a plugin for password setting.

The reasoning for removing this feature is to reduce the overall security footprint of Etherpad.  It is unnecessary and cumbersome to keep this feature and with the thousands of available authentication methods available in the world our focus should be on supporting those and allowing more granual access based on their implementations (instead of half assed baking our own).

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Develop -- TODO Change to 1.8.x.
 ### Compatibility-breaking changes
+* **IMPORTANT:** It is no longer possible to protect a group pad with a
+  password. All API calls to `setPassword` or `isPasswordProtected` will fail.
+  Existing group pads that were previously password protected will no longer be
+  password protected. If you need fine-grained access control, you can restrict
+  API session creation in your frontend service, or you can use plugins.
 * Authorization failures now return 403 by default instead of 401
 * The `authorize` hook is now only called after successful
   authentication. Use the new `preAuthorize` hook if you need to bypass

doc/api/hooks_server-side.md

@@ -139,9 +139,8 @@ Called from: src/node/db/SecurityManager.js
 Things in context:
 
 1. padID - the pad the user wants to access
-2. password - the password the user has given to access the pad
-3. token - the token of the author
-4. sessionCookie - the session the use has
+2. token - the token of the author
+3. sessionCookie - the session the use has
 
 This hook gets called when the access to the concrete pad is being checked. Return `false` to deny access.
 

doc/api/http_api.md

@@ -581,24 +581,6 @@ return true of false
   * `{code: 0, message:"ok", data: {publicStatus: true}}`
   * `{code: 1, message:"padID does not exist", data: null}`
 
-#### setPassword(padID, password)
- * API >= 1
-
-returns ok or an error message
-
-*Example returns:*
-  * `{code: 0, message:"ok", data: null}`
-  * `{code: 1, message:"padID does not exist", data: null}`
-
-#### isPasswordProtected(padID)
- * API >= 1
-
-returns true or false
-
-*Example returns:*
-  * `{code: 0, message:"ok", data: {passwordProtection: true}}`
-  * `{code: 1, message:"padID does not exist", data: null}`
-
 #### listAuthorsOfPad(padID)
  * API >= 1
 

doc/docker.md

@@ -171,7 +171,6 @@ For the editor container, you can also make it full width by adding `full-width-
 | `SUPPRESS_ERRORS_IN_PAD_TEXT`     | Should we suppress errors from being visible in the default Pad Text?                                                                                                                                  | `false`            |
 | `REQUIRE_SESSION`                 | If this option is enabled, a user must have a session to access pads. This effectively allows only group pads to be accessed.                                                                          | `false`            |
 | `EDIT_ONLY`                       | Users may edit pads but not create new ones. Pad creation is only via the API. This applies both to group pads and regular pads.                                                                       | `false`            |
-| `SESSION_NO_PASSWORD`             | If set to true, those users who have a valid session will automatically be granted access to password protected pads.                                                                                  | `false`            |
 | `MINIFY`                          | If true, all css & js will be minified before sending to the client. This will improve the loading performance massively, but makes it difficult to debug the javascript/css                           | `true`             |
 | `MAX_AGE`                         | How long may clients use served javascript code (in seconds)? Not setting this may cause problems during deployment. Set to 0 to disable caching.                                                      | `21600` (6 hours)  |
 | `ABIWORD`                         | Absolute path to the Abiword executable. Abiword is needed to get advanced import/export features of pads. Setting it to null disables Abiword and will only allow plain text and HTML import/exports. | `null`             |

settings.json.docker

@@ -260,12 +260,6 @@
    */
   "editOnly": "${EDIT_ONLY:false}",
 
-  /*
-   * If set to true, those users who have a valid session will automatically be
-   * granted access to password protected pads.
-   */
-  "sessionNoPassword": "${SESSION_NO_PASSWORD:false}",
-
   /*
    * If true, all css & js will be minified before sending to the client.
    *

settings.json.template

@@ -263,12 +263,6 @@
    */
   "editOnly": false,
 
-  /*
-   * If set to true, those users who have a valid session will automatically be
-   * granted access to password protected pads.
-   */
-  "sessionNoPassword": false,
-
   /*
    * If true, all css & js will be minified before sending to the client.
    *

src/locales/en.json

@@ -60,9 +60,7 @@
 
   "pad.loading": "Loading...",
   "pad.noCookie": "Cookie could not be found. Please allow cookies in your browser!  Your session and settings will not be saved between visits.  This may be due to Etherpad being included in an iFrame in some Browsers.  Please ensure Etherpad is on the same subdomain/domain as the parent iFrame",
-  "pad.passwordRequired": "You need a password to access this pad",
   "pad.permissionDenied": "You do not have permission to access this pad",
-  "pad.wrongPassword": "Your password was wrong",
 
   "pad.settings.padSettings": "Pad Settings",
   "pad.settings.myView": "My View",

src/node/db/API.js

@@ -684,7 +684,6 @@ exports.setPublicStatus = async function(padID, publicStatus)
     publicStatus = (publicStatus.toLowerCase() === "true");
   }
 
-  // set the password
   await pad.setPublicStatus(publicStatus);
 }
 
@@ -706,44 +705,6 @@ exports.getPublicStatus = async function(padID)
   return { publicStatus: pad.getPublicStatus() };
 }
 
-/**
-setPassword(padID, password) returns ok or a error message
-
-Example returns:
-
-{code: 0, message:"ok", data: null}
-{code: 1, message:"padID does not exist", data: null}
-*/
-exports.setPassword = async function(padID, password)
-{
-  // ensure this is a group pad
-  checkGroupPad(padID, "password");
-
-  // get the pad
-  let pad = await getPadSafe(padID, true);
-
-  // set the password
-  await pad.setPassword(password === '' ? null : password);
-}
-
-/**
-isPasswordProtected(padID) returns true or false
-
-Example returns:
-
-{code: 0, message:"ok", data: {passwordProtection: true}}
-{code: 1, message:"padID does not exist", data: null}
-*/
-exports.isPasswordProtected = async function(padID)
-{
-  // ensure this is a group pad
-  checkGroupPad(padID, "password");
-
-  // get the pad
-  let pad = await getPadSafe(padID, true);
-  return { isPasswordProtected: pad.isPasswordProtected() };
-}
-
 /**
 listAuthorsOfPad(padID) returns an array of authors who contributed to this pad
 

src/node/db/Pad.js

@@ -37,7 +37,6 @@ let Pad = function Pad(id) {
   this.head = -1;
   this.chatHead = -1;
   this.publicStatus = false;
-  this.passwordHash = null;
   this.id = id;
   this.savedRevisions = [];
 };
@@ -595,19 +594,6 @@ Pad.prototype.setPublicStatus = async function setPublicStatus(publicStatus) {
   await this.saveToDatabase();
 };
 
-Pad.prototype.setPassword = async function setPassword(password) {
-  this.passwordHash = password == null ? null : hash(password, generateSalt());
-  await this.saveToDatabase();
-};
-
-Pad.prototype.isCorrectPassword = function isCorrectPassword(password) {
-  return compare(this.passwordHash, password);
-};
-
-Pad.prototype.isPasswordProtected = function isPasswordProtected() {
-  return this.passwordHash != null;
-};
-
 Pad.prototype.addSavedRevision = async function addSavedRevision(revNum, savedById, label) {
   // if this revision is already saved, return silently
   for (var i in this.savedRevisions) {
@@ -633,19 +619,3 @@ Pad.prototype.getSavedRevisions = function getSavedRevisions() {
   return this.savedRevisions;
 };
 
-/* Crypto helper methods */
-
-function hash(password, salt) {
-  var shasum = crypto.createHash('sha512');
-  shasum.update(password + salt);
-
-  return shasum.digest("hex") + "$" + salt;
-}
-
-function generateSalt() {
-  return randomString(86);
-}
-
-function compare(hashStr, password) {
-  return hash(password, hashStr.split("$")[1]) === hashStr;
-}

src/node/db/SecurityManager.js

@@ -28,8 +28,6 @@ var log4js = require('log4js');
 var authLogger = log4js.getLogger("auth");
 
 const DENY = Object.freeze({accessStatus: 'deny'});
-const WRONG_PASSWORD = Object.freeze({accessStatus: 'wrongPassword'});
-const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'});
 
 /**
  * Determines whether the user can access a pad.
@@ -42,16 +40,14 @@ const NEED_PASSWORD = Object.freeze({accessStatus: 'needPassword'});
  *     is false and the user is accessing a public pad. If there is not an author already associated
  *     with this token then a new author object is created (including generating an author ID) and
  *     associated with this token.
- * @param password is the password the user has given to access this pad. It can be null.
  * @param userSettings is the settings.users[username] object (or equivalent from an authn plugin).
- * @return {accessStatus: grant|deny|wrongPassword|needPassword, authorID: a.xxxxxx}. The caller
- *     must use the author ID returned in this object when making any changes associated with the
- *     author.
+ * @return {accessStatus: grant|deny, authorID: a.xxxxxx}. The caller must use the author ID
+ *     returned in this object when making any changes associated with the author.
  *
  * WARNING: Tokens and session IDs MUST be kept secret, otherwise users will be able to impersonate
  * each other (which might allow them to gain privileges).
  */
-exports.checkAccess = async function(padID, sessionCookie, token, password, userSettings)
+exports.checkAccess = async function(padID, sessionCookie, token, userSettings)
 {
   if (!padID) {
     authLogger.debug('access denied: missing padID');
@@ -84,7 +80,7 @@ exports.checkAccess = async function(padID, sessionCookie, token, password, user
 
   // allow plugins to deny access
   const isFalse = (x) => x === false;
-  if (hooks.callAll('onAccessCheck', {padID, password, token, sessionCookie}).some(isFalse)) {
+  if (hooks.callAll('onAccessCheck', {padID, token, sessionCookie}).some(isFalse)) {
     authLogger.debug('access denied: an onAccessCheck hook function returned false');
     return DENY;
   }
@@ -112,8 +108,7 @@ exports.checkAccess = async function(padID, sessionCookie, token, password, user
   };
 
   if (!padID.includes('$')) {
-    // Only group pads can be private or have passwords, so there is nothing more to check for this
-    // non-group pad.
+    // Only group pads can be private, so there is nothing more to check for this non-group pad.
     return grant;
   }
 
@@ -122,7 +117,7 @@ exports.checkAccess = async function(padID, sessionCookie, token, password, user
       authLogger.debug('access denied: must have an HTTP API session to create a group pad');
       return DENY;
     }
-    // Creating a group pad, so there is no password or public status to check.
+    // Creating a group pad, so there is no public status to check.
     return grant;
   }
 
@@ -133,18 +128,5 @@ exports.checkAccess = async function(padID, sessionCookie, token, password, user
     return DENY;
   }
 
-  const passwordExempt = settings.sessionNoPassword && sessionAuthorID != null;
-  const requirePassword = pad.isPasswordProtected() && !passwordExempt;
-  if (requirePassword) {
-    if (password == null) {
-      authLogger.debug('access denied: password required');
-      return NEED_PASSWORD;
-    }
-    if (!password || !pad.isCorrectPassword(password)) {
-      authLogger.debug('access denied: wrong password');
-      return WRONG_PASSWORD;
-    }
-  }
-
   return grant;
 };

src/node/handler/APIHandler.js

@@ -70,8 +70,6 @@ version["1"] = Object.assign({},
   , "getReadOnlyID"             : ["padID"]
   , "setPublicStatus"           : ["padID", "publicStatus"]
   , "getPublicStatus"           : ["padID"]
-  , "setPassword"               : ["padID", "password"]
-  , "isPasswordProtected"       : ["padID"]
   , "listAuthorsOfPad"          : ["padID"]
   , "padUsersCount"             : ["padID"]
   }

src/node/handler/PadMessageHandler.js

@@ -219,7 +219,7 @@ exports.handleMessage = async function(client, message)
 
   const {session: {user} = {}} = client.client.request;
   const {accessStatus, authorID} =
-      await securityManager.checkAccess(padId, auth.sessionID, auth.token, auth.password, user);
+      await securityManager.checkAccess(padId, auth.sessionID, auth.token, user);
   if (accessStatus !== 'grant') {
     // Access denied. Send the reason to the user.
     client.json.send({accessStatus});
@@ -826,7 +826,7 @@ async function handleSwitchToPad(client, message, _authorID)
   const newPadIds = await readOnlyManager.getIds(message.padId);
   const {session: {user} = {}} = client.client.request;
   const {accessStatus, authorID} = await securityManager.checkAccess(
-      newPadIds.padId, message.sessionID, message.token, message.password, user);
+      newPadIds.padId, message.sessionID, message.token, user);
   if (accessStatus !== 'grant') {
     // Access denied. Send the reason to the user.
     client.json.send({accessStatus});
@@ -868,7 +868,6 @@ function createSessionInfoAuth(sessionInfo, message)
     sessionID: message.sessionID,
     padID: message.padId,
     token: message.token,
-    password: message.password,
   };
 }
 

src/node/handler/SocketIORouter.js

@@ -58,7 +58,7 @@ exports.setSocketIO = function(_socket) {
     // wrap the original send function to log the messages
     client._send = client.send;
     client.send = function(message) {
-      messageLogger.debug("to " + client.id + ": " + stringifyWithoutPassword(message));
+      messageLogger.debug(`to ${client.id}: ${JSON.stringify(message)}`);
       client._send(message);
     }
 
@@ -69,14 +69,14 @@ exports.setSocketIO = function(_socket) {
 
     client.on('message', async function(message) {
       if (message.protocolVersion && message.protocolVersion != 2) {
-        messageLogger.warn("Protocolversion header is not correct:" + stringifyWithoutPassword(message));
+        messageLogger.warn(`Protocolversion header is not correct: ${JSON.stringify(message)}`);
         return;
       }
       if (!message.component || !components[message.component]) {
-        messageLogger.error("Can't route the message:" + stringifyWithoutPassword(message));
+        messageLogger.error(`Can't route the message: ${JSON.stringify(message)}`);
         return;
       }
-      messageLogger.debug("from " + client.id + ": " + stringifyWithoutPassword(message));
+      messageLogger.debug(`from ${client.id}: ${JSON.stringify(message)}`);
       await components[message.component].handleMessage(client, message);
     });
 
@@ -88,16 +88,3 @@ exports.setSocketIO = function(_socket) {
     });
   });
 }
-
-// returns a stringified representation of a message, removes the password
-// this ensures there are no passwords in the log
-function stringifyWithoutPassword(message)
-{
-  let newMessage = Object.assign({}, message);
-
-  if (newMessage.password != null) {
-    newMessage.password = "xxx";
-  }
-
-  return JSON.stringify(newMessage);
-}

src/node/hooks/express/importexport.js

@@ -65,7 +65,7 @@ exports.expressCreateServer = function (hook_name, args, cb) {
   args.app.post('/p/:pad/import', async function(req, res, next) {
     const {session: {user} = {}} = req;
     const {accessStatus} = await securityManager.checkAccess(
-        req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password, user);
+        req.params.pad, req.cookies.sessionID, req.cookies.token, user);
     if (accessStatus !== 'grant' || !webaccess.userCanModify(req.params.pad, req)) {
       return res.status(403).send('Forbidden');
     }

src/node/hooks/express/openapi.js

@@ -201,15 +201,6 @@ const resources = {
       summary: 'return true of false',
       responseSchema: { publicStatus: { type: 'boolean' } },
     },
-    setPassword: {
-      operationId: 'setPassword',
-      summary: 'returns ok or a error message',
-    },
-    isPasswordProtected: {
-      operationId: 'isPasswordProtected',
-      summary: 'returns true or false',
-      responseSchema: { passwordProtection: { type: 'boolean' } },
-    },
     authors: {
       operationId: 'listAuthorsOfPad',
       summary: 'returns an array of authors who contributed to this pad',

src/node/padaccess.js

@@ -5,7 +5,7 @@ module.exports = async function (req, res) {
   try {
     const {session: {user} = {}} = req;
     const accessObj = await securityManager.checkAccess(
-        req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password, user);
+        req.params.pad, req.cookies.sessionID, req.cookies.token, user);
 
     if (accessObj.accessStatus === "grant") {
       // there is access, continue

src/node/utils/Settings.js

@@ -193,11 +193,6 @@ exports.requireSession = false;
  */
 exports.editOnly = false;
 
-/**
- * A flag that bypasses password prompts for users with valid sessions
- */
-exports.sessionNoPassword = false;
-
 /**
  * Max age that responses will have (affects caching layer).
  */

src/static/css/pad/loadingbox.css

@@ -10,10 +10,6 @@
   z-index: 100;
 }
 
-#editorloadingbox .passForm{
-  padding:10px;
-}
-
 #editorloadingbox input{
   padding:10px;
 }
@@ -22,6 +18,6 @@
   padding:10px;
 }
 
-#passwordRequired, #permissionDenied, #wrongPassword, #noCookie {
+#permissionDenied, #noCookie {
   display:none;
-}
+}

src/static/js/pad.js

@@ -126,15 +126,6 @@ function getUrlVars()
   return vars;
 }
 
-function savePassword()
-{
-  //set the password cookie
-  Cookies.set('password', $('#passwordinput').val(), {path: document.location.pathname});
-  //reload
-  document.location=document.location;
-  return false;
-}
-
 function sendClientReady(isReconnect, messageType)
 {
   messageType = typeof messageType !== 'undefined' ? messageType : 'CLIENT_READY';
@@ -160,7 +151,6 @@ function sendClientReady(isReconnect, messageType)
     type: messageType,
     padId: padId,
     sessionID: Cookies.get('sessionID'),
-    password: Cookies.get('password'),
     token: token,
     protocolVersion: 2
   };
@@ -225,10 +215,6 @@ function handshake()
     //the access was not granted, give the user a message
     if(obj.accessStatus)
     {
-      if(!receivedClientVars){
-        $('.passForm').submit(require(module.id).savePassword);
-      }
-
       if(obj.accessStatus == "deny")
       {
         $('#loading').hide();
@@ -241,19 +227,6 @@ function handshake()
           $("#editorloadingbox").show();
         }
       }
-      else if(obj.accessStatus == "needPassword")
-      {
-        $('#loading').hide();
-        $('#passwordRequired').show();
-        $("#passwordinput").focus();
-      }
-      else if(obj.accessStatus == "wrongPassword")
-      {
-        $('#loading').hide();
-        $('#wrongPassword').show();
-        $('#passwordRequired').show();
-        $("#passwordinput").focus();
-      }
     }
 
     //if we haven't recieved the clientVars yet, then this message should it be
@@ -954,7 +927,6 @@ exports.settings = settings;
 exports.randomString = randomString;
 exports.getParams = getParams;
 exports.getUrlVars = getUrlVars;
-exports.savePassword = savePassword;
 exports.handshake = handshake;
 exports.pad = pad;
 exports.init = init;

src/static/js/timeslider.js

@@ -113,7 +113,6 @@ function sendSocketMsg(type, data)
     padId,
     token,
     sessionID: Cookies.get('sessionID'),
-    password: Cookies.get('password'),
     protocolVersion: 2,
   });
 }

src/static/tests.html

@@ -161,7 +161,5 @@
   <div class="define">getReadOnlyID(padID)</div>
   <div class="define">setPublicStatus(padID,publicStatus)</div>
   <div class="define">getPublicStatus(padID)</div>
-  <div class="define">setPassword(padID,password)</div>
-  <div class="define">isPasswordProtected(padID)</div>
 </body>
 </html>

src/templates/pad.html

@@ -91,18 +91,9 @@
       <div id="editorcontainer" class="editorcontainer"></div>
 
       <div id="editorloadingbox">
-        <div id="passwordRequired">
-          <p data-l10n-id="pad.passwordRequired">You need a password to access this pad</p>
-          <form class='passForm' method='POST'>
-            <input id='passwordinput' type='password' name='password'><input type='submit' value='Submit'>
-          </form>
-        </div>
         <div id="permissionDenied">
           <p data-l10n-id="pad.permissionDenied">You do not have permission to access this pad</p>
         </div>
-        <div id="wrongPassword">
-          <p data-l10n-id="pad.wrongPassword">Your password was wrong</p>
-        </div>
         <% e.begin_block("loading"); %>
         <p data-l10n-id="pad.loading" id="loading">Loading...</p>
         <% e.end_block(); %>

tests/backend/specs/api/sessionsAndGroups.js

@@ -51,9 +51,6 @@ describe('API Versioning', function() {
    -> getPublicStatus(padId)
     -> setPublicStatus(padId, status)
      -> getPublicStatus(padId)
-      -> isPasswordProtected(padID) -- should be false
-       -> setPassword(padID, password)
-        -> isPasswordProtected(padID) -- should be true
 
 -> listPadsOfAuthor(authorID)
 */
@@ -269,35 +266,6 @@ describe('API: Pad security', function() {
           assert.equal(res.body.data.publicStatus, true);
         });
   });
-
-  it('isPasswordProtected', async function() {
-    await api.get(endPoint('isPasswordProtected') + `&padID=${padID}`)
-        .expect(200)
-        .expect('Content-Type', /json/)
-        .expect((res) => {
-          assert.equal(res.body.code, 0);
-          assert.equal(res.body.data.isPasswordProtected, false);
-        });
-  });
-
-  it('setPassword', async function() {
-    await api.get(endPoint('setPassword') + `&padID=${padID}&password=test`)
-        .expect(200)
-        .expect('Content-Type', /json/)
-        .expect((res) => {
-          assert.equal(res.body.code, 0);
-        });
-  });
-
-  it('isPasswordProtected after setting password', async function() {
-    await api.get(endPoint('isPasswordProtected') + `&padID=${padID}`)
-        .expect(200)
-        .expect('Content-Type', /json/)
-        .expect((res) => {
-          assert.equal(res.body.code, 0);
-          assert.equal(res.body.data.isPasswordProtected, true);
-        });
-  });
 });
 
 // NOT SURE HOW TO POPULAT THIS /-_-\

tests/backend/specs/socketio.js

@@ -83,7 +83,6 @@ const handshake = async (socket, padID) => {
     type: 'CLIENT_READY',
     padId: padID,
     sessionID: null,
-    password: null,
     token: 't.12345',
     protocolVersion: 2,
   });