fix: vulnerability in cover and admin uploads (#8419)

* fix: vulnerability in cover and admin uploads

* fix: remove old test

* fix: update tests

public/src/client/account/edit.js

@@ -228,7 +228,6 @@ define('forum/account/edit', ['forum/account/header', 'translator', 'components'
 
 			pictureCropper.show({
 				socketMethod: 'user.uploadCroppedPicture',
-				route: config.relative_path + '/api/user/' + ajaxify.data.userslug + '/uploadpicture',
 				aspectRatio: 1 / 1,
 				paramName: 'uid',
 				paramValue: ajaxify.data.theirid,

src/file.js

@@ -30,6 +30,9 @@ file.saveFileToLocal = async function (filename, folder, tempPath) {
 	filename = filename.split('.').map(name => utils.slugify(name)).join('.');
 
 	const uploadPath = path.join(nconf.get('upload_path'), folder, filename);
+	if (!uploadPath.startsWith(nconf.get('upload_path'))) {
+		throw new Error('[[error:invalid-path]]');
+	}
 
 	winston.verbose('Saving file ' + filename + ' to : ' + uploadPath);
 	await mkdirp(path.dirname(uploadPath));

src/image.js

@@ -101,7 +101,7 @@ image.stripEXIF = async function (path) {
 		const sharp = requireSharp();
 		await sharp(buffer, { failOnError: true }).rotate().toFile(path);
 	} catch (err) {
-		winston.error(err);
+		winston.error(err.stack);
 	}
 };
 

src/user/picture.js

@@ -45,9 +45,9 @@ module.exports = function (User) {
 
 			validateUpload(data, meta.config.maximumCoverImageSize, ['image/png', 'image/jpeg', 'image/bmp']);
 
-			picture.path = await getTempPath(data);
+			picture.path = await image.writeImageDataToTempFile(data.imageData);
 
-			const extension = file.typeToExtension(getMimeType(data));
+			const extension = file.typeToExtension(image.mimeFromBase64(data.imageData));
 			const filename = data.uid + '-profilecover' + extension;
 			const uploadData = await image.uploadImage(filename, 'profile', picture);
 
@@ -61,7 +61,7 @@ module.exports = function (User) {
 				url: uploadData.url,
 			};
 		} finally {
-			file.delete(picture.path || (data.file && data.file.path));
+			file.delete(picture.path);
 		}
 	};
 
@@ -78,12 +78,12 @@ module.exports = function (User) {
 
 			validateUpload(data, meta.config.maximumProfileImageSize, User.getAllowedImageTypes());
 
-			const extension = file.typeToExtension(getMimeType(data));
+			const extension = file.typeToExtension(image.mimeFromBase64(data.imageData));
 			if (!extension) {
 				throw new Error('[[error:invalid-image-extension]]');
 			}
 
-			picture.path = await getTempPath(data);
+			picture.path = await image.writeImageDataToTempFile(data.imageData);
 			picture.path = await convertToPNG(picture.path);
 
 			await image.resizeImage({
@@ -101,36 +101,25 @@ module.exports = function (User) {
 			});
 			return uploadedImage;
 		} finally {
-			file.delete(picture.path || (data.file && data.file.path));
+			file.delete(picture.path);
 		}
 	};
 
 	function validateUpload(data, maxSize, allowedTypes) {
-		if (!data.imageData && !data.file) {
+		if (!data.imageData) {
 			throw new Error('[[error:invalid-data]]');
 		}
-		const size = data.file ? data.file.size : image.sizeFromBase64(data.imageData);
+		const size = image.sizeFromBase64(data.imageData);
 		if (size > maxSize * 1024) {
 			throw new Error('[[error:file-too-big, ' + maxSize + ']]');
 		}
 
-		const type = getMimeType(data);
+		const type = image.mimeFromBase64(data.imageData);
 		if (!type || !allowedTypes.includes(type)) {
 			throw new Error('[[error:invalid-image]]');
 		}
 	}
 
-	function getMimeType(data) {
-		return data.file ? data.file.type : image.mimeFromBase64(data.imageData);
-	}
-
-	async function getTempPath(data) {
-		if (data.file) {
-			return data.file.path;
-		}
-		return await image.writeImageDataToTempFile(data.imageData);
-	}
-
 	async function convertToPNG(path) {
 		const convertToPNG = meta.config['profile:convertProfileImageToPNG'] === 1;
 		if (!convertToPNG) {

test/file.js

@@ -96,6 +96,14 @@ describe('file', function () {
 				done();
 			});
 		});
+
+		it('should error if folder is relative', function (done) {
+			file.saveFileToLocal(filename, '../../text', tempPath + '000000000', function (err) {
+				assert(err);
+				assert.strictEqual(err.message, '[[error:invalid-path]]');
+				done();
+			});
+		});
 	});
 
 	it('should walk directory', function (done) {

test/helpers/index.js

@@ -107,7 +107,7 @@ helpers.uploadFile = function (uploadEndPoint, filePath, body, jar, csrf_token,
 			return callback(err);
 		}
 		if (res.statusCode !== 200) {
-			winston.error(body);
+			winston.error(JSON.stringify(body));
 		}
 		callback(null, res, body);
 	});

test/uploads.js

@@ -220,6 +220,13 @@ describe('Upload Controllers', function () {
 			});
 		});
 
+		it('should not allow non image uploads', function (done) {
+			socketUser.updateCover({ uid: 1 }, { uid: 1, file: { path: '../../text.txt' } }, function (err) {
+				assert.equal(err.message, '[[error:invalid-data]]');
+				done();
+			});
+		});
+
 		it('should not allow non image uploads', function (done) {
 			socketUser.updateCover({ uid: 1 }, { uid: 1, imageData: 'data:text/html;base64,PHN2Zy9vbmxvYWQ9YWxlcnQoMik+' }, function (err) {
 				assert.equal(err.message, '[[error:invalid-image]]');
@@ -234,6 +241,13 @@ describe('Upload Controllers', function () {
 			});
 		});
 
+		it('should not allow non image uploads', function (done) {
+			socketUser.uploadCroppedPicture({ uid: 1 }, { uid: 1, file: { path: '../../text.txt' } }, function (err) {
+				assert.equal(err.message, '[[error:invalid-data]]');
+				done();
+			});
+		});
+
 		it('should not allow non image uploads', function (done) {
 			socketUser.uploadCroppedPicture({ uid: 1 }, { uid: 1, imageData: 'data:text/html;base64,PHN2Zy9vbmxvYWQ9YWxlcnQoMik+' }, function (err) {
 				assert.equal(err.message, '[[error:invalid-image]]');
@@ -396,5 +410,33 @@ describe('Upload Controllers', function () {
 				});
 			});
 		});
+
+		it('should upload regular file', function (done) {
+			helpers.uploadFile(nconf.get('url') + '/api/admin/upload/file', path.join(__dirname, '../test/files/test.png'), {
+				params: JSON.stringify({
+					folder: 'system',
+				}),
+			}, jar, csrf_token, function (err, res, body) {
+				assert.ifError(err);
+				assert.equal(res.statusCode, 200);
+				assert(Array.isArray(body));
+				assert.equal(body[0].url, '/assets/uploads/system/test.png');
+				assert(file.existsSync(path.join(nconf.get('upload_path'), 'system', 'test.png')));
+				done();
+			});
+		});
+
+		it('should fail to upload regular file in wrong directory', function (done) {
+			helpers.uploadFile(nconf.get('url') + '/api/admin/upload/file', path.join(__dirname, '../test/files/test.png'), {
+				params: JSON.stringify({
+					folder: '../../system',
+				}),
+			}, jar, csrf_token, function (err, res, body) {
+				assert.ifError(err);
+				assert.equal(res.statusCode, 500);
+				assert.strictEqual(body.error, '[[error:invalid-path]]');
+				done();
+			});
+		});
 	});
 });

test/user.js

@@ -936,31 +936,6 @@ describe('User', function () {
 			});
 		});
 
-		it('should upload profile picture', function (done) {
-			helpers.copyFile(
-				path.join(nconf.get('base_dir'), 'test/files/test.png'),
-				path.join(nconf.get('base_dir'), 'test/files/test_copy.png'),
-				function (err) {
-					assert.ifError(err);
-					var picture = {
-						path: path.join(nconf.get('base_dir'), 'test/files/test_copy.png'),
-						size: 7189,
-						name: 'test_copy.png',
-						type: 'image/png',
-					};
-					User.uploadCroppedPicture({
-						uid: uid,
-						file: picture,
-					}, function (err, uploadedPicture) {
-						assert.ifError(err);
-						assert.equal(uploadedPicture.url, '/assets/uploads/profile/' + uid + '-profileavatar.png');
-						assert.equal(uploadedPicture.path, path.join(nconf.get('upload_path'), 'profile', uid + '-profileavatar.png'));
-						done();
-					});
-				}
-			);
-		});
-
 		it('should return error if profile image uploads disabled', function (done) {
 			meta.config.allowProfileImageUploads = 0;
 			var picture = {
@@ -974,37 +949,15 @@ describe('User', function () {
 				file: picture,
 			}, function (err) {
 				assert.equal(err.message, '[[error:profile-image-uploads-disabled]]');
-				done();
-			});
-		});
-
-		it('should return error if profile image is too big', function (done) {
 				meta.config.allowProfileImageUploads = 1;
-			var picture = {
-				path: path.join(nconf.get('base_dir'), 'test/files/test_copy.png'),
-				size: 265000,
-				name: 'test.png',
-				type: 'image/png',
-			};
-
-			User.uploadCroppedPicture({
-				uid: uid,
-				file: picture,
-			}, function (err) {
-				assert.equal(err.message, '[[error:file-too-big, 256]]');
 				done();
 			});
 		});
 
 		it('should return error if profile image has no mime type', function (done) {
-			var picture = {
-				path: path.join(nconf.get('base_dir'), 'test/files/test_copy.png'),
-				size: 7189,
-				name: 'test',
-			};
 			User.uploadCroppedPicture({
 				uid: uid,
-				file: picture,
+				imageData: 'data:image/invalid;base64,R0lGODlhPQBEAPeoAJosM/',
 			}, function (err) {
 				assert.equal(err.message, '[[error:invalid-image]]');
 				done();