diff --git a/changelog.d/storage-hardener-s1-s3-s6.changed.md b/changelog.d/storage-hardener-s1-s3-s6.changed.md new file mode 100644 index 000000000..0d060d938 --- /dev/null +++ b/changelog.d/storage-hardener-s1-s3-s6.changed.md @@ -0,0 +1,5 @@ +- LocalDisk `$resolve` throws `Wheels.Storage.InvalidKey` for empty and slash-only keys instead of concatenating onto `root` +- LocalDisk `url()` and `signedUrl()` RFC3986-encode object keys the same way S3 does (spaces and reserved characters) +- `S3Disk.delete()` HEADs first and returns false when the object is missing. An existing object still deletes and returns true +- `S3Disk.put()` sends a signed `x-amz-acl` (`public` maps to `public-read`; default and `private` send `private`) +- `signedUrl()` / `presignGetUrl()` throw `Wheels.Storage.InvalidExpiresIn` when `expiresIn` is outside `1..604800` diff --git a/changelog.d/storage-hardener-s1-s5.security.md b/changelog.d/storage-hardener-s1-s5.security.md new file mode 100644 index 000000000..4bdda2b96 --- /dev/null +++ b/changelog.d/storage-hardener-s1-s5.security.md @@ -0,0 +1,2 @@ +- LocalDisk refuses empty and slash-only keys so `put()` cannot write the disk root +- S3 `put()` honours visibility by sending a signed `x-amz-acl` header diff --git a/vendor/wheels/storage/S3Signer.cfc b/vendor/wheels/storage/S3Signer.cfc index de31eed60..3438502e4 100644 --- a/vendor/wheels/storage/S3Signer.cfc +++ b/vendor/wheels/storage/S3Signer.cfc @@ -71,6 +71,7 @@ component output="false" { string contentDisposition = "", string amzDate = "" ) { + $assertExpiresIn(arguments.expiresIn); local.amzDate = Len(arguments.amzDate) ? arguments.amzDate : $amzNow(); local.dateStamp = Left(local.amzDate, 8); local.credentialScope = local.dateStamp & "/" & variables.region & "/" & variables.service & "/aws4_request"; @@ -120,6 +121,7 @@ component output="false" { * @payload Request body (binary or string); empty for GET/DELETE/HEAD. * @amzDate Optional deterministic timestamp override. * @range Optional Range header value (e.g. "bytes=0-9"). Empty keeps the current signed header set. + * @acl Optional x-amz-acl value. Empty keeps the current signed header set (S8 vector). * @return Struct of header name => value to attach to the request. */ public struct function signedHeaders( @@ -127,7 +129,8 @@ component output="false" { required string key, any payload = "", string amzDate = "", - string range = "" + string range = "", + string acl = "" ) { local.amzDate = Len(arguments.amzDate) ? arguments.amzDate : $amzNow(); local.dateStamp = Left(local.amzDate, 8); @@ -139,19 +142,21 @@ component output="false" { ? "/" & $uriEncodePath(variables.bucket & "/" & arguments.key) : "/" & $uriEncodePath(arguments.key); - // Headers signed for header-auth: host, x-amz-content-sha256, x-amz-date (sorted). - local.canonicalHeaders = "host:" & variables.host & Chr(10) - & "x-amz-content-sha256:" & local.payloadHash & Chr(10) - & "x-amz-date:" & local.amzDate & Chr(10); - local.signedHeaderList = "host;x-amz-content-sha256;x-amz-date"; - + // Optional range / acl insert in code-point order so the S8 no-acl vector + // stays byte-identical when both are empty. + local.canonicalHeaders = "host:" & variables.host & Chr(10); + local.signedHeaderList = "host"; if (Len(arguments.range)) { - local.canonicalHeaders = "host:" & variables.host & Chr(10) - & "range:" & arguments.range & Chr(10) - & "x-amz-content-sha256:" & local.payloadHash & Chr(10) - & "x-amz-date:" & local.amzDate & Chr(10); - local.signedHeaderList = "host;range;x-amz-content-sha256;x-amz-date"; + local.canonicalHeaders &= "range:" & arguments.range & Chr(10); + local.signedHeaderList &= ";range"; + } + if (Len(arguments.acl)) { + local.canonicalHeaders &= "x-amz-acl:" & arguments.acl & Chr(10); + local.signedHeaderList &= ";x-amz-acl"; } + local.canonicalHeaders &= "x-amz-content-sha256:" & local.payloadHash & Chr(10) + & "x-amz-date:" & local.amzDate & Chr(10); + local.signedHeaderList &= ";x-amz-content-sha256;x-amz-date"; local.canonicalRequest = UCase(arguments.method) & Chr(10) & local.canonicalUri & Chr(10) @@ -167,21 +172,19 @@ component output="false" { & "SignedHeaders=" & local.signedHeaderList & ", " & "Signature=" & local.signature; - if (Len(arguments.range)) { - return { - "Authorization" = local.authorization, - "x-amz-content-sha256" = local.payloadHash, - "x-amz-date" = local.amzDate, - "Host" = variables.host, - "Range" = arguments.range - }; - } - return { + local.headers = { "Authorization" = local.authorization, "x-amz-content-sha256" = local.payloadHash, "x-amz-date" = local.amzDate, "Host" = variables.host }; + if (Len(arguments.range)) { + local.headers["Range"] = arguments.range; + } + if (Len(arguments.acl)) { + local.headers["x-amz-acl"] = arguments.acl; + } + return local.headers; } /** @@ -293,6 +296,15 @@ component output="false" { return Replace($uriEncodeSegment(arguments.key), "%2F", "/", "all"); } + private void function $assertExpiresIn(required numeric expiresIn) { + if (arguments.expiresIn < 1 || arguments.expiresIn > 604800) { + throw( + type = "Wheels.Storage.InvalidExpiresIn", + message = "signedUrl expiresIn must be between 1 and 604800 seconds (got #arguments.expiresIn#)." + ); + } + } + /** * Current UTC time as an ISO8601 basic timestamp ("yyyymmddTHHnnssZ"). */ diff --git a/vendor/wheels/storage/drivers/LocalDisk.cfc b/vendor/wheels/storage/drivers/LocalDisk.cfc index 62edbb3de..489bd8cc2 100644 --- a/vendor/wheels/storage/drivers/LocalDisk.cfc +++ b/vendor/wheels/storage/drivers/LocalDisk.cfc @@ -73,6 +73,7 @@ component implements="wheels.interfaces.StorageDiskInterface" output="false" { } public string function signedUrl(required string key, numeric expiresIn = 300, string contentDisposition = "") { + $assertExpiresIn(arguments.expiresIn); if (!Len(variables.signingKey)) { throw( type = "Wheels.Storage.MissingSigningKey", @@ -157,6 +158,14 @@ component implements="wheels.interfaces.StorageDiskInterface" output="false" { message = "Storage key [#arguments.key#] must not contain '..'." ); } + // Empty or slash-only keys resolve to root itself. Throw rather than + // let put() write the disk root or get() treat the directory as NotFound. + if (!Len(REReplace(local.clean, "/", "", "all"))) { + throw( + type = "Wheels.Storage.InvalidKey", + message = "Storage key [#arguments.key#] must not be empty or slash-only." + ); + } return variables.root & "/" & local.clean; } @@ -178,12 +187,28 @@ component implements="wheels.interfaces.StorageDiskInterface" output="false" { private string function $joinUrl(required string prefix, required string key) { local.p = REReplace(arguments.prefix, "/+$", ""); local.k = REReplace(arguments.key, "^/+", ""); - return local.p & "/" & local.k; + return local.p & "/" & $uriEncodePath(local.k); } private string function $uriEncode(required string value) { - local.encoded = CreateObject("java", "java.net.URLEncoder").encode(arguments.value, "UTF-8"); - return Replace(local.encoded, "+", "%20", "all"); + local.encoded = CreateObject("java", "java.net.URLEncoder").encode(ToString(arguments.value), "UTF-8"); + local.encoded = Replace(local.encoded, "+", "%20", "all"); + local.encoded = Replace(local.encoded, "*", "%2A", "all"); + local.encoded = Replace(local.encoded, "%7E", "~", "all"); + return local.encoded; + } + + private string function $uriEncodePath(required string key) { + return Replace($uriEncode(arguments.key), "%2F", "/", "all"); + } + + private void function $assertExpiresIn(required numeric expiresIn) { + if (arguments.expiresIn < 1 || arguments.expiresIn > 604800) { + throw( + type = "Wheels.Storage.InvalidExpiresIn", + message = "signedUrl expiresIn must be between 1 and 604800 seconds (got #arguments.expiresIn#)." + ); + } } private numeric function $epochSeconds() { diff --git a/vendor/wheels/storage/drivers/S3Disk.cfc b/vendor/wheels/storage/drivers/S3Disk.cfc index 54c19cf63..af910d3bc 100644 --- a/vendor/wheels/storage/drivers/S3Disk.cfc +++ b/vendor/wheels/storage/drivers/S3Disk.cfc @@ -42,7 +42,14 @@ component implements="wheels.interfaces.StorageDiskInterface" output="false" { } public any function put(required string key, required any content, string contentType = "application/octet-stream", string visibility = "") { - local.headers = variables.signer.signedHeaders(method = "PUT", key = arguments.key, payload = arguments.content); + local.visibility = Len(arguments.visibility) ? arguments.visibility : variables.visibility; + local.acl = $aclFromVisibility(local.visibility); + local.headers = variables.signer.signedHeaders( + method = "PUT", + key = arguments.key, + payload = arguments.content, + acl = local.acl + ); local.result = $request(method = "PUT", key = arguments.key, headers = local.headers, body = arguments.content, contentType = arguments.contentType); $assertSuccess(result = local.result, method = "PUT", key = arguments.key); return arguments.key; @@ -77,10 +84,11 @@ component implements="wheels.interfaces.StorageDiskInterface" output="false" { } public boolean function delete(required string key) { + if (!exists(arguments.key)) { + return false; + } local.headers = variables.signer.signedHeaders(method = "DELETE", key = arguments.key); local.result = $request(method = "DELETE", key = arguments.key, headers = local.headers); - // S3 DELETE is idempotent — 2xx whether or not the object existed — but a - // connection failure or 5xx must not masquerade as a successful delete. $assertSuccess(result = local.result, method = "DELETE", key = arguments.key); return true; } @@ -99,6 +107,17 @@ component implements="wheels.interfaces.StorageDiskInterface" output="false" { // ---- internals -------------------------------------------------------- + private string function $aclFromVisibility(required string visibility) { + local.v = LCase(Trim(arguments.visibility)); + if (local.v == "public") { + return "public-read"; + } + if (local.v == "private" || !Len(local.v)) { + return "private"; + } + return arguments.visibility; + } + private string function $objectPath(required string key) { local.k = REReplace(arguments.key, "^/+", ""); // Encode through the signer so the wire path is byte-identical to the diff --git a/vendor/wheels/tests/_assets/storage/S3DiskDeleteStub.cfc b/vendor/wheels/tests/_assets/storage/S3DiskDeleteStub.cfc index abb91d475..b30883da3 100644 --- a/vendor/wheels/tests/_assets/storage/S3DiskDeleteStub.cfc +++ b/vendor/wheels/tests/_assets/storage/S3DiskDeleteStub.cfc @@ -1,5 +1,16 @@ component extends="wheels.storage.drivers.S3Disk" { + variables.objects = {}; + variables.lastRequest = {}; + + public void function seed(required string key) { + variables.objects[arguments.key] = true; + } + + public struct function lastRequest() { + return variables.lastRequest; + } + public struct function $request( required string method, required string key, @@ -8,7 +19,32 @@ component extends="wheels.storage.drivers.S3Disk" { string contentType = "", boolean getAsBinary = false ) { - return {statusCode = "204 No Content", fileContent = ""}; + local.copiedHeaders = {}; + for (local.name in arguments.headers) { + local.copiedHeaders[local.name] = arguments.headers[local.name]; + } + variables.lastRequest = { + method = arguments.method, + key = arguments.key, + headers = local.copiedHeaders, + contentType = arguments.contentType + }; + + if (arguments.method == "HEAD") { + if (StructKeyExists(variables.objects, arguments.key)) { + return {statusCode = "200 OK", fileContent = ""}; + } + return {statusCode = "404 Not Found", fileContent = ""}; + } + if (arguments.method == "DELETE") { + StructDelete(variables.objects, arguments.key); + return {statusCode = "204 No Content", fileContent = ""}; + } + if (arguments.method == "PUT") { + variables.objects[arguments.key] = true; + return {statusCode = "200 OK", fileContent = ""}; + } + return {statusCode = "200 OK", fileContent = ""}; } } diff --git a/vendor/wheels/tests/specs/storage/StorageSpec.cfc b/vendor/wheels/tests/specs/storage/StorageSpec.cfc index 34123e6b4..10f577156 100644 --- a/vendor/wheels/tests/specs/storage/StorageSpec.cfc +++ b/vendor/wheels/tests/specs/storage/StorageSpec.cfc @@ -70,6 +70,26 @@ component extends="wheels.WheelsTest" { expect(headers.Authorization).toInclude("Signature="); }); + it("includes x-amz-acl in the signed header set when acl is passed", function() { + var signer = new wheels.storage.S3Signer( + accessKeyId = "AKIAIOSFODNN7EXAMPLE", + secretAccessKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + region = "us-east-1", + bucket = "examplebucket" + ); + var headers = signer.signedHeaders( + method = "PUT", + key = "a.txt", + payload = "x", + amzDate = "20130524T000000Z", + acl = "public-read" + ); + + expect(headers).toHaveKey("x-amz-acl"); + expect(headers["x-amz-acl"]).toBe("public-read"); + expect(headers.Authorization).toInclude("SignedHeaders=host;x-amz-acl;x-amz-content-sha256;x-amz-date"); + }); + it("reproduces the AWS-documented SigV4 GET Object header-auth test vector", function() { var signer = new wheels.storage.S3Signer( accessKeyId = "AKIAIOSFODNN7EXAMPLE", @@ -143,21 +163,26 @@ component extends="wheels.WheelsTest" { }).toThrow("Wheels.Storage.InvalidKey"); }); - it("resolves empty and slash-only keys as root concatenations, not InvalidKey", function() { - var expectedRoot = REReplace(Replace(ctx.root, "\", "/", "all"), "/+$", ""); - expect(disk.$resolve("")).toBe(expectedRoot & "/"); - expect(disk.$resolve("/")).toBe(expectedRoot & "//"); - expect(disk.$resolve("///")).toBe(expectedRoot & "////"); + it("throws InvalidKey for empty and slash-only keys", function() { + expect(function() { + disk.$resolve(""); + }).toThrow("Wheels.Storage.InvalidKey"); + expect(function() { + disk.$resolve("/"); + }).toThrow("Wheels.Storage.InvalidKey"); + expect(function() { + disk.$resolve("///"); + }).toThrow("Wheels.Storage.InvalidKey"); expect(function() { disk.get(""); - }).toThrow("Wheels.Storage.NotFound"); + }).toThrow("Wheels.Storage.InvalidKey"); expect(function() { - disk.get("/"); - }).toThrow("Wheels.Storage.NotFound"); + disk.put(key = "", content = "x"); + }).toThrow("Wheels.Storage.InvalidKey"); expect(function() { - disk.get("///"); - }).toThrow("Wheels.Storage.NotFound"); + disk.put(key = "/", content = "x"); + }).toThrow("Wheels.Storage.InvalidKey"); }); it("follows a symlink under root to a file outside (Find('..') does not see the hop)", function() { @@ -186,6 +211,20 @@ component extends="wheels.WheelsTest" { expect(disk.url("a/b.png")).toBe("/uploads/a/b.png"); }); + it("rfc3986-encodes spaces and reserved characters in url() and signedUrl()", function() { + expect(disk.url("my docs/q3 report.pdf")).toBe("/uploads/my%20docs/q3%20report.pdf"); + expect(disk.url("a+b*.txt")).toBe("/uploads/a%2Bb%2A.txt"); + + var signed = disk.signedUrl(key = "my docs/q3 report.pdf", expiresIn = 600); + expect(signed).toInclude("/uploads/my%20docs/q3%20report.pdf?"); + expect(signed).toInclude("signature="); + + var qs = ListLast(signed, "?"); + var sig = ReReplace(qs, ".*signature=([a-f0-9]+).*", "\1"); + var exp = Val(ReReplace(qs, ".*expires=([0-9]+).*", "\1")); + expect(disk.verifySignature(key = "my docs/q3 report.pdf", expires = exp, signature = sig)).toBeTrue(); + }); + it("builds a signed url whose token round-trips through verifySignature", function() { var signed = disk.signedUrl(key = "a/b.png", expiresIn = 600); expect(signed).toInclude("/uploads/a/b.png?expires="); @@ -202,13 +241,30 @@ component extends="wheels.WheelsTest" { }); it("rejects an expired signed url", function() { - var signed = disk.signedUrl(key = "a/b.png", expiresIn = -10); - var qs = ListLast(signed, "?"); - var sig = ReReplace(qs, ".*signature=([a-f0-9]+).*", "\1"); - var exp = Val(ReReplace(qs, ".*expires=([0-9]+).*", "\1")); + var exp = 1; + var sig = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; expect(disk.verifySignature(key = "a/b.png", expires = exp, signature = sig)).toBeFalse(); }); + it("throws InvalidExpiresIn for non-positive or over-max expiresIn", function() { + expect(function() { + disk.signedUrl(key = "a/b.png", expiresIn = 0); + }).toThrow("Wheels.Storage.InvalidExpiresIn"); + expect(function() { + disk.signedUrl(key = "a/b.png", expiresIn = -10); + }).toThrow("Wheels.Storage.InvalidExpiresIn"); + expect(function() { + disk.signedUrl(key = "a/b.png", expiresIn = 604801); + }).toThrow("Wheels.Storage.InvalidExpiresIn"); + }); + + it("accepts expiresIn at the documented bounds", function() { + var minSigned = disk.signedUrl(key = "a/b.png", expiresIn = 1); + expect(minSigned).toInclude("expires="); + var maxSigned = disk.signedUrl(key = "a/b.png", expiresIn = 604800); + expect(maxSigned).toInclude("expires="); + }); + it("binds contentDisposition into the signed-url token", function() { var signed = disk.signedUrl(key = "a/b.png", expiresIn = 600, contentDisposition = "attachment; filename=safe.png"); var qs = ListLast(signed, "?"); @@ -284,6 +340,25 @@ component extends="wheels.WheelsTest" { expect(presigned).toInclude("X-Amz-Signature="); }); + it("throws InvalidExpiresIn for non-positive or over-max expiresIn", function() { + expect(function() { + s3.signedUrl(key = "avatars/1.png", expiresIn = 0); + }).toThrow("Wheels.Storage.InvalidExpiresIn"); + expect(function() { + s3.signedUrl(key = "avatars/1.png", expiresIn = -10); + }).toThrow("Wheels.Storage.InvalidExpiresIn"); + expect(function() { + s3.signedUrl(key = "avatars/1.png", expiresIn = 604801); + }).toThrow("Wheels.Storage.InvalidExpiresIn"); + }); + + it("accepts expiresIn at the documented SigV4 bounds", function() { + var minSigned = s3.signedUrl(key = "avatars/1.png", expiresIn = 1); + expect(minSigned).toInclude("X-Amz-Expires=1"); + var maxSigned = s3.signedUrl(key = "avatars/1.png", expiresIn = 604800); + expect(maxSigned).toInclude("X-Amz-Expires=604800"); + }); + it("requires bucket/region/credentials", function() { expect(function() { new wheels.storage.drivers.S3Disk(config = {bucket = "b", region = "us-east-1"}); @@ -340,15 +415,47 @@ component extends="wheels.WheelsTest" { describe("S3Disk delete()", function() { - it("returns true after a 2xx, including a second delete of the same missing key", function() { + it("HEADs first and returns false when the object is missing", function() { + var stubDisk = new wheels.tests._assets.storage.S3DiskDeleteStub(config = { + bucket = "myapp", + region = "us-east-1", + accessKeyId = "AKIAIOSFODNN7EXAMPLE", + secretAccessKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + }); + expect(stubDisk.delete("gone.txt")).toBeFalse(); + expect(stubDisk.lastRequest().method).toBe("HEAD"); + }); + + it("deletes an existing object and returns true", function() { var stubDisk = new wheels.tests._assets.storage.S3DiskDeleteStub(config = { bucket = "myapp", region = "us-east-1", accessKeyId = "AKIAIOSFODNN7EXAMPLE", secretAccessKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" }); - expect(stubDisk.delete("gone.txt")).toBeTrue(); - expect(stubDisk.delete("gone.txt")).toBeTrue(); + stubDisk.seed("here.txt"); + expect(stubDisk.delete("here.txt")).toBeTrue(); + expect(stubDisk.lastRequest().method).toBe("DELETE"); + expect(stubDisk.delete("here.txt")).toBeFalse(); + }); + + }); + + describe("S3Disk put() ACL", function() { + + it("sends x-amz-acl private by default and public-read for visibility=public", function() { + var stubDisk = new wheels.tests._assets.storage.S3DiskDeleteStub(config = { + bucket = "myapp", + region = "us-east-1", + accessKeyId = "AKIAIOSFODNN7EXAMPLE", + secretAccessKey = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + }); + stubDisk.put(key = "a.txt", content = "x"); + expect(stubDisk.lastRequest().headers["x-amz-acl"]).toBe("private"); + + stubDisk.put(key = "b.txt", content = "x", visibility = "public"); + expect(stubDisk.lastRequest().headers["x-amz-acl"]).toBe("public-read"); + expect(stubDisk.lastRequest().headers.Authorization).toInclude("SignedHeaders=host;x-amz-acl;x-amz-content-sha256;x-amz-date"); }); });