From d93f1f23832a53b1f61171166400743f45acce17 Mon Sep 17 00:00:00 2001 From: Erica Marigold Date: Thu, 9 Jan 2025 07:25:59 +0000 Subject: [PATCH] feat: add path safety and sanitization methods to `ZipEntry` * Added `ZipEntry:getSafePath` which returns the path of the entry if it was safe, or nil * Added `ZipEntry:sanitizePath` which converts a potentially unsafe path to a safe one, although it can change the meaning of the paths * Updated path utility with functions `isSafe` and `sanitize` * Path utility now always uses `/` as a path separator, converting `\\` to `/` when needed * Included tests for path utility --- lib/init.luau | 59 +++++++++++++++++------------ lib/utils/path.luau | 92 +++++++++++++++++++++++++++++++++++++++++++-- tests/path.luau | 83 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 207 insertions(+), 27 deletions(-) create mode 100644 tests/path.luau diff --git a/lib/init.luau b/lib/init.luau index 23a5aa9..58a164d 100644 --- a/lib/init.luau +++ b/lib/init.luau @@ -46,30 +46,28 @@ local EMPTY_PROPERTIES: ZipEntryProperties = table.freeze({ }) local MADE_BY_OS_LOOKUP: { [number]: MadeByOS } = { - [0x0] = "FAT", - [0x1] = "AMIGA", - [0x2] = "VMS", - [0x3] = "UNIX", - [0x4] = "VM/CMS", - [0x5] = "Atari ST", - [0x6] = "OS/2", - [0x7] = "MAC", - [0x8] = "Z-System", - [0x9] = "CP/M", - [0xa] = "NTFS", - [0xb] = "MVS", - [0xc] = "VSE", - [0xd] = "Acorn RISCOS", - [0xe] = "VFAT", - [0xf] = "Alternate MVS", - [0x10] = "BeOS", - [0x11] = "TANDEM", - [0x12] = "OS/400", - [0x13] = "OS/X", + [0x0] = "FAT", + [0x1] = "AMIGA", + [0x2] = "VMS", + [0x3] = "UNIX", + [0x4] = "VM/CMS", + [0x5] = "Atari ST", + [0x6] = "OS/2", + [0x7] = "MAC", + [0x8] = "Z-System", + [0x9] = "CP/M", + [0xa] = "NTFS", + [0xb] = "MVS", + [0xc] = "VSE", + [0xd] = "Acorn RISCOS", + [0xe] = "VFAT", + [0xf] = "Alternate MVS", + [0x10] = "BeOS", + [0x11] = "TANDEM", + [0x12] = "OS/400", + [0x13] = "OS/X", } --- TODO: ERROR HANDLING! - local ZipEntry = {} export type ZipEntry = typeof(setmetatable({} :: ZipEntryInner, { __index = ZipEntry })) -- stylua: ignore @@ -129,7 +127,7 @@ export type ZipEntryProperties = { function ZipEntry.new(offset: number, name: string, properties: ZipEntryProperties): ZipEntry local versionMadeByOS = bit32.rshift(properties.versionMadeBy, 8) local versionMadeByVersion = bit32.band(properties.versionMadeBy, 0x00ff) - + return setmetatable( { name = name, @@ -166,6 +164,21 @@ function ZipEntry.getPath(self: ZipEntry): string return path end +function ZipEntry.getSafePath(self: ZipEntry): string? + local pathStr = self:getPath() + + if path.isSafe(pathStr) then + return pathStr + end + + return nil +end + +function ZipEntry.sanitizePath(self: ZipEntry): string + local pathStr = self:getPath() + return path.sanitize(pathStr) +end + -- TODO: More methods for `ZipEntry`, handle octals and unix perms local ZipReader = {} diff --git a/lib/utils/path.luau b/lib/utils/path.luau index bdb6b1f..030aec3 100644 --- a/lib/utils/path.luau +++ b/lib/utils/path.luau @@ -1,8 +1,7 @@ --- Canonicalize a path by removing redundant components local function canonicalize(path: string): string - -- NOTE: It is fine for us to use `/` here because ZIP file names - -- always use `/` as the path separator - local components = string.split(path, "/") + -- We convert any `\\` separators to `/` separators to ensure consistency + local components = string.split(path:gsub("\\", "/"), "/") local result = {} for _, component in components do if component == "." then @@ -25,7 +24,12 @@ end --- Check if a path is absolute local function isAbsolute(path: string): boolean - return (string.match(path, "^/") or string.match(path, "^[a-zA-Z]:[\\/]") or string.match(path, "^//")) ~= nil + return ( + string.match(path, "^/") + or string.match(path, "^[a-zA-Z]:[\\/]") + or string.match(path, "^//") + or string.match(path, "^\\\\") + ) ~= nil end --- Check if a path is relative @@ -33,8 +37,88 @@ local function isRelative(path: string): boolean return not isAbsolute(path) end +local function replaceBackslashes(input: string, replacement: "/"): string + -- Check if the string starts with double backslashes + if input:sub(1, 2) == "\\\\" then + -- Replace all single backslashes except the first two + return "\\\\" .. input:sub(3):gsub("\\", replacement) + else + -- Replace all single backslashes + return input:gsub("\\", replacement) + end +end + +--- Check if a path is safe to use, i.e., it does not: +--- - Contain null bytes +--- - Resolve to a directory outside of the current directory +--- - Have absolute components +local function isSafe(path: string): boolean + if string.find(path, "\0") or isAbsolute(path) then + -- Null bytes or absolute path, path is unsafe + return false + end + + local components = string.split(replaceBackslashes(path, "/"), "/") + local depth = 0 + for _, component in components do + if string.match(component, "^[a-zA-Z]:$") or string.find(component, "^\\\\") or component == "" then + -- Was a prefix component, or the root directory, path is unsafe + return false + end + + if component == ".." then + -- Traverse one upwards + depth -= 1 + if depth < 0 then + -- Traversed too far, path is unsafe + return false + end + + continue + end + + if component == "." then + -- Skip current directory + continue + end + + -- Otherwise, increment depth + depth += 1 + end + + return depth >= 0 +end + +--- Sanitize a path by ignoring special components +--- - Absolute paths become relative +--- - Special components (like upwards traversing) are removed +--- - Truncates path to the first null byte +local function sanitize(path: string): string + local truncatedPath = if string.find(path, "\0") then string.split(path, "\0")[1] else path + local components = string.split(replaceBackslashes(truncatedPath, "/"), "/") + local result = {} + for _, component in components do + if + not ( + component == ".." + or component == "." + or component == string.match(component, "^[a-zA-Z]:$") + or string.find(component, "^\\\\") + or component == "" + ) + then + -- If the path is not a special component, add it to the result + table.insert(result, component) + end + end + + return table.concat(result, "/") +end + return { canonicalize = canonicalize, isAbsolute = isAbsolute, isRelative = isRelative, + isSafe = isSafe, + sanitize = sanitize, } diff --git a/tests/path.luau b/tests/path.luau new file mode 100644 index 0000000..faf8644 --- /dev/null +++ b/tests/path.luau @@ -0,0 +1,83 @@ +local frktest = require("../lune_packages/frktest") +local check = frktest.assert.check + +local path = require("../lib/utils/path") + +return function(test: typeof(frktest.test)) + test.suite("Path utilities function as expected", function() + test.case("Canonicalize should handle basic paths", function() + check.equal(path.canonicalize("foo/bar"), "foo/bar") + check.equal(path.canonicalize("foo\\bar"), "foo/bar") + end) + + test.case("Canonicalize should remove current directory markers", function() + check.equal(path.canonicalize("foo/./bar"), "foo/bar") + check.equal(path.canonicalize("./foo/bar"), "foo/bar") + end) + + test.case("Canonicalize should handle parent directory traversal", function() + check.equal(path.canonicalize("foo/bar/../baz"), "foo/baz") + check.equal(path.canonicalize("foo/bar/../../baz"), "baz") + end) + + test.case("isAbsolute should identify Unix-style absolute paths", function() + check.is_true(path.isAbsolute("/foo/bar")) + check.is_true(path.isAbsolute("//foo/bar")) + end) + + test.case("isAbsolute should identify Windows-style absolute paths", function() + check.is_true(path.isAbsolute("C:\\foo\\bar")) + check.is_true(path.isAbsolute("\\\\server\\share")) + end) + + test.case("isAbsolute should identify relative paths", function() + check.is_false(path.isAbsolute("foo/bar")) + check.is_false(path.isAbsolute("./foo/bar")) + end) + + test.case("isRelative should be inverse of isAbsolute", function() + check.is_false(path.isRelative("/foo/bar")) + check.is_false(path.isRelative("C:\\foo\\bar")) + check.is_true(path.isRelative("foo/bar")) + check.is_true(path.isRelative("./foo/bar")) + end) + + test.case("isSafe should reject paths with null bytes", function() + check.is_false(path.isSafe("foo\0bar")) + end) + + test.case("isSafe should reject absolute paths", function() + check.is_false(path.isSafe("/foo/bar")) + check.is_false(path.isSafe("C:\\foo\\bar")) + end) + + test.case("isSafe should reject paths that escape current directory", function() + check.is_false(path.isSafe("../foo")) + check.is_false(path.isSafe("foo/../../bar")) + end) + + test.case("isSafe should accept safe relative paths", function() + check.is_true(path.isSafe("foo/bar")) + check.is_true(path.isSafe("foo/bar/baz")) + end) + + test.case("Sanitize should remove special components", function() + check.equal(path.sanitize("../foo/bar"), "foo/bar") + check.equal(path.sanitize("./foo/bar"), "foo/bar") + check.equal(path.sanitize("C:\\foo\\bar"), "foo/bar") + check.equal(path.sanitize("\\\\server\\share\\foo"), "share/foo") + end) + + test.case("Sanitize should truncate at null bytes", function() + check.equal(path.sanitize("foo\0bar/baz"), "foo") + end) + + test.case("Sanitize should convert backslashes to forward slashes", function() + check.equal(path.sanitize("foo\\bar\\baz"), "foo/bar/baz") + end) + + test.case("Sanitize should handle empty components", function() + check.equal(path.sanitize("/foo//bar"), "foo/bar") + end) + end) +end