feat!: add native AES-GCM encrypt/decrypt#55
Merged
Conversation
- Extend TurboModule spec with aesGcmEncrypt / aesGcmDecrypt that accept
hex-encoded data, key, nonce and aad.
- Android: implement via Cipher.getInstance("AES/GCM/NoPadding") with
GCMParameterSpec(128, nonce). Output is hex-encoded ciphertext||tag.
- iOS: implement via CryptoKit AES.GCM in a new AesCryptoGcm.swift
helper bridged from AesCrypto.mm.
- Podspec: link Security + CryptoKit, disable Swift explicit modules to
match the existing ObjC++ + Swift bridging setup.
- AesCrypto.h: guard Objective-C++ header with #ifdef __cplusplus so
the generated AesCrypto-Swift.h can import it cleanly.
Encrypt accepts AAD and returns ciphertext||tag (tag length 16 bytes).
Decrypt accepts the same combined ciphertext||tag layout, verifies the
authentication tag and rejects the promise on tag mismatch.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d2d718d4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Drop the unsafe early-returns flagged by Codex on PR #55: - aesGcmEncryptImpl no longer short-circuits empty plaintext. CryptoKit on iOS still runs AES.GCM.seal and returns the 16-byte authentication tag for an empty plaintext; Android must do the same so the two backends produce the same `ciphertext || tag` shape and the JS layer can detect tampering on either side. - aesGcmDecryptImpl no longer treats empty input as a successful decrypt. Instead it rejects any ciphertextWithTag shorter than the GCM tag length (16 bytes), matching the iOS `encrypted.count < 16` guard. This closes a path where a truncated or missing ciphertext could otherwise bypass authentication and silently resolve to "" on Android. The decrypt guard throws an IllegalArgumentException, which the JS-facing wrapper translates into a promise rejection ("-1", message).
sidmorizon
added a commit
that referenced
this pull request
May 13, 2026
Drop the unsafe early-returns flagged by Codex on PR #55: - aesGcmEncryptImpl no longer short-circuits empty plaintext. CryptoKit on iOS still runs AES.GCM.seal and returns the 16-byte authentication tag for an empty plaintext; Android must do the same so the two backends produce the same `ciphertext || tag` shape and the JS layer can detect tampering on either side. - aesGcmDecryptImpl no longer treats empty input as a successful decrypt. Instead it rejects any ciphertextWithTag shorter than the GCM tag length (16 bytes), matching the iOS `encrypted.count < 16` guard. This closes a path where a truncated or missing ciphertext could otherwise bypass authentication and silently resolve to "" on Android. The decrypt guard throws an IllegalArgumentException, which the JS-facing wrapper translates into a promise rejection ("-1", message).
sidmorizon
added a commit
to OneKeyHQ/app-monorepo
that referenced
this pull request
May 13, 2026
Sync the Android-side fix landing in upstream PR OneKeyHQ/app-modules#55: remove the unsafe empty-string early returns in aesGcmEncryptImpl / aesGcmDecryptImpl so the Android backend stays byte-aligned with the iOS CryptoKit path. - Encrypt path: always runs Cipher.doFinal so an empty plaintext still resolves to the 16-byte authentication tag, matching CryptoKit's `AES.GCM.seal` behavior. Without this the JS layer would receive "" on Android while iOS returns a tag, breaking cross-platform decrypt. - Decrypt path: rejects any ciphertextWithTag shorter than the GCM tag length (16 bytes) with IllegalArgumentException. Previously a missing or truncated payload could silently resolve to "" on Android while iOS already throws `invalidCiphertext`. This closes a path where a caller could bypass tag verification. Patch hunk size adjusted from +30 to +31 lines accordingly.
sidmorizon
added a commit
to OneKeyHQ/app-monorepo
that referenced
this pull request
May 13, 2026
Cover the three cases Codex flagged on OneKeyHQ/app-modules#55 that the on-node jest suite cannot exercise because they require the real native CryptoKit / Cipher paths: - Empty plaintext: assert noble emits exactly the 16-byte GCM tag, the RNAes wrapper produces the same hex, and the native decrypt round-trips back to an empty buffer. - Short ciphertext (<16 bytes): both native and noble decrypt must reject, so a missing or truncated tag can never bypass GCM authentication. - Wrong AAD: both native and noble decrypt must reject when the AAD is tampered, even with otherwise-valid ciphertext. These vectors run inside AES-GCM v2 Test and surface a clear pass / reject in the table so the next round of device-level verification (iOS simulator + Android emulator) can confirm parity.
Add defensive input validation at every native entry point so that malformed callers — forgotten parameters, miswired AAD, truncated input, zero key length — surface as an explicit promise rejection instead of silently passing through to the cipher / digest layer. Scope (Android Kotlin + iOS ObjC++ + iOS Swift): - encrypt / decrypt: data | ciphertext, key, iv, algorithm must be non-empty. - aesGcmEncrypt / aesGcmDecrypt: data | ciphertextWithTag, key, nonce AND aad must all be non-empty. We intentionally also require a non-empty AAD even though AEAD allows 0-byte AAD — every production caller already supplies an explicit context binding (v2 envelope header bytes; keyless fixed AAD constants) so rejecting empty AAD catches "forgot to pass AAD" bugs without affecting any current call site. - pbkdf2: password, salt, algorithm must be non-empty; cost and length must be > 0. - hmac256 / hmac512: data, key must be non-empty. - sha1 / sha256 / sha512: text must be non-empty. - randomKey: length must be > 0. Implementation pattern: - Android: shared `requireNonEmpty` / `requirePositive` helpers in the companion object; each `override fun` validates its inputs in the try block so any failure rejects the JS promise with the standard "-1" code and a descriptive message. - iOS: ONEKEY_AES_REQUIRE_NON_EMPTY / _POSITIVE macros guard each `RCT_EXPORT_METHOD` body before any dispatch_async. They reject the promise synchronously with `-1` and a `<method>: <paramName> must not be empty` message. - iOS Swift: matching `requireNonEmpty` helper inside AesCryptoGcm so the AES-GCM entry points reject empty hex even when called outside the ObjC++ macro guard. The decrypt-side >= 16-byte tag check on AES-GCM (added in 2ea235e for PR #55 Codex review) is preserved; the new empty-string guard runs first and catches the "completely empty" case before the tag- length check is reached.
sidmorizon
added a commit
that referenced
this pull request
May 13, 2026
Add defensive input validation at every native entry point so that malformed callers — forgotten parameters, miswired AAD, truncated input, zero key length — surface as an explicit promise rejection instead of silently passing through to the cipher / digest layer. Scope (Android Kotlin + iOS ObjC++ + iOS Swift): - encrypt / decrypt: data | ciphertext, key, iv, algorithm must be non-empty. - aesGcmEncrypt / aesGcmDecrypt: data | ciphertextWithTag, key, nonce AND aad must all be non-empty. We intentionally also require a non-empty AAD even though AEAD allows 0-byte AAD — every production caller already supplies an explicit context binding (v2 envelope header bytes; keyless fixed AAD constants) so rejecting empty AAD catches "forgot to pass AAD" bugs without affecting any current call site. - pbkdf2: password, salt, algorithm must be non-empty; cost and length must be > 0. - hmac256 / hmac512: data, key must be non-empty. - sha1 / sha256 / sha512: text must be non-empty. - randomKey: length must be > 0. Implementation pattern: - Android: shared `requireNonEmpty` / `requirePositive` helpers in the companion object; each `override fun` validates its inputs in the try block so any failure rejects the JS promise with the standard "-1" code and a descriptive message. - iOS: ONEKEY_AES_REQUIRE_NON_EMPTY / _POSITIVE macros guard each `RCT_EXPORT_METHOD` body before any dispatch_async. They reject the promise synchronously with `-1` and a `<method>: <paramName> must not be empty` message. - iOS Swift: matching `requireNonEmpty` helper inside AesCryptoGcm so the AES-GCM entry points reject empty hex even when called outside the ObjC++ macro guard. The decrypt-side >= 16-byte tag check on AES-GCM (added in 2ea235e for PR #55 Codex review) is preserved; the new empty-string guard runs first and catches the "completely empty" case before the tag- length check is reached.
sidmorizon
added a commit
to OneKeyHQ/app-monorepo
that referenced
this pull request
May 13, 2026
Sync upstream PR OneKeyHQ/app-modules#55 commit 75031eb so the native module rejects empty string / non-positive numeric inputs at every entry point. AAD is included on purpose: every production GCM call site supplies an explicit context binding (v2 envelope header bytes; keyless fixed AAD constants), so rejecting empty AAD catches "forgot to pass AAD" bugs without affecting any current caller. - patches/react-native-aes-crypto+3.0.27.patch: regenerated from the pristine 3.0.27 yarn cache zip + the upstream PR working tree to include the new Kotlin/ObjC++/Swift guards. - CryptoGallery AES-GCM v2 Test: replace the three buggy "empty plaintext" vectors (the JS invoke check already rejects zero- length data, so those tests never reached the cipher layer) with three vectors that actually exercise the new contract: noble and native both reject empty plaintext, and native alone rejects empty AAD (the JS invoke check intentionally does not validate AAD so diagnostic tools can drive raw inputs).
Contributor
Author
|
Update: in addition to the empty-plaintext / short-ciphertext fix from 2ea235e, commit 75031eb expands the same defensive philosophy to every native entry point. Codex's two P2 highlighted the value of catching "empty/missing" inputs early — rather than only patch the two GCM paths that were flagged, the module now rejects empty strings and non-positive numeric arguments uniformly. Scope of 75031eb:
All three layers share the same contract:
Validation:
Native runtime verification (Android Gradle + iOS xcodebuild + device-side |
sidmorizon
added a commit
to OneKeyHQ/app-monorepo
that referenced
this pull request
May 13, 2026
The native guard added upstream in OneKeyHQ/app-modules#55 commit 75031eb rejects empty AAD, but the JS-side noble fallback used on desktop/web/extension did not — so a desktop client could emit an empty-AAD GCM payload that mobile native then refuses to decrypt. Add the same check to _aesGcmInvokeCheck so both backends share one contract. - packages/shared/src/appCrypto/modules/aesGcm.ts: extend _aesGcmInvokeCheck to also throw on zero-length aad. Audit confirmed every production callsite supplies a non-empty AAD (v2 envelope header bytes for all v2 traffic; KEYLESS_*_AAD constants for legacy GCM in keyless storage / cloud sync / mnemonic / backend share). Plumb aad through the four encryptByNoble / decryptByNoble / encryptByRNAes / decryptByRNAes call sites so the guard sees it. - CryptoGallery AES-GCM v2 Test: add an "AES-GCM noble rejects empty AAD" boundary vector to match the existing native one, so the table now shows the contract is symmetrical across backends. Existing core/secret AES-GCM jest tests (aes256.test.ts + secret-aes256.test.ts) still pass (59/59) — every test already supplies an explicit aad in mode=gcm calls. The unrelated snapshot drift in secret.test.ts predates this change and is out of scope.
sidmorizon
added a commit
that referenced
this pull request
May 13, 2026
Drop the unsafe early-returns flagged by Codex on PR #55: - aesGcmEncryptImpl no longer short-circuits empty plaintext. CryptoKit on iOS still runs AES.GCM.seal and returns the 16-byte authentication tag for an empty plaintext; Android must do the same so the two backends produce the same `ciphertext || tag` shape and the JS layer can detect tampering on either side. - aesGcmDecryptImpl no longer treats empty input as a successful decrypt. Instead it rejects any ciphertextWithTag shorter than the GCM tag length (16 bytes), matching the iOS `encrypted.count < 16` guard. This closes a path where a truncated or missing ciphertext could otherwise bypass authentication and silently resolve to "" on Android. The decrypt guard throws an IllegalArgumentException, which the JS-facing wrapper translates into a promise rejection ("-1", message).
sidmorizon
added a commit
that referenced
this pull request
May 13, 2026
Add defensive input validation at every native entry point so that malformed callers — forgotten parameters, miswired AAD, truncated input, zero key length — surface as an explicit promise rejection instead of silently passing through to the cipher / digest layer. Scope (Android Kotlin + iOS ObjC++ + iOS Swift): - encrypt / decrypt: data | ciphertext, key, iv, algorithm must be non-empty. - aesGcmEncrypt / aesGcmDecrypt: data | ciphertextWithTag, key, nonce AND aad must all be non-empty. We intentionally also require a non-empty AAD even though AEAD allows 0-byte AAD — every production caller already supplies an explicit context binding (v2 envelope header bytes; keyless fixed AAD constants) so rejecting empty AAD catches "forgot to pass AAD" bugs without affecting any current call site. - pbkdf2: password, salt, algorithm must be non-empty; cost and length must be > 0. - hmac256 / hmac512: data, key must be non-empty. - sha1 / sha256 / sha512: text must be non-empty. - randomKey: length must be > 0. Implementation pattern: - Android: shared `requireNonEmpty` / `requirePositive` helpers in the companion object; each `override fun` validates its inputs in the try block so any failure rejects the JS promise with the standard "-1" code and a descriptive message. - iOS: ONEKEY_AES_REQUIRE_NON_EMPTY / _POSITIVE macros guard each `RCT_EXPORT_METHOD` body before any dispatch_async. They reject the promise synchronously with `-1` and a `<method>: <paramName> must not be empty` message. - iOS Swift: matching `requireNonEmpty` helper inside AesCryptoGcm so the AES-GCM entry points reject empty hex even when called outside the ObjC++ macro guard. The decrypt-side >= 16-byte tag check on AES-GCM (added in 2ea235e for PR #55 Codex review) is preserved; the new empty-string guard runs first and catches the "completely empty" case before the tag- length check is reached.
sidmorizon
force-pushed
the
feat/aes-crypto-gcm-support
branch
from
75031eb to
eb95010
Compare
huhuanming
reviewed
May 13, 2026
Contributor
huhuanming
left a comment
huhuanming
left a comment
There was a problem hiding this comment.
整体 AES-GCM 实现 OK(布局/tag 处理/认证失败抛错都对),但把「新增 GCM」和「给所有 legacy 入口加严格非空校验」捆在一个 PR 里,带来了几处隐性 breaking change 和契约一致性问题。逐行评论如下,优先级标在每条开头。
originalix
reviewed
May 13, 2026
Address review feedback on PR #55 (huhuanming + originalix) without relaxing the strict-input contract: - AES-GCM nonce: enforce a 12-byte (96-bit) length on both Android and iOS. GCMParameterSpec on Android accepts arbitrary nonce lengths, CryptoKit on iOS effectively does not — letting non-12-byte nonces through made it possible for Android to encrypt payloads that the iOS side could not decrypt. The bridge layer now decodes the nonce hex up front and rejects any length != 12 on both platforms. - Numeric guards (pbkdf2 cost/length, randomKey length): the previous `<= 0` check passed NaN, Infinity, and fractional values straight through to the cipher/KDF layer (NaN compares false to every value; 1.5 would silently cast to 1). The Kotlin helper and the ObjC++ macro now reject non-finite, non-integer, non-positive, and > INT_MAX inputs uniformly with the existing `-1` reject code. - AES-GCM impl: drop the dead `if (aad.isNullOrEmpty())` branch in AesCryptoModule.kt's encrypt/decrypt impls. The entry point already enforces non-empty AAD via requireNonEmpty, so the impl signature is tightened to `aad: String` and `updateAAD` is called unconditionally. - AES-GCM error codes (iOS): rejects in aesGcmEncrypt / aesGcmDecrypt now use `"-1"` to match every other method in the module instead of the module-local `"aes_gcm_encrypt_fail"` / `"aes_gcm_decrypt_fail"` strings. - AesCryptoGcm.swift: update the requireNonEmpty doc comment so it reflects the post-75031eb reality (legacy CBC/CTR also enforces non-empty inputs through the ObjC++ entry — the previous comment implied they were unaffected). - AesCrypto.h: keep the `#ifdef __cplusplus` block, but add a comment explaining why the `@interface` lives inside it (the TurboModule base class is ObjC++-only). - AesCrypto.podspec: comment the `SWIFT_ENABLE_EXPLICIT_MODULES = NO` workaround so future maintainers know it is tied to the ObjC++ + Swift bridging-header conflict rather than a general toolchain pessimisation.
sidmorizon
added a commit
that referenced
this pull request
May 13, 2026
Address review feedback on PR #55 (huhuanming + originalix) without relaxing the strict-input contract: - AES-GCM nonce: enforce a 12-byte (96-bit) length on both Android and iOS. GCMParameterSpec on Android accepts arbitrary nonce lengths, CryptoKit on iOS effectively does not — letting non-12-byte nonces through made it possible for Android to encrypt payloads that the iOS side could not decrypt. The bridge layer now decodes the nonce hex up front and rejects any length != 12 on both platforms. - Numeric guards (pbkdf2 cost/length, randomKey length): the previous `<= 0` check passed NaN, Infinity, and fractional values straight through to the cipher/KDF layer (NaN compares false to every value; 1.5 would silently cast to 1). The Kotlin helper and the ObjC++ macro now reject non-finite, non-integer, non-positive, and > INT_MAX inputs uniformly with the existing `-1` reject code. - AES-GCM impl: drop the dead `if (aad.isNullOrEmpty())` branch in AesCryptoModule.kt's encrypt/decrypt impls. The entry point already enforces non-empty AAD via requireNonEmpty, so the impl signature is tightened to `aad: String` and `updateAAD` is called unconditionally. - AES-GCM error codes (iOS): rejects in aesGcmEncrypt / aesGcmDecrypt now use `"-1"` to match every other method in the module instead of the module-local `"aes_gcm_encrypt_fail"` / `"aes_gcm_decrypt_fail"` strings. - AesCryptoGcm.swift: update the requireNonEmpty doc comment so it reflects the post-75031eb reality (legacy CBC/CTR also enforces non-empty inputs through the ObjC++ entry — the previous comment implied they were unaffected). - AesCrypto.h: keep the `#ifdef __cplusplus` block, but add a comment explaining why the `@interface` lives inside it (the TurboModule base class is ObjC++-only). - AesCrypto.podspec: comment the `SWIFT_ENABLE_EXPLICIT_MODULES = NO` workaround so future maintainers know it is tied to the ObjC++ + Swift bridging-header conflict rather than a general toolchain pessimisation.
huhuanming
reviewed
May 13, 2026
Contributor
huhuanming
left a comment
huhuanming
left a comment
There was a problem hiding this comment.
拉了一遍 0add229,补几条按行的 follow-up,优先级标在每条开头。整体 GCM 实现 OK,主要是入口收紧后的契约一致性 / dead-code / iOS UX 的尾巴。
另:PR description 的 Test plan 6 个 checkbox 全部未勾。yarn typecheck / swiftc -typecheck / monorepo patch-back 已确认,但跨端 byte 一致性 vector + boundary 用例(11/13/0-byte nonce、NaN/Infinity cost、空 AAD)还没贴出来,建议合并前补一组实测。
Title 是 feat 但 body 列了 6 个方法的 breaking,按 conventional commits 应该是 feat!: —— 不影响合并,只是后续自动 changelog 会更准。
… dead branches Address huhuanming's second-round review (P1×2, P2×3, P3×1) on PR #55: - GCM data/ciphertextWithTag: drop the entry-level `requireNonEmpty` guard for `aesGcmEncrypt(data)` and `aesGcmDecrypt(ciphertextWithTag)` on all three layers (Kotlin / ObjC++ macros / Swift). Empty plaintext is a legitimate AEAD operation (produces the 16-byte tag), and was the scenario commit `2f7086be3` was already trying to handle — the entry guard from `75031ebc` was silently nullifying that fix. `key` / `nonce` / `aad` remain strictly non-empty. The decrypt-side `>= 16-byte` tag guard is the only check that still fires for "too short" inputs. - iOS legacy reject codes: switch every CBC/CTR/PBKDF2/HMAC/SHA/random rejection from the per-method string codes (`encrypt_fail`, `decrypt_fail`, `keygen_fail`, `hmac_fail`, `sha*_fail`, `uuid_fail`, `random_fail`) to the unified `"-1"`. Now matches Android (which was already uniform) and the GCM path; JS callers only need to handle a single error code across both platforms and all methods. - Swift `AesCryptoGcmError`: implement `LocalizedError` with an `errorDescription` for `.invalidHex` and `.invalidCiphertext`, so that when the enum bridges to `NSError`, `localizedDescription` carries the real message instead of Cocoa's default "The operation couldn't be completed. (... error N.)". The JS reject message is now informative on both Android and iOS. - Android `encryptImpl` / `decryptImpl` cleanup: remove the unreachable `if (text.isEmpty()) return null`, the `emptyIvSpec` companion, and the `if (hexIv == null || hexIv.isEmpty())` fallback. Entry-level `requireNonEmpty(iv, ...)` already covers those cases; tighten impl signatures to non-null `hexIv`. - `src/index.tsx`: add JSDoc to `aesGcmEncrypt` / `aesGcmDecrypt` spelling out the contract (12-byte nonce, non-empty AAD, hex strings, `ciphertext || tag` layout, empty plaintext allowed) so consumers do not have to read native source to understand it. P2 "legacy `fromHex` strtol leniency" left as follow-up — fixing it requires broadening this PR to all CBC/CTR/PBKDF2/HMAC/SHA paths and revalidating against existing fixed-vector tests; tracking separately.
sidmorizon
added a commit
that referenced
this pull request
May 13, 2026
… dead branches Address huhuanming's second-round review (P1×2, P2×3, P3×1) on PR #55: - GCM data/ciphertextWithTag: drop the entry-level `requireNonEmpty` guard for `aesGcmEncrypt(data)` and `aesGcmDecrypt(ciphertextWithTag)` on all three layers (Kotlin / ObjC++ macros / Swift). Empty plaintext is a legitimate AEAD operation (produces the 16-byte tag), and was the scenario commit `2f7086be3` was already trying to handle — the entry guard from `75031ebc` was silently nullifying that fix. `key` / `nonce` / `aad` remain strictly non-empty. The decrypt-side `>= 16-byte` tag guard is the only check that still fires for "too short" inputs. - iOS legacy reject codes: switch every CBC/CTR/PBKDF2/HMAC/SHA/random rejection from the per-method string codes (`encrypt_fail`, `decrypt_fail`, `keygen_fail`, `hmac_fail`, `sha*_fail`, `uuid_fail`, `random_fail`) to the unified `"-1"`. Now matches Android (which was already uniform) and the GCM path; JS callers only need to handle a single error code across both platforms and all methods. - Swift `AesCryptoGcmError`: implement `LocalizedError` with an `errorDescription` for `.invalidHex` and `.invalidCiphertext`, so that when the enum bridges to `NSError`, `localizedDescription` carries the real message instead of Cocoa's default "The operation couldn't be completed. (... error N.)". The JS reject message is now informative on both Android and iOS. - Android `encryptImpl` / `decryptImpl` cleanup: remove the unreachable `if (text.isEmpty()) return null`, the `emptyIvSpec` companion, and the `if (hexIv == null || hexIv.isEmpty())` fallback. Entry-level `requireNonEmpty(iv, ...)` already covers those cases; tighten impl signatures to non-null `hexIv`. - `src/index.tsx`: add JSDoc to `aesGcmEncrypt` / `aesGcmDecrypt` spelling out the contract (12-byte nonce, non-empty AAD, hex strings, `ciphertext || tag` layout, empty plaintext allowed) so consumers do not have to read native source to understand it. P2 "legacy `fromHex` strtol leniency" left as follow-up — fixing it requires broadening this PR to all CBC/CTR/PBKDF2/HMAC/SHA paths and revalidating against existing fixed-vector tests; tracking separately.
sidmorizon
changed the title
Contributor
Author
|
第二轮 review 已处理,新 HEAD inline 回复(每条具体在原 thread 里):
总评回应:
BREAKING CHANGES 表格已同步更新:
请复审。 |
sidmorizon
force-pushed
the
feat/aes-crypto-gcm-support
branch
from
e8ec66a to
4822ed4
Compare
Contributor
Author
|
已 rebase 到 origin/main(
mergeable 状态从 review thread 里 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
huhuanming
approved these changes
May 13, 2026
originalix
reviewed
May 13, 2026
| // would silently truncate (1.5 -> 1) or saturate (NaN -> 0), and the | ||
| // `<= 0` guard alone would let either path slip through into cipher / | ||
| // KDF code that subsequently treats the value as a buffer size. | ||
| private fun requirePositiveInt(value: Double, method: String, paramName: String): Int { |
There was a problem hiding this comment.
这里虽然已经挡住了 NaN / Infinity / 小数,但 <= Int.MAX_VALUE 仍然太宽。randomKey(2147483647) 会尝试分配接近 2GB 的 ByteArray,Android 侧还会抛 OutOfMemoryError 而不是 Exception,当前 catch (e: Exception) 接不住,进程可能直接崩。pbkdf2(cost, length) 也类似:超大的 cost 会把 KDF 线程拖死,超大的 length 会生成巨大的输出 buffer。
建议不要用一个通用的 INT_MAX 上限,而是按方法做业务上限:例如 randomKey 限制到实际需要的 key/material 字节数,pbkdf2.length 限制到可接受的 key bits,pbkdf2.cost 限制到可接受耗时范围。iOS 的 ONEKEY_AES_REQUIRE_POSITIVE 现在也有同样的 INT_MAX 上限问题,需要同步收紧。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add native AES-GCM (
AES-256-GCM) support to@onekeyfe/react-native-aes-crypto, paired with the OneKey app v2 envelope rollout (1K_ENC_V2) that lands inapp-monorepo. The JS side already calls these methods via a patch-package patch onreact-native-aes-crypto+3.0.27.patch; this PR moves the changes upstream so the patch can be retired once a new version is published.Surface area:
TurboModuleSpec: addaesGcmEncrypt(data, key, nonce, aad)andaesGcmDecrypt(ciphertextWithTag, key, nonce, aad). All four args are hex strings; return value is a hex string.Cipher.getInstance("AES/GCM/NoPadding")withGCMParameterSpec(128, nonce). Output isciphertext || tag, tag is 16 bytes.updateAADis called unconditionally (AAD is required). Decrypt rejects on tag mismatch.CryptoKit.AES.GCMvia a newAesCryptoGcm.swifthelper, called fromAesCrypto.mmthrough the generatedAesCrypto-Swift.h. Sameciphertext || taglayout.AesCryptoGcmErroradoptsLocalizedErrorso JS rejects carry a real message.Security+CryptoKit, setSWIFT_ENABLE_EXPLICIT_MODULES = NOto match the existing ObjC++ + Swift bridging.AesCrypto.h: wrap the Objective-C++ header in#ifdef __cplusplusso the generatedAesCrypto-Swift.hcan import it cleanly.src/index.tsx: JSDoc onaesGcmEncrypt/aesGcmDecryptdocumenting the tighter-than-spec contract.This PR also tightens the input contract on every native entry point (introduced in 75031eb, hardened further in 0add229, refined in e8ec66a). Callers that previously got silent zero-length / 0-init behaviour now receive a synchronous
-1promise rejection.The module is consumed inside OneKey monorepo only (via patch-package), so no external npm consumer is affected. Bumping to a new major version when this is released upstream —
feat!:per conventional commits.Newly enforced contracts:
encrypt,decryptdata/ciphertext,key,iv,algorithmaesGcmEncryptkey,nonce,aad;noncelength != 12 bytes.datais allowed to be empty (yields the bare 16-byte auth tag, matching CryptoKit).aesGcmDecryptkey,nonce,aad;noncelength != 12 bytes; decodedciphertextWithTag< 16 bytes (auth-tag length). EmptyciphertextWithTagis caught by the same< 16guard.pbkdf2password,salt,algorithm; non-finite / non-integer / non-positive /> INT_MAXcostorlengthhmac256,hmac512data,keysha1,sha256,sha512textrandomKey> INT_MAXlengthAAD is deliberately enforced even though AEAD allows 0-byte AAD — every consumer in our monorepo already supplies an explicit context binding (v2 envelope header bytes; fixed keyless AAD constants), so rejecting empty AAD turns "forgot to pass AAD" bugs into loud failures. See review thread for rationale.
Unified reject code
All native methods (Android + iOS, legacy + GCM, entry-validation failure + impl failure) now reject promises with code
"-1". iOS legacy paths previously used per-method string codes (encrypt_fail/decrypt_fail/keygen_fail/hmac_fail/sha*_fail/uuid_fail/random_fail); those are gone. The reject message still carries the specific context.JS callers that branched on the legacy iOS string codes need to switch to a single
"-1"catch and inspect the message for details.Validation
yarn typecheck✅yarn prepare(react-native-builder-bob module + typescript targets) ✅swiftc -typecheckonAesCryptoGcm.swiftfor bothiphonesimulatorandiphoneosSDKs (arm64-apple-ios15.5-simulator,arm64-apple-ios15.5) ✅, verified in the OneKey app-monorepo worktree.Test plan
pod install+ Xcode build in a consumer appciphertext || tagoutput on iOS / Android against the existing@noble/ciphersJS implementation using fixed vectorscost/length, empty AAD, empty plaintext (must produce 16-byte tag, not reject),ciphertextWithTag< 16 bytes — all expected reject/accept results documented in PR; consumer-sideAESGcmV2Testwill assert them on both platforms before merge.Follow-up
fromHex(CBC/CTR/PBKDF2/HMAC/SHA) usesstrtolwhich silently maps"zz"→0x00, accepts odd-length strings (truncates), and accepts0xprefixes. Asymmetric vs the strict SwiftdataFromHex/ AndroidHex.decode. Tracked separately because tightening it requires re-validating all legacy fixed-vector tests.