Bug 1555108
Summary: | AES isn't clearing the key information in it's context when finished. | ||||||
---|---|---|---|---|---|---|---|
Product: | Red Hat Enterprise Linux 7 | Reporter: | Bob Relyea <rrelyea> | ||||
Component: | nss-softokn | Assignee: | nss-nspr-maint <nss-nspr-maint> | ||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | BaseOS QE Security Team <qe-baseos-security> | ||||
Severity: | high | Docs Contact: | |||||
Priority: | high | ||||||
Version: | 7.0 | CC: | dueno, kengert, mthacker, nmavrogi, nss-nspr-maint, omoris, salmy, tmraz, toneata | ||||
Target Milestone: | rc | Keywords: | ZStream | ||||
Target Release: | 7.5 | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | If docs needed, set a value | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | |||||||
: | 1556910 (view as bug list) | Environment: | |||||
Last Closed: | 2018-10-30 15:44:53 UTC | Type: | Bug | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 1472750, 1556910 | ||||||
Attachments: |
|
Description
Bob Relyea
2018-03-14 00:28:05 UTC
Created attachment 1407755 [details]
Patch to zero the context (which key information) when finished with them.
void AES_DestroyContext(AESContext *cx, PRBool freeit) { + void *mem = cx->mem; if (cx->worker_cx && cx->destroy) { (*cx->destroy)(cx->worker_cx, PR_TRUE); cx->worker_cx = NULL; cx->destroy = NULL; } + PORT_Memset(cx, 0, sizeof(AESContext)); if (freeit) { - PORT_Free(cx->mem); + PORT_Free(mem); } } The code seems correct, and I agree it clears the context structures. But I have two suggestions/questions: Q1: The code doesn't clear the contents of cx->mem. Should it? But that would require to know the size of that block. Is it known? Q2: The helper variable "mem" seems unnecessary. Why not simply place the memset call as the very last line in this function? I explained this to kai verbally, but it's worth documenting. cx is contained in mem: mem-> +---------+ | | . . . . . . | | cx-> +---------+ | | | | . . . . | cx->mem | . . . . | | end of cx +---------+ | | . . . . | | end of mem +---------+ This is done for memory alignment purposes. So space is allocated sizeof(cx) + sizeof(alignment) and assigned to mem. cx is set to the aligned pointer within mem, and cx->mem is set to mem. cx is used as the active context (the extra space in mem is not touched. This was all added by mozilla in the last year to speed up AES performance. So the order in the patch is important. 1. We need to save mem before we clear the cx (because cx->mem would be cleared and we can't reference it later). 2. We have to clear the cx before we free mem, (because cx is embedded in the mem block and freeing mem frees cx) 3. Finally with mem saved and cx cleared we can safely free mem. |