Skip to content

Commit a7524cb

Browse files
mcollinaaduh95
authored andcommitted
crypto: prevent Hmac.digest() from returning uninitialized memory
Hmac.prototype._flush was aliased to Hash.prototype._flush, which finalizes the native HMAC context but never sets the JavaScript-side kFinalized flag. After an Hmac has been used as a stream, a subsequent Hmac.prototype.digest() call therefore still believes the object has not been finalized and calls into C++ a second time. On that second call the native context has already been reset, so the digest buffer is never written and Digest::MAX_SIZE bytes of uninitialized stack memory are returned to JavaScript. Hash is not affected because Hash::HashDigest caches its digest (refs #28245); Hmac never received the equivalent protection. Give Hmac its own _flush that sets kFinalized so repeat digest() calls after stream use are handled by the existing DEP0206 guard. As defense in depth, also set buf.len = 0 on the native side when the context has already been reset so unwritten bytes can never be emitted. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #65112 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
1 parent 8d8dca0 commit a7524cb

3 files changed

Lines changed: 29 additions & 1 deletion

File tree

lib/internal/crypto/hash.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,12 @@ Hmac.prototype.digest = function digest(outputEncoding) {
245245
return ret;
246246
};
247247

248-
Hmac.prototype._flush = Hash.prototype._flush;
248+
Hmac.prototype._flush = function _flush(callback) {
249+
this.push(this[kHandle].digest());
250+
this[kState][kFinalized] = true; // This diverges from Hash.prototype._flush:
251+
// Hash instances are still usable after a flush, Hmac are not, see DEP0206.
252+
callback();
253+
};
249254
Hmac.prototype._transform = Hash.prototype._transform;
250255

251256
// Implementation for WebCrypto subtle.digest()

src/crypto/crypto_hmac.cc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,9 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
141141
return ThrowCryptoError(env, ERR_get_error(), "Failed to finalize HMAC");
142142
}
143143
hmac->ctx_.reset();
144+
} else {
145+
// The context has already been finalized; never emit unwritten bytes.
146+
buf.len = 0;
144147
}
145148

146149
Local<Value> ret;

test/parallel/test-crypto-hmac.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,26 @@ for (let i = 0, l = rfc4231.length; i < l; i++) {
312312
}
313313
}
314314

315+
// Calling digest() after the Hmac has already been used as a stream must
316+
// return an empty buffer (the DEP0206 repeat-digest guard), not uninitialized
317+
// stack memory. The stream itself must still produce the correct digest.
318+
// See: https://github.com/nodejs/node/issues/28245
319+
{
320+
const key = 'key';
321+
const data = 'some data to hash';
322+
323+
const streamHmac = crypto.createHmac('sha256', key);
324+
streamHmac.end(data);
325+
const streamDigest = streamHmac.read();
326+
327+
// digest() after the stream already finalized must not return garbage.
328+
assert.deepStrictEqual(streamHmac.digest(), Buffer.alloc(0));
329+
330+
// Sanity check: the stream itself produced the correct digest.
331+
const expected = crypto.createHmac('sha256', key).update(data).digest();
332+
assert.deepStrictEqual(streamDigest, expected);
333+
}
334+
315335
// Test HMAC-MD5/SHA1 (rfc 2202 Test Cases)
316336
const rfc2202_md5 = [
317337
{

0 commit comments

Comments
 (0)