Scrub native private-key copies in JS crypto bindings - #8354
Scrub native private-key copies in JS crypto bindings#8354Amaury Chamayou (achamayou) wants to merge 6 commits into
Conversation
Add ccf::crypto::ScopedCleanse RAII guard and use it in js.crypto.generateRsaKeyPair, generateEcdsaKeyPair, generateEddsaKeyPair, pemToJwk, jwkToPem, and sign, so any C++-side copy of private key material (Pem, std::string, std::vector<uint8_t>) is zeroed via OPENSSL_cleanse on every exit path (normal, early return, or exception) rather than only on the straight-line success path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
e7068c6 to
e0fc46b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues leave private-key copies potentially uncleansed on conversion and allocation-failure paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds RAII-based cleansing for temporary private-key material in JavaScript crypto bindings.
Changes:
- Introduces reusable
ScopedCleanse. - Applies cleansing across key-generation, JWK conversion, and signing paths.
- Adds regression tests and changelog documentation.
File summaries
| File | Summary |
|---|---|
src/js/test/js.cpp |
Adds cleansing and crypto regression tests. |
src/js/extensions/ccf/crypto.cpp |
Adds guards, but unguarded Pem copies and to_str allocation failures remain. Moderate findings (2 and 1 votes). |
include/ccf/crypto/scoped_cleanse.h |
Defines the reusable cleansing guard. |
CHANGELOG.md |
Nit (1 vote): narrow the note to the buffers actually cleansed. |
Review details
Suppressed comments (4)
CHANGELOG.md:20
- The note says all private key material handled by these bindings is scrubbed, but both conversion functions still keep private parameters in the local
T jwkstrings, which the PR explicitly leaves unsanitized. Please narrow the release note to the temporary buffers actually guarded so it does not overstate the security coverage.
- Private key material handled by `ccf.crypto.generateRsaKeyPair`, `ccf.crypto.generateEcdsaKeyPair`, `ccf.crypto.generateEddsaKeyPair`, `ccf.crypto.pemToJwk` (and its RSA/EdDSA variants), `ccf.crypto.jwkToPem` (and its RSA/EdDSA variants), and `ccf.crypto.sign` is now scrubbed from memory on all exit paths, including exceptions and early returns. Previously the scrubbing only ran on the successful straight-line path (#8354).
src/js/extensions/ccf/crypto.cpp:944
make_ec_key_pair(key)andmake_rsa_key_pair(key)acceptconst Pem&, so each call implicitly creates an unguardedPemcontaining another copy of the private PEM.key_str_guardonly cleanses the sourcestd::string; the temporary is destroyed without cleansing. Please use a named, guardedPemin these branches (without changing HMAC's arbitrary-string semantics).
ccf::crypto::ScopedCleanse<std::string> key_str_guard(*key_str);
auto& key = *key_str;
src/js/extensions/ccf/crypto.cpp:960
- The
Pemguard is installed only after construction succeeds.Pem(std::string)moves the key into its internal string and thencheck_pem_format()can throw for malformed caller-supplied input, leaving that partially constructed copy to be freed without cleansing. The construction-failure path needs to cleanse thePemstorage before rethrowing, or use a construction path with the same guarantee.
ccf::crypto::Pem key_pem(key);
ccf::crypto::ScopedCleanse<ccf::crypto::Pem> key_pem_guard(key_pem);
src/js/extensions/ccf/crypto.cpp:943
- This guard is installed only after
to_strhas successfully built the optional string.Context::to_strfirst constructsstd::string r(val, len)and frees theJS_ToCStringLenbuffer afterward (src/js/core/context.cpp:617-618); if that allocation throws for a private-key argument, no guard is installed and the buffer containing the key is left allocated and unscrubbed. To cover the claimed exception path, make the conversion own the QuickJS buffer with cleanup/scrubbing on failure (or otherwise guard that buffer before the conversion).
ccf::crypto::ScopedCleanse<std::string> key_str_guard(*key_str);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: eddyashton <6000239+eddyashton@users.noreply.github.com>
Guard native PEM, JWK and JSON copies, including partial JWK conversion and invalid PEM construction. Release QuickJS strings if native copying throws without modifying JS-owned storage. Reuse the private guard for wrap/unwrap and clarify the cleanup boundary in the 7.0.16 release note. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Amaury Chamayou (@achamayou) |
Preserve the historical-state and crypto cleansing regression coverage. Use valid wrapped ciphertext in the buffer-detachment regression and check the recovered plaintext, avoiding the upstream AES-KWP failure-path bug without changing its implementation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Name the public JWK parameters and use non-throwing typed JSON access with native container iteration on the cleanup path. Cover JSON root types and nested values during exception unwinding. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DescriptionComparing 1 available run from this branch (#8354) against the trend of the last 30 Each chart plots every benchmark as an axis, with values normalized so 100 is the EWMA baseline of recent Axis labels show the latest branch value and its difference from the main EWMA baseline, where 0% is on the baseline. They are coloured green where the latest run improves on the baseline, red where it regresses, and grey where the difference is within one std dev of the baseline (within noise). Higher is better for throughput and rate, lower for latency and memory. A benchmark which does not exist on Throughput (tx/s)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#808A94!important}
.radarAxisLabel:nth-of-type(3){fill:#808A94!important}
.radarAxisLabel:nth-of-type(4){fill:#808A94!important}
.radarAxisLabel:nth-of-type(5){fill:#808A94!important}
.radarAxisLabel:nth-of-type(6){fill:#808A94!important}
.radarAxisLabel:nth-of-type(7){fill:#808A94!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b0["Basic Blocking 100ms: 3,055 tx/s ▬ 0%"]
axis b1["Basic Blocking 20ms: 15,146 tx/s ▬ 0%"]
axis b2["Basic Blocking 2ms: 32,416 tx/s ▬ +1%"]
axis b3["Basic JS: 9,232 tx/s ▬ +1%"]
axis b4["Historical Queries: 604,833 tx/s ▬ +2%"]
axis b5["L…g Certificate Blocking: 24,014 tx/s ▬ -2%"]
axis b6["Logging JWT Blocking: 15,059 tx/s ▬ 0%"]
curve stddev2_high["main EWMA + 2 std dev"]{100.39, 100.24, 106.21, 115.56, 107.34, 104.85, 100.89}
curve stddev1_high["main EWMA + 1 std dev"]{100.20, 100.12, 103.11, 107.78, 103.67, 102.43, 100.45}
curve stddev1_low["main EWMA - 1 std dev"]{99.80, 99.88, 96.89, 92.22, 96.33, 97.57, 99.55}
curve stddev2_low["main EWMA - 2 std dev"]{99.61, 99.76, 93.79, 84.44, 92.66, 95.15, 99.11}
curve branch_0["#8354"]{99.98, 99.83, 100.81, 101.24, 101.58, 97.98, 100.15}
graticule polygon
max 127
min 73
ticks 0
showLegend false
Latency (ms)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#808A94!important}
.radarAxisLabel:nth-of-type(3){fill:#808A94!important}
.radarAxisLabel:nth-of-type(4){fill:#808A94!important}
.radarAxisLabel:nth-of-type(5){fill:#808A94!important}
.radarAxisLabel:nth-of-type(6){fill:#808A94!important}
.radarAxisLabel:nth-of-type(7){fill:#808A94!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b0["Basic Blocking 100ms: 99 ms ▬ 0%"]
axis b1["Basic Blocking 20ms: 19 ms ▬ 0%"]
axis b2["Basic Blocking 2ms: 9 ms ▬ 0%"]
axis b3["Basic JS: 32 ms ▬ -2%"]
axis b4["Historical Queries: 51 ms ▬ -3%"]
axis b5["Logging Certificate Blocking: 22 ms ▬ +2%"]
axis b6["Logging JWT Blocking: 20 ms ▬ 0%"]
curve stddev2_high["main EWMA + 2 std dev"]{100.00, 100.00, 103.99, 118.55, 108.52, 106.21, 101.80}
curve stddev1_high["main EWMA + 1 std dev"]{100.00, 100.00, 101.99, 109.28, 104.26, 103.11, 100.90}
curve stddev1_low["main EWMA - 1 std dev"]{100.00, 100.00, 98.01, 90.72, 95.74, 96.89, 99.10}
curve stddev2_low["main EWMA - 2 std dev"]{100.00, 100.00, 96.01, 81.45, 91.48, 93.79, 98.20}
curve branch_0["#8354"]{100.00, 100.00, 99.93, 98.04, 96.77, 102.37, 100.08}
graticule polygon
max 132
min 68
ticks 0
showLegend false
Memory (bytes)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#808A94!important}
.radarAxisLabel:nth-of-type(3){fill:#2DA44E!important}
.radarAxisLabel:nth-of-type(4){fill:#2DA44E!important}
.radarAxisLabel:nth-of-type(5){fill:#808A94!important}
.radarAxisLabel:nth-of-type(6){fill:#808A94!important}
.radarAxisLabel:nth-of-type(7){fill:#808A94!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b0["Basic Blocking 100ms: 91 MiB ▬ 0%"]
axis b1["Basic Blocking 20ms: 90.9 MiB ▬ 0%"]
axis b2["Basic Blocking 2ms: 92.7 MiB ▼ 1%"]
axis b3["Basic JS: 93.1 MiB ▼ 1%"]
axis b4["Historical Queries: 151 MiB ▬ 0%"]
axis b5["Logging Certificate Blocking: 118 MiB ▬ 0%"]
axis b6["Logging JWT Blocking: 91.3 MiB ▬ 0%"]
curve stddev2_high["main EWMA + 2 std dev"]{102.10, 101.90, 101.79, 101.94, 100.95, 101.23, 101.82}
curve stddev1_high["main EWMA + 1 std dev"]{101.05, 100.95, 100.90, 100.97, 100.48, 100.61, 100.91}
curve stddev1_low["main EWMA - 1 std dev"]{98.95, 99.05, 99.10, 99.03, 99.52, 99.39, 99.09}
curve stddev2_low["main EWMA - 2 std dev"]{97.90, 98.10, 98.21, 98.06, 99.05, 98.77, 98.18}
curve branch_0["#8354"]{100.35, 99.74, 98.95, 98.91, 100.14, 99.77, 99.99}
graticule polygon
max 107
min 93
ticks 0
showLegend false
Rate (ops/s)---
config:
radar:
width: 620
height: 620
marginTop: 90
marginRight: 220
marginBottom: 60
marginLeft: 220
axisLabelFactor: 1.12
curveTension: 0.08
theme: base
themeCSS: |
.radarCurve-0{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-1{fill:color-mix(in srgb, #62B5E5 40%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-2{fill:color-mix(in srgb, #62B5E5 13%, var(--color-canvas-default,var(--bgColor-default,#fff)))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarCurve-3{fill:var(--color-canvas-default,var(--bgColor-default,#fff))!important;fill-opacity:1!important;stroke:none!important;stroke-width:0!important}
.radarAxisLabel,.radarTitle{fill:var(--color-fg-default,var(--fgColor-default,#111827))!important;color:var(--color-fg-default,var(--fgColor-default,#111827))!important}
.radarCurve-4{stroke-width:1.75px!important;stroke-opacity:1.00!important}
.radarAxisLabel:nth-of-type(1){fill:#808A94!important}
.radarAxisLabel:nth-of-type(2){fill:#2DA44E!important}
.radarAxisLabel:nth-of-type(3){fill:#808A94!important}
.radarAxisLabel:nth-of-type(4){fill:#808A94!important}
.radarAxisLabel:nth-of-type(5){fill:#808A94!important}
.radarAxisLabel:nth-of-type(6){fill:#2DA44E!important}
themeVariables:
cScale0: "#62B5E5"
cScale1: "#62B5E5"
cScale2: "#62B5E5"
cScale3: "#62B5E5"
cScale4: "#F97316"
radar:
axisColor: "#9CA3AF"
graticuleColor: "#E5E7EB"
graticuleOpacity: 0
axisStrokeWidth: 1
curveOpacity: 0
---
radar-beta
axis b0["CHAMP get: 37,925,926 ops/s ▬ 0%"]
axis b1["CHAMP put: 5,565,172 ops/s ▲ 1%"]
axis b2["KV deserialisation: 1,634,254 ops/s ▬ 0%"]
axis b3["KV serialisation: 1,377,410 ops/s ▬ -1%"]
axis b4["KV s…t deserialisation: 4,215 ops/s ▬ 0%"]
axis b5["KV snapshot serialisation: 4,837 ops/s ▲ 9%"]
curve stddev2_high["main EWMA + 2 std dev"]{101.24, 101.63, 102.14, 101.97, 101.89, 108.07}
curve stddev1_high["main EWMA + 1 std dev"]{100.62, 100.82, 101.07, 100.98, 100.95, 104.03}
curve stddev1_low["main EWMA - 1 std dev"]{99.38, 99.18, 98.93, 99.02, 99.05, 95.97}
curve stddev2_low["main EWMA - 2 std dev"]{98.76, 98.37, 97.86, 98.03, 98.11, 91.93}
curve branch_0["#8354"]{99.76, 101.07, 99.78, 99.46, 100.16, 108.96}
graticule polygon
max 115
min 85
ticks 0
showLegend false
|
Follow-up to #8349, now integrated from
main.Changes
ccf::js::ScopedCleansehelper insrc/js/extensions/ccf/scoped_cleanse.h, next to its users. Reuse it for the wrap/unwrap buffers introduced in Copy ArrayBuffer arguments before re-entering JS in crypto and attestation bindings #8349 instead of retaining a duplicate local guard. The public RSA wrapping-key PEM remains unchanged.Context::to_str, including when allocating the native string fails. Keep embedded-NUL handling unchanged.Design and limits
ScopedCleanseis non-copyable/non-movable, references an object that must outlive it, and callsOPENSSL_cleansefor byte containers, private JWK fields and JSON string values. Guards are installed immediately after creating the native value, or before populating it where possible.This cleans the explicitly owned native values above on scope exit, including early returns and exception unwinding. It is not a guarantee that every representation of a private key is erased.
JavaScript-owned strings are deliberately untouched:
JS_ToCStringLencan return storage shared with a live JS string, so cleansing that buffer would mutate caller-owned data. Internal library temporaries, including parser/serializer scratch allocations and partially constructed library return values, are outside this change. The changelog states these limits explicitly.The public
Pemand JWK types do not gain scrubbing destructors.Pemalso represents public certificates and keys throughout CCF, so changing its general destruction semantics remains outside this PR. Private JWK members owned by the bindings are now scrubbed using the private helper rather than changing the public types. No JS-visible result or error behavior is intentionally changed.Regression coverage
src/js/test/js.cppcovers byte-container cleansing, all private JWK fields, partially deserialized JWKs on exception unwinding, and nested JSON strings on early return. Bytes are inspected while their containers remain alive; the tests do not read freed memory.Binding coverage via
CommonContextincludes all three key-generation paths, all three private/public PEM-JWK round trips, ECDSA/EdDSA/RSA-PSS signing and verification, an HMAC known-answer case with a non-PEM key, malformed PEM/JWK rejection, and preservation of caller-owned values.Local validation
For the updated revision:
cmake -S . -B build -GNinjacmake --build build --target js_test crypto_test -j 4ctest --test-dir build --output-on-failure -R '^(js_test|crypto_test)$' --no-tests=error: both targets passed.scripts/ci-checks.sh: all 15 checks passed.These runs used RelWithDebInfo and Clang 21. The original revision also had an ASAN run, which reported a pre-existing leak in the QuickJS OOM-backtrace test; ASAN was not rerun for this update. CI remains required before merge.