From 53e0e9bd1ffb60eddaafb0f545e8342569481aff Mon Sep 17 00:00:00 2001 From: Stefan Forstenlechner Date: Fri, 29 Apr 2022 21:14:38 +0200 Subject: [PATCH] Add security check to avoid access violations In case of malicious users double check paths, so that they are not able to traverse outside of the image directory. --- picture-gallery-server/src/app.ts | 2 +- .../src/controller/images.ts | 126 +++++++++++------- .../src/controller/securityChecks.ts | 26 ++++ picture-gallery-server/src/thumbnails.ts | 23 ++-- 4 files changed, 122 insertions(+), 55 deletions(-) create mode 100644 picture-gallery-server/src/controller/securityChecks.ts diff --git a/picture-gallery-server/src/app.ts b/picture-gallery-server/src/app.ts index e92ae1d..13053dc 100644 --- a/picture-gallery-server/src/app.ts +++ b/picture-gallery-server/src/app.ts @@ -28,7 +28,7 @@ app.use(expressLogger); const imagesPath = "/images"; -app.get(`${imagesPath}(/*)?`, getImages(imagesPath)); +app.get(`${imagesPath}(/*)?`, getImages); app.get("/directories", async (req, res) => { res.json(await walk("")); diff --git a/picture-gallery-server/src/controller/images.ts b/picture-gallery-server/src/controller/images.ts index 7602f98..a8793d1 100644 --- a/picture-gallery-server/src/controller/images.ts +++ b/picture-gallery-server/src/controller/images.ts @@ -1,58 +1,92 @@ -import fs from "fs"; +import fs, { Dirent } from "fs"; import express from "express"; import sharp from "sharp"; +import path from "path"; import { publicPath, thumbnailPath, thumbnailPublicPath } from "../paths"; import { a, Folder, Image } from "../models"; import { createThumbnailAsyncForImage } from "../thumbnails"; import { consoleLogger } from "../logging"; +import { securityValidation } from "./securityChecks"; -function notEmpty( +const notEmpty = ( value: TValue | void | null | undefined -): value is TValue { +): value is TValue => { return value !== null && value !== undefined; -} +}; -export const getImages = - (imagesPath: string) => - async (req: express.Request, res: express.Response) => { - const requestedPath = decodeURI(req.path.substring(imagesPath.length)); - const normalizedPath = requestedPath === "/" ? "" : requestedPath; +const getRequestedPath = (req: express.Request) => + req.params[1] === undefined || req.params[1] === "/" ? "" : req.params[1]; - try { - const dirents = fs.readdirSync(publicPath + requestedPath, { - withFileTypes: true, +const readThumbnails = (requestedPath: string) => { + const requestedThumbnailPath = path.posix.join( + thumbnailPublicPath, + requestedPath + ); + return fs.existsSync(requestedThumbnailPath) + ? fs.readdirSync(requestedThumbnailPath) + : []; +}; + +const getSrc = (thumbnailExists: boolean, requestedPath: string, f: Dirent) => + thumbnailExists + ? path.posix.join("/staticImages", thumbnailPath, requestedPath, f.name) + : path.posix.join("/staticImages", requestedPath, f.name); + +const toImage = ( + metadata: sharp.Metadata, + thumbnailExists: boolean, + requestedPath: string, + f: Dirent +) => { + const widthAndHeightSwap = metadata.orientation > 4; // see https://exiftool.org/TagNames/EXIF.html + return a({ + src: getSrc(thumbnailExists, requestedPath, f), + width: widthAndHeightSwap ? metadata.height : metadata.width, + height: widthAndHeightSwap ? metadata.width : metadata.height, + }); +}; + +export const getImages = async ( + req: express.Request, + res: express.Response +) => { + const requestedPath = getRequestedPath(req); + + try { + securityValidation(requestedPath); + + const dirents = fs.readdirSync(path.posix.join(publicPath, requestedPath), { + withFileTypes: true, + }); + + const thumbnails = readThumbnails(requestedPath); + + const imagesToBeLoaded = dirents + .filter((f) => f.isFile()) + .map((f) => { + const thumbnailExists: boolean = thumbnails.includes(f.name); + if (!thumbnailExists) { + createThumbnailAsyncForImage(path.posix.join(requestedPath, f.name)); + } + return sharp(path.posix.join(publicPath, requestedPath, f.name)) + .metadata() + .then((metadata) => + toImage(metadata, thumbnailExists, requestedPath, f) + ) + .catch((err) => { + consoleLogger.error( + `Reading metadata from ${path.posix.join( + publicPath, + requestedPath, + f.name + )} produced the following error: ${err.message}` + ); + }); }); - const thumbnails = fs.readdirSync(thumbnailPublicPath + requestedPath); - - const imagesToBeLoaded = dirents - .filter((f) => f.isFile()) - .map((f) => { - const thumbnailExists: boolean = thumbnails.includes(f.name); - if (!thumbnailExists) { - createThumbnailAsyncForImage(`${requestedPath}/${f.name}`); - } - return sharp(`${publicPath}${requestedPath}/${f.name}`) - .metadata() - .then((metadata) => { - const widthAndHeightSwap = metadata.orientation > 4; // see https://exiftool.org/TagNames/EXIF.html - return a({ - src: thumbnailExists - ? `/staticImages${thumbnailPath}${normalizedPath}/${f.name}` - : `/staticImages${normalizedPath}/${f.name}`, - width: widthAndHeightSwap ? metadata.height : metadata.width, - height: widthAndHeightSwap ? metadata.width : metadata.height, - }); - }) - .catch((err) => { - consoleLogger.error( - `Reading metadata from ${publicPath}${requestedPath}/${f.name} produced the following error: ${err.message}` - ); - }); - }); - const images = (await Promise.all(imagesToBeLoaded)).filter(notEmpty); - res.json(a({ images })); - } catch (e) { - consoleLogger.warn(`Error when trying to access ${req.path}: ${e}`); - res.status(400).json({ message: `Path ${req.path} not accessible.` }); - } - }; + const images = (await Promise.all(imagesToBeLoaded)).filter(notEmpty); + res.json(a({ images })); + } catch (e) { + consoleLogger.warn(`Error when trying to access ${req.path}: ${e}`); + res.status(400).json({ message: `Path ${req.path} not accessible.` }); + } +}; diff --git a/picture-gallery-server/src/controller/securityChecks.ts b/picture-gallery-server/src/controller/securityChecks.ts new file mode 100644 index 0000000..bba028a --- /dev/null +++ b/picture-gallery-server/src/controller/securityChecks.ts @@ -0,0 +1,26 @@ +import path from "path"; +import { consoleLogger } from "../logging"; + +// in case the requested path starts with ".." and tries to traverse outside of the image directory +const dirToCheckAccessViolation = "anyDir"; + +export const securityValidation = (requestedPath: string) => { + // use normalize as well in case `join` would not normalize + const validatePath = path.posix.normalize( + path.posix.join(dirToCheckAccessViolation, requestedPath) + ); + // do not use `join` is join also normalizes the path + const requestedPathWithNewRoot = + requestedPath === "" + ? dirToCheckAccessViolation + : `${dirToCheckAccessViolation}/${requestedPath}`; + + if (validatePath !== requestedPathWithNewRoot) { + consoleLogger.error( + `Security violation. Requested path "${requestedPath} did not match normalized path."` + ); + throw new Error( + `Security violation. Requested path "${requestedPath} did not match normalized path."` + ); + } +}; diff --git a/picture-gallery-server/src/thumbnails.ts b/picture-gallery-server/src/thumbnails.ts index 687c07a..ceb210e 100644 --- a/picture-gallery-server/src/thumbnails.ts +++ b/picture-gallery-server/src/thumbnails.ts @@ -8,7 +8,8 @@ const percentage = 25; const minimumPixelForThumbnail = 1024; export const createThumbnailAsyncForImage = (image: string) => { - sharp(`${publicPath}${image}`) + const publicImagePath = path.posix.join(publicPath, image); + sharp(publicImagePath) .metadata() .then((info) => { const width = Math.max( @@ -21,19 +22,19 @@ export const createThumbnailAsyncForImage = (image: string) => { ); fs.mkdir( - thumbnailPublicPath + path.dirname(image), + path.posix.join(thumbnailPublicPath, path.dirname(image)), { recursive: true }, () => { - sharp(`${publicPath}${image}`) + sharp(publicImagePath) .withMetadata() .resize(info.width > info.height ? { width } : { height }) - .toFile(`${thumbnailPublicPath}${image}`); + .toFile(`${path.posix.join(thumbnailPublicPath, image)}`); } ); }) .catch((err) => { consoleLogger.error( - `Thumbnail creation of ${publicPath}${image} produced the following error: ${err.message}` + `Thumbnail creation of ${publicImagePath} produced the following error: ${err.message}` ); }); }; @@ -42,9 +43,15 @@ export const initThumbnailsAsync = (dirPath: string) => { if (dirPath.includes(thumbnailPath)) { return; } - const dirEnts = fs.readdirSync(publicPath + dirPath, { withFileTypes: true }); - fs.mkdirSync(thumbnailPublicPath + dirPath, { recursive: true }); - const thumbnails = fs.readdirSync(thumbnailPublicPath + dirPath); + const dirEnts = fs.readdirSync(path.posix.join(publicPath, dirPath), { + withFileTypes: true, + }); + fs.mkdirSync(path.posix.join(thumbnailPublicPath, dirPath), { + recursive: true, + }); + const thumbnails = fs.readdirSync( + path.posix.join(thumbnailPublicPath, dirPath) + ); dirEnts .filter((f) => f.isFile())