Closed Bug 1460409 Opened 2 years ago Closed 2 years ago

Potential data leak in blake2b_Begin()

Categories

(NSS :: Libraries, enhancement, P3)

3.34.1
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozillabugs, Assigned: franziskus)

Details

(Keywords: sec-low)

Attachments

(1 file)

46 bytes, text/x-phabricator-request
ttaubert
: review+
Details | Review
blake2b_Begin() (security\nss\lib\freebl\blake2b.c) fails to zero its |ctx| argument on failure, thus potentially leaking data. The code clearly intends to do this, but zeroes a local pointer to the context instead. The bug is on line 183, which should read something like:

	if (ctx != nullptr) {
		PORT_Memset(ctx, 0, sizeof(*ctx));
	}

146: static SECStatus
147: blake2b_Begin(BLAKE2BContext* ctx, uint8_t outlen, const uint8_t* key,
148:               size_t keylen)
149: {
150:     PORT_Assert(ctx != NULL);
151:     if (!ctx) {
152:         goto failure;
153:     }
154:     if (outlen == 0 || outlen > BLAKE2B512_LENGTH) {
155:         goto failure;
156:     }
157:     if (key && keylen > BLAKE2B_KEY_SIZE) {
158:         goto failure;
159:     }
160:     /* Note: key can be null if it's unkeyed. */
161:     if ((key == NULL && keylen > 0) || keylen > BLAKE2B_KEY_SIZE ||
162:         (key != NULL && keylen == 0)) {
163:         goto failure;
164:     }
...
179: 
180:     return SECSuccess;
181: 
182: failure:
183:     PORT_Memset(&ctx, 0, sizeof(ctx));
184:     PORT_SetError(SEC_ERROR_INVALID_ARGS);
185:     return SECFailure;
186: }
Flags: needinfo?(franziskuskiefer)
Getting in there would actually smash the stack. So yes this is a bug. Looks like we don't have a test for this so no one noticed. I'll add a test fix it.

This isn't really a security issue at the moment as this function can't be used outside of NSS and is not used within NSS.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(franziskuskiefer)
Keywords: sec-low
Priority: -- → P3
This case is exercised in `OverlongKeyTest`. We can't really test that the context is zeroed.
Comment on attachment 8975437 [details]
Bug 1460409 - free it all

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D1271
Attachment #8975437 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/6e4b0141df2f33fcb75bc02811110aab8e0af501
Assignee: nobody → franziskuskiefer
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.38
Group: crypto-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.