Skip to content

fix(vault): replace hand-rolled XOR+HMAC with real AES-GCM (#302) - #1714

Closed
aaniya22 wants to merge 2 commits into
utksh1:mainfrom
aaniya22:fix/vaultcrypto-real-aes-gcm-302
Closed

fix(vault): replace hand-rolled XOR+HMAC with real AES-GCM (#302)#1714
aaniya22 wants to merge 2 commits into
utksh1:mainfrom
aaniya22:fix/vaultcrypto-real-aes-gcm-302

Conversation

@aaniya22

@aaniya22 aaniya22 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Description

VaultCrypto previously encrypted secrets using a hand-rolled scheme: a SHA-256-derived keystream XOR'd against the plaintext, authenticated with a separate HMAC. Because the keystream was cycled to cover plaintexts longer than one block, any secret over 32 bytes reused keystream bytes — a crib-dragging vulnerability if an attacker could influence or guess part of the plaintext.

This PR replaces that scheme with AES-256-GCM (via the cryptography library):

  • VaultCrypto now uses AES-256-GCM for authenticated encryption instead of a custom XOR keystream + HMAC scheme, closing a crib-dragging vulnerability on plaintext longer than one keystream block.
  • Key material is SHA-256 hashed to a fixed 32 bytes, so any input key length works safely for AES-256.
  • Ciphertext from the old XOR+HMAC format is now rejected loudly on decrypt (raises ValueError) rather than silently producing garbage.
  • Consolidated a duplicate test_vault.py (one had been added at testing/backend/test_vault.py, colliding with the existing file at testing/backend/unit/test_vault.py) and merged in new regression tests: long-plaintext round-trip, nonce non-determinism, tamper detection, wrong-key rejection, and old-format rejection.

Note: this is a breaking change for data at rest — secrets encrypted under the old format cannot be decrypted with the new code and must be re-encrypted as part of deploying this change.

Related Issues

Closes #302

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

Ran the full backend suite (python -m pytest testing/backend -v). All 6 tests in testing/backend/unit/test_vault.py pass, covering:

  • Basic encrypt/decrypt round-trip
  • Round-trip on plaintext longer than one AES block (regression test for the original vulnerability)
  • Non-determinism (same plaintext encrypts to different ciphertext each time, due to fresh nonces)
  • Tampered ciphertext is rejected
  • Wrong key cannot decrypt
  • Ciphertext in the old XOR+HMAC format is rejected rather than silently corrupted

32 unrelated failures exist elsewhere in the suite (plugin/tooling integration tests, e.g. test_phase2_plugins.py, test_phase3_plugins.py, test_plugins.py). Verified these fail identically on unmodified main, so they're pre-existing environment/tooling issues (likely missing external CLI tools) unrelated to this change.

To reproduce: python -m pytest testing/backend/unit/test_vault.py -v

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Moving from hand-rolled crypto to AES-GCM is the right direction, but this revision regresses existing vault behavior:\n\n1. Preserve compatibility with the current vault key contract and keep the existing fingerprint/diagnostics behavior intact.\n2. Do not remove unrelated dependencies from backend/requirements.txt (for example httpx / openai) as part of this PR.\n3. Keep the crypto migration focused and regression-safe.\n\nPlease revise this to preserve current diagnostics/contracts while upgrading the crypto implementation.

@utksh1 utksh1 added type:security Security work category bonus label area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests level:advanced 55 pts difficulty label for advanced contributor PRs type:bug Bug fix work category bonus label labels Jul 7, 2026
aaniya22 added 2 commits July 11, 2026 12:19
- VaultCrypto now uses AES-256-GCM for authenticated encryption instead
  of a custom XOR keystream + HMAC scheme, closing a crib-dragging
  vulnerability on plaintext longer than one keystream block
- Key material is SHA-256 hashed to a fixed 32 bytes, so any input
  key length works safely for AES-256
- Ciphertext from the old XOR+HMAC format is now rejected loudly on
  decrypt (raises ValueError) rather than silently producing garbage
- Consolidated a duplicate test_vault.py (one had been added at
  testing/backend/test_vault.py, colliding with the existing file at
  testing/backend/unit/test_vault.py) and merged in new regression
  tests: long-plaintext round-trip, nonce non-determinism, tamper
  detection, wrong-key rejection, and old-format rejection
@aaniya22
aaniya22 force-pushed the fix/vaultcrypto-real-aes-gcm-302 branch from eb7c31b to 463d43b Compare July 11, 2026 06:56
@aaniya22

Copy link
Copy Markdown
Contributor Author

Moving from hand-rolled crypto to AES-GCM is the right direction, but this revision regresses existing vault behavior:\n\n1. Preserve compatibility with the current vault key contract and keep the existing fingerprint/diagnostics behavior intact.\n2. Do not remove unrelated dependencies from backend/requirements.txt (for example httpx / openai) as part of this PR.\n3. Keep the crypto migration focused and regression-safe.\n\nPlease revise this to preserve current diagnostics/contracts while upgrading the crypto implementation.

upstream/main already has an equivalent (and more complete) AES-GCM
implementation merged, including the key_fingerprint diagnostics property this PR
was missing.rebasing my branch onto upstream/main showed vault.py and test_vault.py now match
upstream exactly with no meaningful diff left, so there's nothing left to contribute
here.
thanks for the review

@utksh1

utksh1 commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Closing this because the author confirmed the AES-GCM vault change is already present on main and there is no meaningful diff left to merge.

@utksh1 utksh1 closed this Jul 13, 2026
@utksh1 utksh1 added the gssoc:invalid Admin validation: invalid for GSSoC scoring label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests gssoc:invalid Admin validation: invalid for GSSoC scoring level:advanced 55 pts difficulty label for advanced contributor PRs type:bug Bug fix work category bonus label type:security Security work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security]: [GSSoC26] feat: VaultCrypto uses weak XOR stream cipher with 32-byte repeating key

2 participants