From a817acbbccad9cbcc5fc69ccd9f4c8eafe36b86e Mon Sep 17 00:00:00 2001 From: muxator Date: Sat, 7 Dec 2019 04:36:01 +0100 Subject: [PATCH] security: when served over https, set the "secure" flag for "express_sid" and "language" cookie The mechanism used for determining if the application is being served over SSL is wrapped by the "express-session" library for "express_sid", and manual for the "language" cookie, but it's very similar in both cases. The "secure" flag is set if one of these is true: 1. we are directly serving Etherpad over SSL using the native nodejs functionality, via the "ssl" options in settings.json 2. Etherpad is being served in plaintext by nodejs, but we are using a reverse proxy for terminating the SSL for us; In this case, the user has to be instructed to properly set trustProxy: true in settings.json, and the information wheter the application is over SSL or not will be extracted from the X-Forwarded-Proto HTTP header. Please note that this will not be compatible with applications being served over http and https at the same time. The change on webaccess.js amends 009b61b33843, which did not work when the SSL termination was performed by a reverse proxy. Reference for automatic "express_sid" configuration: https://github.com/expressjs/session/blob/v1.17.0/README.md#cookiesecure Closes #3561. --- CHANGELOG.md | 1 + doc/docker.md | 1 + settings.json.docker | 8 +++++++- settings.json.template | 6 ++++++ src/node/hooks/express/specialpages.js | 19 ++++++++++++++++++- src/node/hooks/express/webaccess.js | 22 +++++++++++++++++++++- 6 files changed, 54 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8580806d..618a88e10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # 1.8 * SECURITY: change referrer policy so that Etherpad addresses aren't leaked when links are clicked (discussion: https://github.com/ether/etherpad-lite/pull/3636) +* SECURITY: set the "secure" flag for the session cookies when served over SSL. From now on it will not be possible to serve the same instance both in cleartext and over SSL # 1.8-beta.1 * FEATURE: code was migrated to `async`/`await`, getting rid of a lot of callbacks (see https://github.com/ether/etherpad-lite/issues/3540) diff --git a/doc/docker.md b/doc/docker.md index d16e7df58..e77cde409 100644 --- a/doc/docker.md +++ b/doc/docker.md @@ -71,6 +71,7 @@ Available options: * `DB_FILENAME`: in case `DB_TYPE` is `DirtyDB`, the database filename. Default: `var/dirty.db` * `ADMIN_PASSWORD`: the password for the `admin` user (leave unspecified if you do not want to create it) * `USER_PASSWORD`: the password for the first user `user` (leave unspecified if you do not want to create it) +* `TRUST_PROXY`: set to `true` if you are using a reverse proxy in front of Etherpad (for example: Traefik for SSL termination via Let's Encrypt). This will affect security and correctness of the logs if not done * `LOGLEVEL`: valid values are `DEBUG`, `INFO`, `WARN` and `ERROR` ### Examples diff --git a/settings.json.docker b/settings.json.docker index 1fef5660f..ef9893258 100644 --- a/settings.json.docker +++ b/settings.json.docker @@ -287,8 +287,14 @@ /* * When you use NGINX or another proxy/load-balancer set this to true. + * + * This is especially necessary when the reverse proxy performs SSL + * termination, otherwise the cookies will not have the "secure" flag. + * + * The other effect will be that the logs will contain the real client's IP, + * instead of the reverse proxy's IP. */ - "trustProxy": false, + "trustProxy": "${TRUST_PROXY:false}", /* * Privacy: disable IP logging diff --git a/settings.json.template b/settings.json.template index 5f94d3ada..598f07611 100644 --- a/settings.json.template +++ b/settings.json.template @@ -290,6 +290,12 @@ /* * When you use NGINX or another proxy/load-balancer set this to true. + * + * This is especially necessary when the reverse proxy performs SSL + * termination, otherwise the cookies will not have the "secure" flag. + * + * The other effect will be that the logs will contain the real client's IP, + * instead of the reverse proxy's IP. */ "trustProxy": false, diff --git a/src/node/hooks/express/specialpages.js b/src/node/hooks/express/specialpages.js index 69d23ff6d..035074e02 100644 --- a/src/node/hooks/express/specialpages.js +++ b/src/node/hooks/express/specialpages.js @@ -45,7 +45,24 @@ exports.expressCreateServer = function (hook_name, args, cb) { // Or if language cookie doesn't exist if (req.cookies.language === undefined) { - res.cookie('language', settings.padOptions.lang); + cookieOptions = { + /* req.protocol may be 'https' because either: + * + * 1. we are directly serving the nodejs application over SSL, using + * the "ssl" options in settings.json + * + * 2. we are serving the nodejs application in plaintext, but we are + * using a reverse proxy that terminates SSL for us. In this case, + * the user has to set trustProxy = true in settings.json, and thus + * req.protocol will reflect the value of the X-Forwarded-Proto HTTP + * header + * + * Please note that this will not be compatible with applications being + * served over http and https at the same time. + */ + secure: (req.protocol === 'https'), + } + res.cookie('language', settings.padOptions.lang, cookieOptions); } // The below might break for pads being rewritten diff --git a/src/node/hooks/express/webaccess.js b/src/node/hooks/express/webaccess.js index 27825c66f..01a040258 100644 --- a/src/node/hooks/express/webaccess.js +++ b/src/node/hooks/express/webaccess.js @@ -131,7 +131,27 @@ exports.expressConfigure = function (hook_name, args, cb) { name: 'express_sid', proxy: true, cookie: { - secure: !!settings.ssl, + /* + * The automatic express-session mechanism for determining if the + * application is being served over ssl is similar to the one used for + * setting the language cookie, which check if one of these conditions is + * true: + * + * 1. we are directly serving the nodejs application over SSL, using the + * "ssl" options in settings.json + * + * 2. we are serving the nodejs application in plaintext, but we are using + * a reverse proxy that terminates SSL for us. In this case, the user + * has to set trustProxy = true in settings.json, and the information + * wheter the application is over SSL or not will be extracted from the + * X-Forwarded-Proto HTTP header + * + * Please note that this will not be compatible with applications being + * served over http and https at the same time. + * + * reference: https://github.com/expressjs/session/blob/v1.17.0/README.md#cookiesecure + */ + secure: 'auto', } }));