From 6cb2f3cc4aedf2ba9e000d4899a6f08203a8e839 Mon Sep 17 00:00:00 2001 From: RanVaknin Date: Fri, 8 Mar 2024 17:05:49 +0000 Subject: [PATCH 1/3] fix(middleware-ssec): ssecMiddleware with strict base64 validation --- packages/middleware-ssec/src/index.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/middleware-ssec/src/index.ts b/packages/middleware-ssec/src/index.ts index 330e2abae535..fe0771565d42 100644 --- a/packages/middleware-ssec/src/index.ts +++ b/packages/middleware-ssec/src/index.ts @@ -39,8 +39,7 @@ export function ssecMiddleware(options: PreviouslyResolved): InitializeMiddlewar if (value) { let valueForHash: Uint8Array; if (typeof value === "string") { - const isBase64Encoded = /^(?:[A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/.test(value); - if (isBase64Encoded) { + if (isValidBase64(value)) { valueForHash = options.base64Decoder(value); } else { valueForHash = options.utf8Decoder(value); @@ -78,3 +77,19 @@ export const getSsecPlugin = (config: PreviouslyResolved): Pluggable = clientStack.add(ssecMiddleware(config), ssecMiddlewareOptions); }, }); + +function isValidBase64(str: string) { + const base64Regex = /^(?:[A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/; + + if (!base64Regex.test(str)) return false; + + try { + const decodedBytes = Buffer.from(str, "base64"); + if (decodedBytes.length !== 32) return false; + + const reEncoded = decodedBytes.toString("base64"); + return str === reEncoded; + } catch { + return false; + } +} From f1feed3ccf006022fe22fb54167f77753a148763 Mon Sep 17 00:00:00 2001 From: RanVaknin Date: Tue, 12 Mar 2024 18:16:37 +0000 Subject: [PATCH 2/3] fix(middleware-ssec): ssecMiddleware with strict base64 validation, using serde decoder --- packages/middleware-ssec/src/index.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/middleware-ssec/src/index.ts b/packages/middleware-ssec/src/index.ts index fe0771565d42..d11f9db45d74 100644 --- a/packages/middleware-ssec/src/index.ts +++ b/packages/middleware-ssec/src/index.ts @@ -39,7 +39,7 @@ export function ssecMiddleware(options: PreviouslyResolved): InitializeMiddlewar if (value) { let valueForHash: Uint8Array; if (typeof value === "string") { - if (isValidBase64(value)) { + if (isValidBase64EncodedSSECustomerKey(value, options)) { valueForHash = options.base64Decoder(value); } else { valueForHash = options.utf8Decoder(value); @@ -78,17 +78,14 @@ export const getSsecPlugin = (config: PreviouslyResolved): Pluggable = }, }); -function isValidBase64(str: string) { +function isValidBase64EncodedSSECustomerKey(str: string, options: PreviouslyResolved) { const base64Regex = /^(?:[A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/; if (!base64Regex.test(str)) return false; try { - const decodedBytes = Buffer.from(str, "base64"); + const decodedBytes = options.base64Decoder(str); if (decodedBytes.length !== 32) return false; - - const reEncoded = decodedBytes.toString("base64"); - return str === reEncoded; } catch { return false; } From ec42c4150d358a7258311c542535270955e10290 Mon Sep 17 00:00:00 2001 From: RanVaknin Date: Thu, 14 Mar 2024 02:49:33 +0000 Subject: [PATCH 3/3] fix(middleware-ssec): ssecMiddleware with strict base64 validation with tests --- packages/middleware-ssec/src/index.spec.ts | 162 ++++++++++++++++----- packages/middleware-ssec/src/index.ts | 4 +- 2 files changed, 129 insertions(+), 37 deletions(-) diff --git a/packages/middleware-ssec/src/index.spec.ts b/packages/middleware-ssec/src/index.spec.ts index 167442c367d8..9f57558b236b 100644 --- a/packages/middleware-ssec/src/index.spec.ts +++ b/packages/middleware-ssec/src/index.spec.ts @@ -1,7 +1,7 @@ import { ChecksumConstructor } from "@smithy/types"; import * as crypto from "crypto"; -import { ssecMiddleware } from "./"; +import { isValidBase64EncodedSSECustomerKey, ssecMiddleware } from "./"; describe("ssecMiddleware", () => { const next = jest.fn(); @@ -28,22 +28,16 @@ describe("ssecMiddleware", () => { }); it("should base64 encode input keys and set respective MD5 inputs", async () => { - encoder1.mockReturnValue("/+JF8FMG8UVMWSaNz0s6Wg=="); - const key = "TestKey123"; - const binaryRepresentationOfKey = Buffer.from(key); - const base64Key = binaryRepresentationOfKey.toString("base64"); - const md5Hash = crypto.createHash("md5").update(binaryRepresentationOfKey).digest(); - const base64Md5Hash = Buffer.from(md5Hash).toString("base64"); - + encoder2.mockReturnValue("base64"); const args = { input: { - SSECustomerKey: base64Key, - CopySourceSSECustomerKey: base64Key, + SSECustomerKey: "foo", + CopySourceSSECustomerKey: "bar", }, }; const handler = ssecMiddleware({ - base64Encoder: encoder1, + base64Encoder: encoder2, utf8Decoder: decoder, md5: MockHash, base64Decoder: base64Decoder, @@ -54,49 +48,147 @@ describe("ssecMiddleware", () => { expect(next.mock.calls.length).toBe(1); expect(next).toHaveBeenCalledWith({ input: { - SSECustomerKey: base64Key, - SSECustomerKeyMD5: base64Md5Hash, - CopySourceSSECustomerKey: base64Key, - CopySourceSSECustomerKeyMD5: base64Md5Hash, + SSECustomerKey: "base64", + SSECustomerKeyMD5: "base64", + CopySourceSSECustomerKey: "base64", + CopySourceSSECustomerKeyMD5: "base64", }, }); - expect(decoder.mock.calls.length).toBe(0); - expect(encoder1.mock.calls.length).toBe(2); + expect(decoder.mock.calls.length).toBe(2); + expect(encoder2.mock.calls.length).toBe(4); expect(mockHashUpdate.mock.calls.length).toBe(2); expect(mockHashDigest.mock.calls.length).toBe(2); - encoder1.mockClear(); + encoder2.mockClear(); }); - it("should base64 encode input keys and set respective MD5 inputs", async () => { - encoder2.mockReturnValue("base64"); + + it("should handle valid base64 encoded 32byte key input for SSECustomerKey and CopySourceSSECustomerKey", async () => { + const validBase64EncodedKey = "QUIwMTIzNDU2Nzg5QUJDREVGQUJDREVGQUJDREVGQUI="; + const decodedBytes = Buffer.from(validBase64EncodedKey, "base64"); + const md5Hash = crypto.createHash("md5").update(decodedBytes).digest(); + const base64EncodedMD5Hash = Buffer.from(md5Hash).toString("base64"); + + base64Decoder.mockReturnValue(decodedBytes); + const mockMD5Instance = { + update: jest.fn().mockReturnThis(), + digest: jest.fn().mockReturnValue(md5Hash), + }; + const mockMD5Constructor = jest.fn().mockReturnValue(mockMD5Instance); + const base64Encoder = jest.fn().mockReturnValue(base64EncodedMD5Hash); + + const handler = ssecMiddleware({ + base64Encoder, + utf8Decoder: decoder, + md5: mockMD5Constructor, + base64Decoder, + })(next, {} as any); + const args = { input: { - SSECustomerKey: "foo", - CopySourceSSECustomerKey: "bar", + SSECustomerKey: validBase64EncodedKey, + CopySourceSSECustomerKey: validBase64EncodedKey, }, }; + await handler(args); + + expect(next).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledWith({ + input: { + SSECustomerKey: validBase64EncodedKey, + SSECustomerKeyMD5: base64EncodedMD5Hash, + CopySourceSSECustomerKey: validBase64EncodedKey, + CopySourceSSECustomerKeyMD5: base64EncodedMD5Hash, + }, + }); + + expect(base64Decoder).toHaveBeenCalledTimes(4); + expect(base64Decoder).toHaveBeenCalledWith(validBase64EncodedKey); + expect(mockMD5Constructor).toHaveBeenCalledTimes(2); + expect(mockMD5Instance.update).toHaveBeenCalledTimes(2); + expect(mockMD5Instance.update).toHaveBeenCalledWith(decodedBytes); + expect(mockMD5Instance.digest).toHaveBeenCalledTimes(2); + expect(base64Encoder).toHaveBeenCalledTimes(2); + expect(base64Encoder).toHaveBeenCalledWith(md5Hash); + }); + + it("should handle 32-byte binary key input for SSECustomerKey and CopySourceSSECustomerKey", async () => { + const binaryKey = crypto.randomBytes(32); + const md5Hash = crypto.createHash("md5").update(binaryKey).digest(); + const base64EncodedKey = binaryKey.toString("base64"); + const base64EncodedMD5Hash = md5Hash.toString("base64"); + + const mockMD5Constructor = jest.fn().mockReturnValue({ + update: jest.fn().mockReturnThis(), + digest: jest.fn().mockReturnValueOnce(md5Hash).mockReturnValueOnce(md5Hash), + }); + + const base64Encoder = jest + .fn() + .mockReturnValueOnce(base64EncodedKey) + .mockReturnValueOnce(base64EncodedMD5Hash) + .mockReturnValueOnce(base64EncodedKey) + .mockReturnValueOnce(base64EncodedMD5Hash); + const handler = ssecMiddleware({ - base64Encoder: encoder2, + base64Encoder, utf8Decoder: decoder, - md5: MockHash, - base64Decoder: base64Decoder, + md5: mockMD5Constructor, + base64Decoder, })(next, {} as any); + const args = { + input: { + SSECustomerKey: binaryKey, + CopySourceSSECustomerKey: binaryKey, + }, + }; + await handler(args); - expect(next.mock.calls.length).toBe(1); + expect(next).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledWith({ input: { - SSECustomerKey: "base64", - SSECustomerKeyMD5: "base64", - CopySourceSSECustomerKey: "base64", - CopySourceSSECustomerKeyMD5: "base64", + SSECustomerKey: base64EncodedKey, + SSECustomerKeyMD5: base64EncodedMD5Hash, + CopySourceSSECustomerKey: base64EncodedKey, + CopySourceSSECustomerKeyMD5: base64EncodedMD5Hash, }, }); - expect(decoder.mock.calls.length).toBe(2); - expect(encoder2.mock.calls.length).toBe(4); - expect(mockHashUpdate.mock.calls.length).toBe(2); - expect(mockHashDigest.mock.calls.length).toBe(2); - encoder2.mockClear(); + + expect(mockMD5Constructor).toHaveBeenCalledTimes(2); + expect(base64Encoder).toHaveBeenCalledTimes(4); + }); + + it("should return false for an invalid base64 string", () => { + const invalidBase64 = "invalid!@#$%"; + const base64Decoder = jest.fn(); + const options = { base64Decoder }; + + const result = isValidBase64EncodedSSECustomerKey(invalidBase64, options as any); + expect(result).toBe(false); + expect(base64Decoder).not.toHaveBeenCalled(); + }); + + it("should return true for a valid base64 string and 32 bytes", () => { + const validBase64EncodedSSECustomerKey = "QUIwMTIzNDU2Nzg5QUJDREVGQUJDREVGQUJDREVGQUI="; + const decodedBytes = new Uint8Array(32); + const base64Decoder = jest.fn().mockReturnValue(decodedBytes); + const options = { base64Decoder }; + + const result = isValidBase64EncodedSSECustomerKey(validBase64EncodedSSECustomerKey, options as any); + expect(result).toBe(true); + expect(base64Decoder).toHaveBeenCalledTimes(1); + expect(base64Decoder).toHaveBeenCalledWith(validBase64EncodedSSECustomerKey); + }); + + it("should return false for a valid base64 string but not 32 bytes", () => { + const validBase64NonThirtyTwoBytes = "SGVsbG8="; + const base64Decoder = jest.fn().mockReturnValue(new Uint8Array([72, 101, 108, 108, 111])); + const options = { base64Decoder }; + + const result = isValidBase64EncodedSSECustomerKey(validBase64NonThirtyTwoBytes, options as any); + expect(result).toBe(false); + expect(base64Decoder).toHaveBeenCalledTimes(1); + expect(base64Decoder).toHaveBeenCalledWith(validBase64NonThirtyTwoBytes); }); }); diff --git a/packages/middleware-ssec/src/index.ts b/packages/middleware-ssec/src/index.ts index d11f9db45d74..29466311a738 100644 --- a/packages/middleware-ssec/src/index.ts +++ b/packages/middleware-ssec/src/index.ts @@ -78,14 +78,14 @@ export const getSsecPlugin = (config: PreviouslyResolved): Pluggable = }, }); -function isValidBase64EncodedSSECustomerKey(str: string, options: PreviouslyResolved) { +export function isValidBase64EncodedSSECustomerKey(str: string, options: PreviouslyResolved) { const base64Regex = /^(?:[A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/; if (!base64Regex.test(str)) return false; try { const decodedBytes = options.base64Decoder(str); - if (decodedBytes.length !== 32) return false; + return decodedBytes.length === 32; } catch { return false; }