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

@@ -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(""));

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<TValue>(
+const notEmpty = <TValue>(
   value: TValue | void | null | undefined
-): value is TValue {
+): value is TValue => {
   return value !== null && value !== undefined;
-}
+};
 
-export const getImages =
+const getRequestedPath = (req: express.Request) =>
-  (imagesPath: string) =>
+  req.params[1] === undefined || req.params[1] === "/" ? "" : req.params[1];
-  async (req: express.Request, res: express.Response) => {
-    const requestedPath = decodeURI(req.path.substring(imagesPath.length));
-    const normalizedPath = requestedPath === "/" ? "" : requestedPath;
 
-    try {
+const readThumbnails = (requestedPath: string) => {
-      const dirents = fs.readdirSync(publicPath + requestedPath, {
+  const requestedThumbnailPath = path.posix.join(
-        withFileTypes: true,
+    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<Image>({
+    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 images = (await Promise.all(imagesToBeLoaded)).filter(notEmpty);
-
+    res.json(a<Folder>({ images }));
-      const imagesToBeLoaded = dirents
+  } catch (e) {
-        .filter((f) => f.isFile())
+    consoleLogger.warn(`Error when trying to access ${req.path}: ${e}`);
-        .map((f) => {
+    res.status(400).json({ message: `Path ${req.path} not accessible.` });
-          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<Image>({
-                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<Folder>({ images }));
-    } catch (e) {
-      consoleLogger.warn(`Error when trying to access ${req.path}: ${e}`);
-      res.status(400).json({ message: `Path ${req.path} not accessible.` });
-    }
-  };

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."`
+    );
+  }
+};

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 });
+  const dirEnts = fs.readdirSync(path.posix.join(publicPath, dirPath), {
-  fs.mkdirSync(thumbnailPublicPath + dirPath, { recursive: true });
+    withFileTypes: true,
-  const thumbnails = fs.readdirSync(thumbnailPublicPath + dirPath);
+  });
+  fs.mkdirSync(path.posix.join(thumbnailPublicPath, dirPath), {
+    recursive: true,
+  });
+  const thumbnails = fs.readdirSync(
+    path.posix.join(thumbnailPublicPath, dirPath)
+  );
 
   dirEnts
     .filter((f) => f.isFile())