From c59f537b1262b5d7997291b8c1e9324d378effb6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Feb 2023 19:31:52 -0500 Subject: [PATCH] Improve decodeBase64() to throw on invalid input rather than silently accept it (#7019) --- .changeset/popular-apples-peel.md | 7 +++++ common/api-review/util.api.md | 8 ++++++ packages/firestore/src/platform/base64.ts | 15 ++++++++++- .../firestore/src/platform/browser/base64.ts | 12 ++++++++- packages/firestore/src/platform/rn/base64.ts | 26 +++++++++++++------ .../firestore/src/util/base64_decode_error.ts | 23 ++++++++++++++++ .../firestore/test/lite/integration.test.ts | 2 +- packages/util/src/crypt.ts | 9 ++++++- 8 files changed, 90 insertions(+), 12 deletions(-) create mode 100644 .changeset/popular-apples-peel.md create mode 100644 packages/firestore/src/util/base64_decode_error.ts diff --git a/.changeset/popular-apples-peel.md b/.changeset/popular-apples-peel.md new file mode 100644 index 00000000000..3269c43d903 --- /dev/null +++ b/.changeset/popular-apples-peel.md @@ -0,0 +1,7 @@ +--- +'@firebase/firestore': patch +'@firebase/util': patch +'firebase': patch +--- + +Modify base64 decoding logic to throw on invalid input, rather than silently truncating it. diff --git a/common/api-review/util.api.md b/common/api-review/util.api.md index be8703adf3f..75e484edd50 100644 --- a/common/api-review/util.api.md +++ b/common/api-review/util.api.md @@ -93,6 +93,14 @@ export function createSubscribe(executor: Executor, onNoObservers?: Execut // @public export const decode: (token: string) => DecodedToken; +// Warning: (ae-missing-release-tag) "DecodeBase64StringError" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public +export class DecodeBase64StringError extends Error { + // (undocumented) + readonly name = "DecodeBase64StringError"; +} + // Warning: (ae-missing-release-tag) "deepCopy" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public diff --git a/packages/firestore/src/platform/base64.ts b/packages/firestore/src/platform/base64.ts index 877a78c732b..e6e2356afbb 100644 --- a/packages/firestore/src/platform/base64.ts +++ b/packages/firestore/src/platform/base64.ts @@ -15,13 +15,26 @@ * limitations under the License. */ +import { Base64DecodeError } from '../util/base64_decode_error'; + // This file is only used under ts-node. // eslint-disable-next-line @typescript-eslint/no-require-imports const platform = require(`./${process.env.TEST_PLATFORM ?? 'node'}/base64`); /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - return platform.decodeBase64(encoded); + const decoded = platform.decodeBase64(encoded); + + // A quick correctness check that the input string was valid base64. + // See https://stackoverflow.com/questions/13378815/base64-length-calculation. + // This is done because node and rn will not always throw an error if the + // input is an invalid base64 string (e.g. "A==="). + const expectedEncodedLength = 4 * Math.ceil(decoded.length / 3); + if (encoded.length !== expectedEncodedLength) { + throw new Base64DecodeError('Invalid base64 string'); + } + + return decoded; } /** Converts a binary string to a Base64 encoded string. */ diff --git a/packages/firestore/src/platform/browser/base64.ts b/packages/firestore/src/platform/browser/base64.ts index 6c068b17244..4cbbfe6ced9 100644 --- a/packages/firestore/src/platform/browser/base64.ts +++ b/packages/firestore/src/platform/browser/base64.ts @@ -15,9 +15,19 @@ * limitations under the License. */ +import { Base64DecodeError } from '../../util/base64_decode_error'; + /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - return atob(encoded); + try { + return atob(encoded); + } catch (e) { + if (e instanceof DOMException) { + throw new Base64DecodeError('Invalid base64 string: ' + e); + } else { + throw e; + } + } } /** Converts a binary string to a Base64 encoded string. */ diff --git a/packages/firestore/src/platform/rn/base64.ts b/packages/firestore/src/platform/rn/base64.ts index 4032190e5cd..f50c604ebf7 100644 --- a/packages/firestore/src/platform/rn/base64.ts +++ b/packages/firestore/src/platform/rn/base64.ts @@ -15,7 +15,9 @@ * limitations under the License. */ -import { base64 } from '@firebase/util'; +import { base64, DecodeBase64StringError } from '@firebase/util'; + +import { Base64DecodeError } from '../../util/base64_decode_error'; // WebSafe uses a different URL-encoding safe alphabet that doesn't match // the encoding used on the backend. @@ -23,13 +25,21 @@ const WEB_SAFE = false; /** Converts a Base64 encoded string to a binary string. */ export function decodeBase64(encoded: string): string { - return String.fromCharCode.apply( - null, - // We use `decodeStringToByteArray()` instead of `decodeString()` since - // `decodeString()` returns Unicode strings, which doesn't match the values - // returned by `atob()`'s Latin1 representation. - base64.decodeStringToByteArray(encoded, WEB_SAFE) - ); + try { + return String.fromCharCode.apply( + null, + // We use `decodeStringToByteArray()` instead of `decodeString()` since + // `decodeString()` returns Unicode strings, which doesn't match the values + // returned by `atob()`'s Latin1 representation. + base64.decodeStringToByteArray(encoded, WEB_SAFE) + ); + } catch (e) { + if (e instanceof DecodeBase64StringError) { + throw new Base64DecodeError('Invalid base64 string: ' + e); + } else { + throw e; + } + } } /** Converts a binary string to a Base64 encoded string. */ diff --git a/packages/firestore/src/util/base64_decode_error.ts b/packages/firestore/src/util/base64_decode_error.ts new file mode 100644 index 00000000000..84a20c55d1d --- /dev/null +++ b/packages/firestore/src/util/base64_decode_error.ts @@ -0,0 +1,23 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * An error encountered while decoding base64 string. + */ +export class Base64DecodeError extends Error { + readonly name = 'Base64DecodeError'; +} diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index dcc9f46a508..079593c1765 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -857,7 +857,7 @@ describe('DocumentSnapshot', () => { it('returns Bytes', () => { return withTestDocAndInitialData( - { bytes: Bytes.fromBase64String('aa') }, + { bytes: Bytes.fromBase64String('aa==') }, async docRef => { const docSnap = await getDoc(docRef); const bytes = docSnap.get('bytes'); diff --git a/packages/util/src/crypt.ts b/packages/util/src/crypt.ts index df1e058756f..7cd32125043 100644 --- a/packages/util/src/crypt.ts +++ b/packages/util/src/crypt.ts @@ -284,7 +284,7 @@ export const base64: Base64 = { ++i; if (byte1 == null || byte2 == null || byte3 == null || byte4 == null) { - throw Error(); + throw new DecodeBase64StringError(); } const outByte1 = (byte1 << 2) | (byte2 >> 4); @@ -333,6 +333,13 @@ export const base64: Base64 = { } }; +/** + * An error encountered while decoding base64 string. + */ +export class DecodeBase64StringError extends Error { + readonly name = 'DecodeBase64StringError'; +} + /** * URL-safe base64 encoding */