From dc29a286e665555cccb92760908e50cd967fd2e7 Mon Sep 17 00:00:00 2001 From: Sheogorath Date: Mon, 23 Nov 2020 12:42:19 +0100 Subject: Fix arbitary file upload for uploadimage API endpoint This patch fixes a security issue with all existing CodiMD and HedgeDoc installation which allows arbitary file uploads to instances that expose the `/uploadimage` API endpoint. With the patch it implies the same restrictions on the MIME-types as the frontend does. Means only images are allowed unless configured differently. This issue was reported by Thomas Lambertz. To verify if you are vulnerable or not, create two files `test.html` and `test.png` and try to upload them to your hedgedoc installation. ``` curl -X POST -F "image=@$(pwd)/test.html" http://localhost:3000/uploadimage curl -X POST -F "image=@$(pwd)/test.png" http://localhost:3000/uploadimage ``` Note: Not all backends are affected. Imgur and lutim should prevent this by their own upload API. But S3, minio, filesystem and azure, will be at risk. Addition Note: When using filesystem instead of an external uploads providers, there is a higher risk of code injections as the default CSP do not block JS from the main domain. References: https://github.com/hedgedoc/hedgedoc/security/advisories/GHSA-wcr3-xhv7-8gxc Signed-off-by: Christoph Kern --- lib/web/imageRouter/index.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'lib/web/imageRouter') diff --git a/lib/web/imageRouter/index.js b/lib/web/imageRouter/index.js index aa02e9b0..b5c486c3 100644 --- a/lib/web/imageRouter/index.js +++ b/lib/web/imageRouter/index.js @@ -20,9 +20,15 @@ imageRouter.post('/uploadimage', function (req, res) { } form.parse(req, function (err, fields, files) { - if (err || !files.image || !files.image.path) { + if (err) { logger.error(`formidable error: ${err}`) - errors.errorForbidden(res) + return errors.errorForbidden(res) + } else if (!files.image || !files.image.path) { + logger.error(`formidable error: Upload didn't contain file)`) + return errors.errorBadRequest(res) + } else if (!config.allowedUploadMimeTypes.includes(files.image.type)) { + logger.error(`formidable error: MIME-type "${files.image.type}" of uploaded file not allowed, only "${config.allowedUploadMimeTypes.join(', ')}" are allowed)`) + return errors.errorBadRequest(res) } else { logger.debug(`SERVER received uploadimage: ${JSON.stringify(files.image)}`) -- cgit v1.2.3 From d097211c545118ac13626e1b0a01390b08880ad7 Mon Sep 17 00:00:00 2001 From: Sheogorath Date: Mon, 23 Nov 2020 12:50:39 +0100 Subject: Fix unauthenticated file uploads This patch fixes the issue of unauthenticated users, being able to upload files, even when anonymous edits are disabled. It's implemented by blocking uploads when either `allowAnonymous` is set to `false` for all unauthenticated users, unless `allowAnonymousEdits` is set to true, to make sure anonymous editors still experience the full feature set. Signed-off-by: Christoph Kern --- lib/web/imageRouter/index.js | 3 +++ 1 file changed, 3 insertions(+) (limited to 'lib/web/imageRouter') diff --git a/lib/web/imageRouter/index.js b/lib/web/imageRouter/index.js index b5c486c3..f456fd30 100644 --- a/lib/web/imageRouter/index.js +++ b/lib/web/imageRouter/index.js @@ -23,6 +23,9 @@ imageRouter.post('/uploadimage', function (req, res) { if (err) { logger.error(`formidable error: ${err}`) return errors.errorForbidden(res) + } else if (!req.isAuthenticated() && !config.allowAnonymous && !config.allowAnonymousEdits) { + logger.error(`formidable error: Anonymous edits and therefore uploads are not allowed)`) + return errors.errorForbidden(res) } else if (!files.image || !files.image.path) { logger.error(`formidable error: Upload didn't contain file)`) return errors.errorBadRequest(res) -- cgit v1.2.3 From f83e4d66ed2b6a7f7f8939e2eb63d262387e9374 Mon Sep 17 00:00:00 2001 From: Sheogorath Date: Mon, 23 Nov 2020 13:59:50 +0100 Subject: Rework error messages for image uploads This patch reworks the error messages for image uploads to make more sense. Instead of using the current `formidable error` for everything, all custom error detection now provide the (hopefully) more useful `Image Upload error` prefix for error messages. Signed-off-by: Christoph Kern --- lib/web/imageRouter/index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'lib/web/imageRouter') diff --git a/lib/web/imageRouter/index.js b/lib/web/imageRouter/index.js index f456fd30..b6ace4a6 100644 --- a/lib/web/imageRouter/index.js +++ b/lib/web/imageRouter/index.js @@ -21,16 +21,16 @@ imageRouter.post('/uploadimage', function (req, res) { form.parse(req, function (err, fields, files) { if (err) { - logger.error(`formidable error: ${err}`) + logger.error(`Image upload error: formidable error: ${err}`) return errors.errorForbidden(res) } else if (!req.isAuthenticated() && !config.allowAnonymous && !config.allowAnonymousEdits) { - logger.error(`formidable error: Anonymous edits and therefore uploads are not allowed)`) + logger.error(`Image upload error: Anonymous edits and therefore uploads are not allowed)`) return errors.errorForbidden(res) } else if (!files.image || !files.image.path) { - logger.error(`formidable error: Upload didn't contain file)`) + logger.error(`Image upload error: Upload didn't contain file)`) return errors.errorBadRequest(res) } else if (!config.allowedUploadMimeTypes.includes(files.image.type)) { - logger.error(`formidable error: MIME-type "${files.image.type}" of uploaded file not allowed, only "${config.allowedUploadMimeTypes.join(', ')}" are allowed)`) + logger.error(`Image upload error: MIME-type "${files.image.type}" of uploaded file not allowed, only "${config.allowedUploadMimeTypes.join(', ')}" are allowed)`) return errors.errorBadRequest(res) } else { logger.debug(`SERVER received uploadimage: ${JSON.stringify(files.image)}`) -- cgit v1.2.3 From cf4344d9e031d2e0bf70b8d8f75ab27ecf8d29ad Mon Sep 17 00:00:00 2001 From: David Mehren Date: Sun, 27 Dec 2020 11:31:01 +0100 Subject: Improve MIME-type checks of uploaded files This commit adds a check if the MIME-type of the uploaded file (detected using the magic bytes) matches the file extension. Signed-off-by: David Mehren --- lib/web/imageRouter/index.js | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) (limited to 'lib/web/imageRouter') diff --git a/lib/web/imageRouter/index.js b/lib/web/imageRouter/index.js index b6ace4a6..5861a2bc 100644 --- a/lib/web/imageRouter/index.js +++ b/lib/web/imageRouter/index.js @@ -2,6 +2,8 @@ const Router = require('express').Router const formidable = require('formidable') +const path = require('path') +const FileType = require('file-type') const config = require('../../config') const logger = require('../../logger') @@ -9,6 +11,23 @@ const errors = require('../../errors') const imageRouter = module.exports = Router() +async function checkUploadType (filePath) { + const typeFromMagic = await FileType.fromFile(filePath) + if (typeFromMagic === undefined) { + logger.error(`Image upload error: Could not determine MIME-type`) + return false + } + if (path.extname(filePath) !== '.' + typeFromMagic.ext) { + logger.error(`Image upload error: Provided file extension does not match MIME-type`) + return false + } + if (!config.allowedUploadMimeTypes.includes(typeFromMagic.mime)) { + logger.error(`Image upload error: MIME-type "${typeFromMagic.mime}" of uploaded file not allowed, only "${config.allowedUploadMimeTypes.join(', ')}" are allowed`) + return false + } + return true +} + // upload image imageRouter.post('/uploadimage', function (req, res) { var form = new formidable.IncomingForm() @@ -19,18 +38,17 @@ imageRouter.post('/uploadimage', function (req, res) { form.uploadDir = config.uploadsPath } - form.parse(req, function (err, fields, files) { + form.parse(req, async function (err, fields, files) { if (err) { logger.error(`Image upload error: formidable error: ${err}`) return errors.errorForbidden(res) - } else if (!req.isAuthenticated() && !config.allowAnonymous && !config.allowAnonymousEdits) { + } else if (!req.isAuthenticated() && !config.allowAnonymous && !config.allowAnonymousEdits) { logger.error(`Image upload error: Anonymous edits and therefore uploads are not allowed)`) return errors.errorForbidden(res) } else if (!files.image || !files.image.path) { logger.error(`Image upload error: Upload didn't contain file)`) return errors.errorBadRequest(res) - } else if (!config.allowedUploadMimeTypes.includes(files.image.type)) { - logger.error(`Image upload error: MIME-type "${files.image.type}" of uploaded file not allowed, only "${config.allowedUploadMimeTypes.join(', ')}" are allowed)`) + } else if (!await checkUploadType(files.image.path)) { return errors.errorBadRequest(res) } else { logger.debug(`SERVER received uploadimage: ${JSON.stringify(files.image)}`) -- cgit v1.2.3 From 6932cc4df7e0c2826e47b2d9ca2f0031f75b1b58 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Sun, 27 Dec 2020 15:52:26 +0100 Subject: Always save uploads to a tmpdir first and cleanup afterwards This makes sure no unintended files are permanently saved. Co-authored-by: Yannick Bungers Signed-off-by: David Mehren --- lib/web/imageRouter/filesystem.js | 11 ++++++++++- lib/web/imageRouter/index.js | 22 ++++++++++++++-------- 2 files changed, 24 insertions(+), 9 deletions(-) (limited to 'lib/web/imageRouter') diff --git a/lib/web/imageRouter/filesystem.js b/lib/web/imageRouter/filesystem.js index 3ba09e88..f8fd7e16 100644 --- a/lib/web/imageRouter/filesystem.js +++ b/lib/web/imageRouter/filesystem.js @@ -1,6 +1,7 @@ 'use strict' const URL = require('url').URL const path = require('path') +const fs = require('fs') const config = require('../../config') const logger = require('../../logger') @@ -16,5 +17,13 @@ exports.uploadImage = function (imagePath, callback) { return } - callback(null, (new URL(path.basename(imagePath), config.serverURL + '/uploads/')).href) + const fileName = path.basename(imagePath) + // move image from temporary path to upload directory + try { + fs.copyFileSync(imagePath, path.join(config.uploadsPath, fileName)) + } catch (e) { + callback(new Error('Error while moving file'), null) + return + } + callback(null, (new URL(fileName, config.serverURL + '/uploads/')).href) } diff --git a/lib/web/imageRouter/index.js b/lib/web/imageRouter/index.js index 5861a2bc..afa9bbf6 100644 --- a/lib/web/imageRouter/index.js +++ b/lib/web/imageRouter/index.js @@ -4,6 +4,9 @@ const Router = require('express').Router const formidable = require('formidable') const path = require('path') const FileType = require('file-type') +const fs = require('fs') +const os = require('os') +const rimraf = require('rimraf') const config = require('../../config') const logger = require('../../logger') @@ -30,25 +33,27 @@ async function checkUploadType (filePath) { // upload image imageRouter.post('/uploadimage', function (req, res) { - var form = new formidable.IncomingForm() + if (!req.isAuthenticated() && !config.allowAnonymous && !config.allowAnonymousEdits) { + logger.error(`Image upload error: Anonymous edits and therefore uploads are not allowed)`) + return errors.errorForbidden(res) + } + var form = new formidable.IncomingForm() form.keepExtensions = true - - if (config.imageUploadType === 'filesystem') { - form.uploadDir = config.uploadsPath - } + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'hedgedoc-')) + form.uploadDir = tmpDir form.parse(req, async function (err, fields, files) { if (err) { logger.error(`Image upload error: formidable error: ${err}`) - return errors.errorForbidden(res) - } else if (!req.isAuthenticated() && !config.allowAnonymous && !config.allowAnonymousEdits) { - logger.error(`Image upload error: Anonymous edits and therefore uploads are not allowed)`) + rimraf(tmpDir) return errors.errorForbidden(res) } else if (!files.image || !files.image.path) { logger.error(`Image upload error: Upload didn't contain file)`) + rimraf.sync(tmpDir) return errors.errorBadRequest(res) } else if (!await checkUploadType(files.image.path)) { + rimraf.sync(tmpDir) return errors.errorBadRequest(res) } else { logger.debug(`SERVER received uploadimage: ${JSON.stringify(files.image)}`) @@ -56,6 +61,7 @@ imageRouter.post('/uploadimage', function (req, res) { const uploadProvider = require('./' + config.imageUploadType) logger.debug(`imageRouter: Uploading ${files.image.path} using ${config.imageUploadType}`) uploadProvider.uploadImage(files.image.path, function (err, url) { + rimraf.sync(tmpDir) if (err !== null) { logger.error(err) return res.status(500).end('upload image error') -- cgit v1.2.3