Skip to content

fix(security): 4 CRITICAL vulnerability fixes#16

Merged
tsavo-at-pieces merged 3 commits intomainfrom
feat/enterprise-byok-runtime-ci-sync
Feb 24, 2026
Merged

fix(security): 4 CRITICAL vulnerability fixes#16
tsavo-at-pieces merged 3 commits intomainfrom
feat/enterprise-byok-runtime-ci-sync

Conversation

@tsavo-at-pieces
Copy link

Summary

  • Fernet HMAC timing attack: Replace ListEquality().equals() with constant-time XOR-accumulation comparison to prevent timing side-channel attacks
  • RSA signature timing attack: Use constant-time XOR comparison for equal-length signature verification path
  • PBKDF2 default iterations: Raise from 100 to 600,000 per OWASP PBKDF2-HMAC-SHA1 recommendations
  • ECB mode deprecation: Mark AESMode.ecb as @Deprecated — encrypts blocks independently, leaking plaintext patterns

Test plan

  • Verify existing Fernet encrypt/decrypt tests still pass with new constant-time comparison
  • Verify RSA sign/verify tests still pass with XOR-accumulation path
  • Verify Key.stretch() uses 600k iterations by default
  • Verify @Deprecated warning appears when using AESMode.ecb

Ref: open-runtime/aot_monorepo#391

🤖 Generated with Claude Code

tsavo-at-pieces and others added 2 commits February 22, 2026 22:15
Apply runtime_ci workflow/template updates and align pubspec dependency metadata for enterprise BYOK prep.
…te ECB

- Fernet: replace ListEquality with XOR-accumulation for HMAC verification
  to prevent timing side-channel attacks
- RSA: use constant-time XOR comparison for equal-length signature verification
- PBKDF2: raise default iterations from 100 to 600,000 per OWASP recommendation
  for PBKDF2-HMAC-SHA1
- AES: mark ECB mode @deprecated — encrypts blocks independently, leaking
  plaintext patterns; use GCM or CBC instead

ref #391

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 23, 2026 23:13
@cursor
Copy link

cursor bot commented Feb 23, 2026

PR Summary

Medium Risk
Touches cryptographic verification and key-derivation defaults; mistakes could cause auth/signature/token verification failures or performance regressions, though changes are localized and straightforward.

Overview
Hardens crypto primitives by switching Fernet HMAC verification and RSA signature verification (equal-length path) to constant-time comparisons to reduce timing side-channels.

Strengthens key derivation defaults by increasing Key.stretch() PBKDF2 iterations from 100 to 600,000 (OWASP-aligned) and deprecates insecure AESMode.ecb with guidance to use safer modes.

Updates CI/tooling config: bumps runtime_ci_tooling to ^0.11.0, regenerates CI workflow to run dart analyze/dart test directly, adds a dart format check, and adjusts git URL token rewrite + LFS smudge handling via env/config.

Written by Cursor Bugbot for commit 5dd3bd3. Configure here.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

if grep -q "^ error -" /tmp/analysis.txt; then
echo "::error::Analysis errors found"
exit 1
fi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI analyze step may miss failures

High Severity

The Analyze step pipes dart analyze into tee without pipefail, so dart analyze’s non-zero exit code can be masked. The follow-up grep for ^ error - is format-dependent and may not match real analyzer errors, allowing analysis failures to pass CI.

Fix in Cursor Fix in Web

/// The [iterationCount] defaults to 600,000 per OWASP recommendations for
/// PBKDF2-HMAC-SHA1. Passing a value below 600,000 is strongly discouraged
/// as it weakens resistance to brute-force attacks.
Key stretch(int desiredKeyLength, {int iterationCount = 600000, Uint8List? salt}) {

This comment was marked as outdated.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements critical security fixes addressing timing attack vulnerabilities in HMAC verification and RSA signature verification, updates PBKDF2 iteration count to modern standards, and deprecates the insecure ECB encryption mode. Additionally, it upgrades the CI tooling infrastructure to version 0.11.0.

Changes:

  • Implemented constant-time comparison using XOR-accumulation in Fernet HMAC verification and RSA signature verification to prevent timing side-channel attacks
  • Increased PBKDF2 default iteration count from 100 to 600,000 per OWASP recommendations
  • Deprecated AESMode.ecb with clear warning about security implications

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lib/src/algorithms/fernet.dart Added constant-time _constantTimeEquals() method to replace ListEquality().equals() for HMAC verification
lib/src/algorithms/rsa.dart Replaced short-circuiting comparison with XOR-accumulation for constant-time signature verification
lib/src/encrypted.dart Increased PBKDF2 default iteration count from 100 to 600,000 with updated documentation
lib/src/algorithms/aes.dart Added @Deprecated annotation to ECB mode with security warning and alternative recommendations
pubspec.yaml Updated runtime_ci_tooling from ^0.10.0 to ^0.11.0 and removed workspace resolution comment
.runtime_ci/config.json Added new "ci" configuration section for tooling v0.11.0
.runtime_ci/template_versions.json Updated tooling version to 0.11.0 with new template hashes and timestamps
.github/workflows/ci.yaml Regenerated CI workflow with direct dart commands, format checking, and user extension points

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 117 to 120
/// The [iterationCount] defaults to 600,000 per OWASP recommendations for
/// PBKDF2-HMAC-SHA1. Passing a value below 600,000 is strongly discouraged
/// as it weakens resistance to brute-force attacks.
Key stretch(int desiredKeyLength, {int iterationCount = 600000, Uint8List? salt}) {
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the default iteration count from 100 to 600,000 is a breaking change that will cause the existing test 'Key.strech' in test/encrypted_test.dart to fail. The test expects the output 'ykT8qFmrPp7TJyzY+E2NoBNjfWymzKOs1OCbRsO67fo=' which was calculated using 100 iterations. The test needs to be updated to either pass iterationCount: 100 explicitly to preserve the old behavior, or the expected hash needs to be recalculated with 600,000 iterations.

Suggested change
/// The [iterationCount] defaults to 600,000 per OWASP recommendations for
/// PBKDF2-HMAC-SHA1. Passing a value below 600,000 is strongly discouraged
/// as it weakens resistance to brute-force attacks.
Key stretch(int desiredKeyLength, {int iterationCount = 600000, Uint8List? salt}) {
/// The [iterationCount] defaults to 100 for backward compatibility with
/// existing code. A higher value (for example, 600,000 or more) is
/// strongly recommended to improve resistance to brute-force attacks.
Key stretch(int desiredKeyLength, {int iterationCount = 100, Uint8List? salt}) {

Copilot uses AI. Check for mistakes.
@tsavo-at-pieces
Copy link
Author

Upstream PR Dependency Compatibility Review

Reviewed two open PRs on the archived upstream (leocavalcante/encrypt) for dependency compatibility with the open-runtime monorepo:


PR leocavalcante#343 — "feat: supports pointycastle 4.0.0" (by @alextekartik, branch 5.x)

Change: Widens pointycastle constraint from ^3.7.3 to >=3.7.3 <5.0.0 in pubspec.yaml.

Downstream Impact

No impact on our fork. open-runtime/encrypt 6.0.0+ already uses pointycastle: ^4.0.0. The monorepo's pubspec.lock resolves pointycastle at 4.0.0 across all consumers (runtime_native_io_core, provisioning, gpt inference). This upstream PR merely catches the archived upstream up to what we already ship.

Version Constraints

  • pointycastle 4.0.0 has no documented API breaking changes relative to 3.x (UTF-16 support, new OID, null-safety fix, dependency cleanup only).
  • The range >=3.7.3 <5.0.0 is a safe backward-compatible widening.
  • Our fork's ^4.0.0 is more forward-looking and appropriate since we don't need to maintain 3.x compat.

Verdict: Already superseded by our fork. No action needed.


PR leocavalcante#341 — "Add GCM to AES supported modes section in README" (by @probabilityhill)

Change: Adds - GCM \AESMode.gcm`to the "Supported modes" list inREADME.md`.

Downstream Impact

None — documentation-only change. GCM mode already exists in the AESMode enum in both the upstream package and our fork. The PR simply documents an existing capability that was missing from the README.

Version Constraints

No pubspec changes. Zero risk.

Verdict: Correct documentation fix. Our fork's README already lists GCM. No action needed.


Summary

Both upstream PRs are on the archived leocavalcante/encrypt repo and cannot be merged there. Neither PR introduces changes that our fork needs to adopt — we are already ahead on both fronts:

No dependency compatibility concerns for the monorepo.

- Merge main (v0.11.2 CI, release bot v6.0.1/v6.0.2 artifacts)
- Resolve CI yaml conflict: keep main's v0.11.2 generated version
- Resolve template_versions.json conflict: keep main's v0.11.2 hashes
- Fix Key.stretch test: pass explicit iterationCount: 100 for legacy test
- Add new test verifying 600k default produces different output

Addresses review comments from Sentry and Copilot bots about breaking
PBKDF2 default change.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@tsavo-at-pieces
Copy link
Author

Deep Review & Conflict Resolution

Merge Conflicts — Resolved ✅

Merged main into the PR branch. Conflicts were in:

  • .github/workflows/ci.yaml → Kept main's v0.11.2 generated version (supersedes PR's v0.11.0)
  • .runtime_ci/template_versions.json → Kept main's v0.11.2 hashes
  • pubspec.yaml → Auto-merged cleanly (version bumped to 6.0.2, runtime_ci_tooling ^0.11.0 preserved)

Also pulled in main's release bot artifacts (v6.0.1, v6.0.2 audit/release notes).


Review Comment Responses

1. @cursor — CI analyze step pipefail (HIGH)

Verdict: Valid concern, but not actionable in this PR.

The dart analyze | tee without set -o pipefail is a known limitation of the generated CI template (runtime_ci_tooling). The grep fallback catches most error formats, but a pipefail-safe version would be more robust. Since ci.yaml is generated by tooling (tracked via template_versions.json hashes), the fix belongs in runtime_ci_tooling, not in a hand-edit here.

Action: Filed mentally for next runtime_ci_tooling update. No change in this PR.

2. @sentry — PBKDF2 600k default is breaking (CRITICAL)

Verdict: Valid. Fixed in this commit.

The default iterationCount change from 100→600,000 is intentionally breaking (security hardening per OWASP). The test was using the default and had a hardcoded expected value from 100 iterations.

Fix applied:

  • Updated existing test to explicitly pass iterationCount: 100 (preserves deterministic assertion)
  • Added new test verifying the 600k default produces a different (stronger) key
  • Both tests pass ✅ (600k test takes ~13s as expected for PBKDF2-HMAC-SHA1)

3. @copilot — Suggests reverting to 100 default

Verdict: Disagree. 600k is the correct security default.

Copilot's suggestion to keep iterationCount: 100 as default "for backward compatibility" would defeat the purpose of this security fix. 100 iterations of PBKDF2-HMAC-SHA1 is trivially brute-forceable. OWASP recommends 600,000 minimum. The breaking change is documented and the test is fixed to handle both cases.


Security Fixes Review — All 4 Correct ✅

Fix Assessment
Fernet HMAC constant-time Correct. XOR-accumulation prevents timing side-channel. _constantTimeEquals is textbook.
RSA signature constant-time Correct. Equal-length path now uses XOR-accumulation. Note: the unequal-length path still has early returns — acceptable since length difference is not secret.
PBKDF2 600k default Correct per OWASP. Test updated.
ECB @deprecated Correct. ECB leaks plaintext patterns (penguin problem). Guidance to use GCM/CBC is appropriate.

pointycastle Version

Already at ^4.0.0 which is the latest (4.0.0). No bump needed.

All Tests Pass

00:13 +51: All tests passed!

Copy link

Copilot AI commented Feb 24, 2026

@tsavo-at-pieces I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you.

@tsavo-at-pieces tsavo-at-pieces merged commit b883ebe into main Feb 24, 2026
3 checks passed
tsavo-at-pieces pushed a commit that referenced this pull request Feb 24, 2026
## Changelog

## [7.0.0] - 2026-02-24

### Breaking Changes
- **BREAKING**: PBKDF2 iteration count increased from 100 to 600,000 by default (#16) (fixes #16)
  - Migration: Default `Key.stretch` behavior → explicitly set `iterationCount: 100` for legacy derivation
- **BREAKING**: Strict validation for AES key and IV lengths has been added (#18) (fixes #18)
  - Migration: Unvalidated AES keys/IVs → keys must be 16, 24, or 32 bytes; IVs 12 bytes (GCM) or 16 bytes (others)

### Added
- Added `base64Url` getter to `SecureRandom` for URL-safe encoding (#18) (fixes #18)
- Added auto-format job to CI workflow and updated config (#16) (fixes #16)

### Changed
- Raised `Key.stretch` (PBKDF2) default iterations from 100 to 600,000 per OWASP recommendations (#16) (fixes #16)
- Updated `runtime_ci_tooling` dependency to `^0.11.0`

### Deprecated
- Deprecated insecure AES ECB mode that leaks plaintext patterns -- will be removed in v8.0.0 (#16) (fixes #16)

### Fixed
- Validated AES key lengths (16/24/32 bytes) and IV lengths (12 bytes for GCM, 16 for others) to prevent misuse (#18) (fixes #18)

### Security
- Implemented constant-time XOR-accumulation comparison for Fernet HMAC verification to fix timing side-channel attacks (#16) (fixes #16)
- Used constant-time XOR comparison for equal-length RSA signature verification to fix timing side-channel attacks (#16) (fixes #16)

## Files Modified

```
.../audit/v7.0.0/explore/breaking_changes.json     | 23 +++++
 .../audit/v7.0.0/explore/commit_analysis.json      | 58 +++++++++++++
 .runtime_ci/audit/v7.0.0/explore/pr_data.json      | 20 +++++
 .runtime_ci/audit/v7.0.0/meta.json                 | 82 ++++++++++++++++++
 .../v7.0.0/version_analysis/version_bump.json      |  1 +
 .../version_analysis/version_bump_rationale.md     | 21 +++++
 .runtime_ci/autodoc.json                           |  8 +-
 .../release_notes/v7.0.0/changelog_entry.md        | 25 ++++++
 .runtime_ci/release_notes/v7.0.0/contributors.json |  5 ++
 .runtime_ci/release_notes/v7.0.0/highlights.md     |  4 +
 .../release_notes/v7.0.0/linked_issues.json        | 32 +++++++
 .../release_notes/v7.0.0/migration_guide.md        | 67 +++++++++++++++
 .runtime_ci/release_notes/v7.0.0/release_notes.md  | 99 ++++++++++++++++++++++
 .../release_notes/v7.0.0/release_notes_body.md     | 99 ++++++++++++++++++++++
 .runtime_ci/version_bumps/v7.0.0.md                | 21 +++++
 CHANGELOG.md                                       | 27 ++++++
 README.md                                          |  2 +
 pubspec.yaml                                       |  2 +-
 18 files changed, 591 insertions(+), 5 deletions(-)
```

## Version Bump Rationale

# Version Bump Rationale

**Decision**: major

## Key Changes
- **Security**: Added constant-time XOR comparison in Fernet HMAC verification and RSA signature verification to prevent timing side-channel attacks.
- **Security**: Raised the default PBKDF2 iteration count in `Key.stretch` from 100 to 600,000 to align with OWASP recommendations.
- **Security**: Added strict validation for AES key lengths (16, 24, or 32 bytes) and IV lengths (12 bytes for GCM, 16 bytes for others).
- **Deprecation**: Deprecated `AESMode.ecb` as it is insecure and leaks plaintext patterns.
- **CI**: Updated CI workflows to run auto-formatting and handle bot commits more cleanly.

## Breaking Changes
- The default `iterationCount` for `Key.stretch` has been changed from `100` to `600000`. This is a breaking change because calling `Key.stretch` without specifying the iteration count will now produce a different derived key. Users relying on the old default to decrypt existing data must update their code to exp...

## Contributors

- @tsavo-at-pieces

---
Automated release by CI/CD pipeline (Gemini CLI + GitHub Actions)
Commits since v6.0.2: 4
Generated: 2026-02-24T01:09:15.979476Z
@tsavo-at-pieces tsavo-at-pieces deleted the feat/enterprise-byok-runtime-ci-sync branch February 24, 2026 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants