diff --git a/frontend/express/app.js b/frontend/express/app.js index 0285e98077d..870bcba1201 100644 --- a/frontend/express/app.js +++ b/frontend/express/app.js @@ -533,7 +533,8 @@ Promise.all([plugins.dbConnection(countlyConfig), plugins.dbConnection("countly_ 'Date': new Date().toUTCString(), 'Last-Modified': stats.mtime.toUTCString(), 'Content-Type': 'image/png', - 'Content-Length': stats.size + 'Content-Length': stats.size, + 'X-Content-Type-Options': 'nosniff' }); stream.pipe(res); } @@ -581,7 +582,8 @@ Promise.all([plugins.dbConnection(countlyConfig), plugins.dbConnection("countly_ 'Date': new Date().toUTCString(), 'Last-Modified': stats.mtime.toUTCString(), 'Content-Type': 'image/png', - 'Content-Length': stats.size + 'Content-Length': stats.size, + 'X-Content-Type-Options': 'nosniff' }); stream.pipe(res); } @@ -620,7 +622,8 @@ Promise.all([plugins.dbConnection(countlyConfig), plugins.dbConnection("countly_ 'Date': new Date().toUTCString(), 'Last-Modified': stats.mtime.toUTCString(), 'Content-Type': 'image/png', - 'Content-Length': stats.size + 'Content-Length': stats.size, + 'X-Content-Type-Options': 'nosniff' }); stream.pipe(res); }); diff --git a/plugins/dashboards/frontend/app.js b/plugins/dashboards/frontend/app.js index ca18d612727..78052f58e7c 100644 --- a/plugins/dashboards/frontend/app.js +++ b/plugins/dashboards/frontend/app.js @@ -53,7 +53,8 @@ var log = common.log('dashboards:frontend'); 'Date': new Date().toUTCString(), 'Last-Modified': stats.mtime.toUTCString(), 'Content-Type': 'image/png', - 'Content-Length': stats.size + 'Content-Length': stats.size, + 'X-Content-Type-Options': 'nosniff' }); stream.pipe(res); }); diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index db73147274a..abf333c1865 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -5,8 +5,9 @@ var exported = {}, log = common.log('star-rating:api'), countlyCommon = require('../../../api/lib/countly.common.js'), plugins = require('../../pluginManager.js'), - { validateCreate, validateRead, validateUpdate, validateDelete } = require('../../../api/utils/rights.js'), - countlyFs = require('../../../api/utils/countlyFs.js'); + { validateCreate, validateRead, validateUpdate, validateDelete, validateGlobalAdmin, validateAppAdmin } = require('../../../api/utils/rights.js'), + countlyFs = require('../../../api/utils/countlyFs.js'), + imageUtils = require('./image-utils.js'); var fetch = require('../../../api/parts/data/fetch.js'); var ejs = require("ejs"), fs = require('fs'), @@ -243,6 +244,16 @@ function create_upload_dir(callback) { }); } +// Map sniffed image MIME → file extension to use on disk. Determines +// the saved filename and the URL component returned to the caller. +// Restricted to png/jpeg/gif to preserve the original /i/feedback/logo +// contract (no widening to webp here). +var SNIFFED_TYPE_TO_EXT = { + "image/png": "png", + "image/jpeg": "jpg", + "image/gif": "gif" +}; + /** * Used for file upload * @param {object} myfile - file object(if empty - returns) @@ -255,48 +266,38 @@ function uploadFile(myfile, id, callback) { return; } var tmp_path = myfile.path; - var type = myfile.type; - myfile.name = myfile.name || "png"; - if (type !== "image/png" && type !== "image/gif" && type !== "image/jpeg") { - fs.unlink(tmp_path, function() { }); - callback("Invalid image format. Must be png or jpeg"); - return; - } - - var allowedExtensions = ["gif", "jpeg", "jpg", "png"]; - var ext = myfile.name.split("."); - ext = ext[ext.length - 1]; - - if (allowedExtensions.indexOf(ext) === -1) { - callback("Invalid file extension. Must be .png, .jpg, .gif or .jpeg"); - return; - } create_upload_dir(function() { fs.readFile(tmp_path, (err, data) => { - if (err) { + if (err || !data) { + fs.unlink(tmp_path, function() { }); callback("Failed to upload image"); return; } - //convert file to data - if (data) { - try { - var pp = path.resolve(__dirname, './../images/' + id + "." + ext); - countlyFs.saveData("star-rating", pp, data, { id: "" + id + "." + ext, writeMode: "overwrite" }, function(err3) { - if (err3) { - callback("Failed to upload image"); - } - else { - fs.unlink(tmp_path, function() { }); - callback(true, id + "." + ext); - } - }); - } - catch (SyntaxError) { - callback("Failed to upload image"); - } + // Detect format from magic bytes; never trust myfile.type or + // myfile.name. Anything not in the allowlist (png/jpeg/gif) + // is rejected. + var detectedType = imageUtils.sniffImageType(data); + var detectedExt = detectedType && SNIFFED_TYPE_TO_EXT[detectedType]; + if (!detectedExt) { + fs.unlink(tmp_path, function() { }); + callback("Invalid image format. Must be png, jpeg, or gif"); + return; } - else { + try { + var pp = path.resolve(__dirname, './../images/' + id + "." + detectedExt); + countlyFs.saveData("star-rating", pp, data, { id: "" + id + "." + detectedExt, writeMode: "overwrite" }, function(err3) { + fs.unlink(tmp_path, function() { }); + if (err3) { + callback("Failed to upload image"); + } + else { + callback(true, id + "." + detectedExt); + } + }); + } + catch (SyntaxError) { + fs.unlink(tmp_path, function() { }); callback("Failed to upload image"); } }); @@ -371,36 +372,39 @@ function uploadFile(myfile, id, callback) { function uploadFeedbackFile(myname, myfile) { return new Promise(function(resolve, reject) { var tmp_path = myfile.path; - var type = myfile.type; if (myfile.size > 1.5 * 1024 * 1024) { fs.unlink(tmp_path, function() {}); reject(Error("feedback.image-error")); } else { fs.readFile(tmp_path, (err, data) => { - if (err) { + if (err || !data) { + fs.unlink(tmp_path, function() {}); reject(Error("feedback.imagee-error")); + return; } - //convert file to data - if (data) { - try { - var data_uri_prefix = "data:" + type + ";base64,"; - var buf = Buffer.from(data); - var image = buf.toString('base64'); - image = data_uri_prefix + image; - countlyFs.gridfs.saveData("feedback", myname, image, {id: myname, writeMode: "overwrite"}, function(err2) { - fs.unlink(tmp_path, function() {}); - if (err2) { - return reject(err2); - } - resolve(); - }); - } - catch (SyntaxError) { - reject(Error("feedback.imagee-error")); - } + var buf = Buffer.from(data); + // Detect MIME from the actual bytes; never trust myfile.type. + // Anything that isn't a recognized safe raster image is rejected. + var detectedType = imageUtils.sniffImageType(buf); + if (!detectedType) { + fs.unlink(tmp_path, function() {}); + reject(Error("feedback.imagef-error")); + return; } - else { + try { + var data_uri_prefix = "data:" + detectedType + ";base64,"; + var image = data_uri_prefix + buf.toString('base64'); + countlyFs.gridfs.saveData("feedback", myname, image, {id: myname, writeMode: "overwrite"}, function(err2) { + fs.unlink(tmp_path, function() {}); + if (err2) { + return reject(err2); + } + resolve(); + }); + } + catch (SyntaxError) { + fs.unlink(tmp_path, function() {}); reject(Error("feedback.imagee-error")); } }); @@ -430,37 +434,64 @@ function uploadFile(myfile, id, callback) { * } */ plugins.register("/i/feedback/upload", function(ob) { - // do not respond if this isn't feedback fetch request + // do not respond if this isn't feedback fetch request // or surveys plugin enabled if (surveysEnabled) { return false; } var params = ob.params; - validateUpdate(params, "global_plugins", function() { - var images = ["feedback_logo"]; - var flag = 0; - if (params.files) { - for (let i = 0; i < images.length; i++) { - if (params.files[images[i]]) { - flag = 1; - uploadFeedbackFile(images[i], params.files[images[i]]).then(function() { - common.returnOutput(params, {"result": "Success"}); - }, function(err) { - common.returnMessage(params, 400, err.message); - }); - break; - } - } - if (flag === 0) { - uploadFeedbackFile(params.qstring.name, params.files.file).then(function() { - common.returnOutput(params, {"result": "Success"}); - }, function(err) { - common.returnMessage(params, 400, err.message); - }); - } - } - }); + if (!params.files) { + common.returnMessage(params, 400, "feedback.imagee-error"); + return true; + } + + // Two upload modes: + // 1. Global feedback logo: file posted under field "feedback_logo", + // stored under id "feedback_logo". Modifies a global plugin + // setting and therefore requires actual global admin. + // 2. Per-app feedback logo: file posted under field "file" with + // ?name=feedback_logo<24-char-hex-app-id>. The target app id is + // decoded from the name itself, NOT taken from qstring.app_id, + // so an admin of one app can't plant a logo for another app. + // + // Any other name shape is rejected. + var globalUpload = !!params.files.feedback_logo; + var fileObj = globalUpload ? params.files.feedback_logo : params.files.file; + var requestedName = globalUpload ? "feedback_logo" : (params.qstring && params.qstring.name); + + var parsed = imageUtils.parseFeedbackLogoName(requestedName); + if (!parsed.valid || !fileObj) { + // Reuse the existing localized "invalid format" key rather than + // adding a new key that would need translating across 25+ locale + // files. From the user's perspective both conditions (bad name + // shape, missing file) are "your upload was malformed, try again". + common.returnMessage(params, 400, "feedback.imagef-error"); + return true; + } + + /** + * Run the actual file upload after auth has passed. + * @returns {void} + */ + function performUpload() { + uploadFeedbackFile(requestedName, fileObj).then(function() { + common.returnOutput(params, {"result": "Success"}); + }, function(err) { + common.returnMessage(params, 400, err.message); + }); + } + + if (parsed.isGlobal) { + validateGlobalAdmin(params, performUpload); + } + else { + // Pin app_id to the value embedded in the upload name so the + // app-admin check can't be satisfied with admin rights on a + // different app. + params.qstring.app_id = parsed.appId; + validateAppAdmin(params, performUpload); + } return true; }); /* diff --git a/plugins/star-rating/api/image-utils.js b/plugins/star-rating/api/image-utils.js new file mode 100644 index 00000000000..3a7a649cc40 --- /dev/null +++ b/plugins/star-rating/api/image-utils.js @@ -0,0 +1,59 @@ +/** + * Detect image MIME type by inspecting magic bytes. Does NOT trust + * any client-supplied or stored MIME string. Returns one of the + * allowlisted image MIME types, or null if the buffer is not a + * recognized safe image format. + * @param {Buffer} buf - file content + * @returns {string|null} MIME type or null if not a recognized image + */ +function sniffImageType(buf) { + if (!buf || buf.length < 12) { + return null; + } + // PNG: 89 50 4E 47 0D 0A 1A 0A + if (buf[0] === 0x89 && buf[1] === 0x50 && buf[2] === 0x4E && buf[3] === 0x47 + && buf[4] === 0x0D && buf[5] === 0x0A && buf[6] === 0x1A && buf[7] === 0x0A) { + return 'image/png'; + } + // JPEG: FF D8 FF + if (buf[0] === 0xFF && buf[1] === 0xD8 && buf[2] === 0xFF) { + return 'image/jpeg'; + } + // GIF87a / GIF89a: 47 49 46 38 (37|39) 61 + if (buf[0] === 0x47 && buf[1] === 0x49 && buf[2] === 0x46 && buf[3] === 0x38 + && (buf[4] === 0x37 || buf[4] === 0x39) && buf[5] === 0x61) { + return 'image/gif'; + } + // WebP: "RIFF" .... "WEBP" + if (buf[0] === 0x52 && buf[1] === 0x49 && buf[2] === 0x46 && buf[3] === 0x46 + && buf[8] === 0x57 && buf[9] === 0x45 && buf[10] === 0x42 && buf[11] === 0x50) { + return 'image/webp'; + } + return null; +} + +// Allowed feedback logo names: the literal global "feedback_logo" or +// "feedback_logo<24-char-hex-app-id>" for per-app logos. +var FEEDBACK_LOGO_NAME_RE = /^feedback_logo([a-f0-9]{24})?$/; + +/** + * Validate a feedback logo name and, if it's a per-app logo, return + * the app id encoded in it. + * @param {string} name - candidate name + * @returns {object} parse result with `valid`, `isGlobal`, and `appId` fields + */ +function parseFeedbackLogoName(name) { + if (typeof name !== 'string') { + return {valid: false, isGlobal: false, appId: null}; + } + var m = FEEDBACK_LOGO_NAME_RE.exec(name); + if (!m) { + return {valid: false, isGlobal: false, appId: null}; + } + return {valid: true, isGlobal: !m[1], appId: m[1] || null}; +} + +module.exports = { + sniffImageType: sniffImageType, + parseFeedbackLogoName: parseFeedbackLogoName +}; diff --git a/plugins/star-rating/frontend/app.js b/plugins/star-rating/frontend/app.js index 037d9fb98af..9a3e976f411 100644 --- a/plugins/star-rating/frontend/app.js +++ b/plugins/star-rating/frontend/app.js @@ -1,10 +1,26 @@ var exported = {}, countlyFs = require('../../../api/utils/countlyFs.js'), countlyConfig = require("../../../frontend/express/config"), - common = require('../../../api/utils/common.js'); + common = require('../../../api/utils/common.js'), + imageUtils = require('../api/image-utils.js'); var path = require('path'); var log = common.log('star-rating:frontend'); +// Names accepted on the preview route. Tighter than the upload-side +// validator so even legacy data with unexpected ids can't be reached +// via path component shenanigans (no path separators, no dots). +var PREVIEW_NAME_RE = /^[a-zA-Z0-9_-]{1,64}$/; + +// File extension → response Content-Type for /star-rating/images/*. +// Upload side restricts saved files to png/jpg/gif; .jpeg kept for any +// legacy data uploaded before that restriction was tightened. +var STAR_RATING_EXT_TO_MIME = { + "png": "image/png", + "jpg": "image/jpeg", + "jpeg": "image/jpeg", + "gif": "image/gif" +}; + (function(plugin) { plugin.init = function(app) { @@ -28,23 +44,43 @@ var log = common.log('star-rating:frontend'); // keep this for backward compatability app.get(countlyConfig.path + '/feedback', renderPopup); app.get(countlyConfig.path + '/feedback/preview/*', function(req, res/*, next*/) { - if (!req.params || !req.params[0] || req.params[0] === '') { + if (!req.params || !req.params[0] || req.params[0] === '' || !PREVIEW_NAME_RE.test(req.params[0])) { res.sendFile(__dirname + '/public/images/default_app_icon.png'); } else { countlyFs.gridfs.getDataById("feedback", req.params[0], function(err, data) { if (err || !data) { res.sendFile(__dirname + '/public/images/default_app_icon.png'); + return; + } + var commaIdx = data.indexOf(','); + if (commaIdx === -1) { + res.sendFile(__dirname + '/public/images/default_app_icon.png'); + return; + } + var img; + try { + img = Buffer.from(data.slice(commaIdx + 1), 'base64'); + } + catch (e) { + res.sendFile(__dirname + '/public/images/default_app_icon.png'); + return; } - else { - var dd = data.split(','); - var img = Buffer.from(dd[1], 'base64'); - res.writeHead(200, { - 'Content-Type': dd = dd[0].substr(5, dd[0].length - 12), - 'Content-Length': img.length - }); - res.end(img); + // Re-derive Content-Type from sniffed bytes. Never trust + // the MIME embedded in the stored data URI. + var safeType = imageUtils.sniffImageType(img); + if (!safeType) { + res.sendFile(__dirname + '/public/images/default_app_icon.png'); + return; } + res.writeHead(200, { + 'Content-Type': safeType, + 'Content-Length': img.length, + 'X-Content-Type-Options': 'nosniff', + 'Content-Security-Policy': "sandbox; default-src 'none'", + 'Content-Disposition': 'inline' + }); + res.end(img); }); } }); @@ -54,6 +90,16 @@ var log = common.log('star-rating:frontend'); res.sendFile(path.resolve(__dirname + './../../../frontend/express/public/images/default_app_icon.png')); return; } + // Derive Content-Type from the requested filename extension. + // Upload side guarantees the saved extension matches the + // sniffed format; this just ensures the response Content-Type + // matches the actual bytes (rather than always claiming png). + var pathExt = (req.params[0].split(".").pop() || "").toLowerCase(); + var responseMime = STAR_RATING_EXT_TO_MIME[pathExt]; + if (!responseMime) { + res.sendFile(path.resolve(__dirname + './../../../frontend/express/public/images/default_app_icon.png')); + return; + } countlyFs.getStats("star-rating", imagePath, {id: "" + req.params[0]}, function(statsErr, stats) { if (statsErr || !stats || !stats.size) { res.sendFile(path.resolve(__dirname + './../../../frontend/express/public/images/default_app_icon.png')); @@ -82,8 +128,11 @@ var log = common.log('star-rating:frontend'); 'Last-Modified': stats.mtime.toUTCString(), 'Server': 'nginx/1.10.3 (Ubuntu)', 'X-Powered-By': 'Express', - 'Content-Type': 'image/png', - 'Content-Length': stats.size + 'Content-Type': responseMime, + 'Content-Length': stats.size, + 'X-Content-Type-Options': 'nosniff', + 'Content-Security-Policy': "sandbox; default-src 'none'", + 'Content-Disposition': 'inline' }); stream.pipe(res); } diff --git a/test/unit-tests/star-rating.image-utils.js b/test/unit-tests/star-rating.image-utils.js new file mode 100644 index 00000000000..73aeb10036c --- /dev/null +++ b/test/unit-tests/star-rating.image-utils.js @@ -0,0 +1,186 @@ +var should = require("should"); +var imageUtils = require("../../plugins/star-rating/api/image-utils.js"); + +// Build a buffer of length `size` whose first bytes are `prefix`. +function bufWith(prefix, size) { + var b = Buffer.alloc(size || Math.max(prefix.length, 12)); + for (var i = 0; i < prefix.length; i++) { + b[i] = prefix[i]; + } + return b; +} + +describe("star-rating image-utils", function() { + + describe("sniffImageType", function() { + it("recognizes PNG signature", function(done) { + var buf = bufWith([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]); + should(imageUtils.sniffImageType(buf)).equal("image/png"); + done(); + }); + + it("recognizes JPEG signature (FF D8 FF)", function(done) { + var buf = bufWith([0xFF, 0xD8, 0xFF, 0xE0]); + should(imageUtils.sniffImageType(buf)).equal("image/jpeg"); + done(); + }); + + it("recognizes GIF87a", function(done) { + // 'GIF87a' = 47 49 46 38 37 61 + var buf = bufWith([0x47, 0x49, 0x46, 0x38, 0x37, 0x61]); + should(imageUtils.sniffImageType(buf)).equal("image/gif"); + done(); + }); + + it("recognizes GIF89a", function(done) { + var buf = bufWith([0x47, 0x49, 0x46, 0x38, 0x39, 0x61]); + should(imageUtils.sniffImageType(buf)).equal("image/gif"); + done(); + }); + + it("recognizes WebP (RIFF....WEBP)", function(done) { + var buf = bufWith([ + 0x52, 0x49, 0x46, 0x46, // RIFF + 0x00, 0x00, 0x00, 0x00, // size placeholder + 0x57, 0x45, 0x42, 0x50 // WEBP + ]); + should(imageUtils.sniffImageType(buf)).equal("image/webp"); + done(); + }); + + it("rejects HTML payloads", function(done) { + var html = Buffer.from("", "utf8"); + should(imageUtils.sniffImageType(html)).be.null(); + done(); + }); + + it("rejects SVG payloads (XML-based, not in allowlist)", function(done) { + var svg = Buffer.from('', "utf8"); + should(imageUtils.sniffImageType(svg)).be.null(); + done(); + }); + + it("rejects plain text", function(done) { + var txt = Buffer.from("just some text content here padding padding", "utf8"); + should(imageUtils.sniffImageType(txt)).be.null(); + done(); + }); + + it("rejects empty buffer", function(done) { + should(imageUtils.sniffImageType(Buffer.alloc(0))).be.null(); + done(); + }); + + it("rejects buffer shorter than 12 bytes", function(done) { + should(imageUtils.sniffImageType(Buffer.from([0x89, 0x50, 0x4E, 0x47]))).be.null(); + done(); + }); + + it("rejects null / undefined input", function(done) { + should(imageUtils.sniffImageType(null)).be.null(); + should(imageUtils.sniffImageType(undefined)).be.null(); + done(); + }); + + it("rejects PNG signature with corrupted byte", function(done) { + // Same as PNG but byte[7] flipped + var buf = bufWith([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x00]); + should(imageUtils.sniffImageType(buf)).be.null(); + done(); + }); + + it("rejects RIFF without WEBP marker (e.g. WAV file)", function(done) { + var buf = bufWith([ + 0x52, 0x49, 0x46, 0x46, // RIFF + 0x00, 0x00, 0x00, 0x00, + 0x57, 0x41, 0x56, 0x45 // WAVE - audio, not image + ]); + should(imageUtils.sniffImageType(buf)).be.null(); + done(); + }); + + it("rejects GIF8 followed by wrong version byte", function(done) { + // 47 49 46 38 38 61 — '8' is not 7 or 9, so not a real GIF + var buf = bufWith([0x47, 0x49, 0x46, 0x38, 0x38, 0x61]); + should(imageUtils.sniffImageType(buf)).be.null(); + done(); + }); + + it("does not sniff payload past the header", function(done) { + // Real GIF89a header followed by HTML — must still classify as gif + // and rely on Content-Type + nosniff + CSP to neutralize the trailing bytes. + var head = [0x47, 0x49, 0x46, 0x38, 0x39, 0x61]; + var trailer = Buffer.from("", "utf8"); + var buf = Buffer.concat([Buffer.from(head), Buffer.alloc(20), trailer]); + should(imageUtils.sniffImageType(buf)).equal("image/gif"); + done(); + }); + }); + + describe("parseFeedbackLogoName", function() { + it("accepts the literal global feedback_logo", function(done) { + var p = imageUtils.parseFeedbackLogoName("feedback_logo"); + p.valid.should.be.true(); + p.isGlobal.should.be.true(); + should(p.appId).be.null(); + done(); + }); + + it("accepts feedback_logo<24-hex-app-id> as per-app", function(done) { + var appId = "0123456789abcdef01234567"; + var p = imageUtils.parseFeedbackLogoName("feedback_logo" + appId); + p.valid.should.be.true(); + p.isGlobal.should.be.false(); + p.appId.should.equal(appId); + done(); + }); + + it("rejects names with non-hex characters in the app id slot", function(done) { + var p = imageUtils.parseFeedbackLogoName("feedback_logoZZZZZZZZZZZZZZZZZZZZZZZZ"); + p.valid.should.be.false(); + done(); + }); + + it("rejects uppercase hex in the app id slot", function(done) { + // App ids are always lowercase hex; uppercase hints at attacker fuzzing + var p = imageUtils.parseFeedbackLogoName("feedback_logoABCDEF0123456789ABCDEF01"); + p.valid.should.be.false(); + done(); + }); + + it("rejects shorter or longer hex tails", function(done) { + imageUtils.parseFeedbackLogoName("feedback_logo0123").valid.should.be.false(); + imageUtils.parseFeedbackLogoName("feedback_logo0123456789abcdef0123456789").valid.should.be.false(); + done(); + }); + + it("rejects path-traversal style names", function(done) { + imageUtils.parseFeedbackLogoName("../../etc/passwd").valid.should.be.false(); + imageUtils.parseFeedbackLogoName("feedback_logo/../foo").valid.should.be.false(); + done(); + }); + + it("rejects HTML-bearing names", function(done) { + imageUtils.parseFeedbackLogoName("evil.html").valid.should.be.false(); + imageUtils.parseFeedbackLogoName("