If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use safe memory cleanup to avoid removal of memset by VC++2010 /O2

NEW
Unassigned

Status

NSS
Libraries
5 years ago
a month ago

People

(Reporter: Masahiro YAMADA, Unassigned)

Tracking

({sec-audit})

trunk
x86
Windows XP
sec-audit

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 years ago
Unfortunatelly, VC++2010 with /O2 sometimes removes memset for memory cleanup.
It may be needed to use safe memory clear function to delete sensitive information.

Tor is updated by this problem.

  http://www.h-online.com/security/news/item/Security-issue-discovered-in-TOR-client-Update-1746884.html
  http://www.viva64.com/en/b/0178/

For example : /security/nss/lib/freebl/camellia.c Camellia_DestroyContext()

According to about:buildconfig, Official Release of Firefox 16 uses /O1, not /O2.
This is reason whiy is think this is not critical bug.
(Reporter)

Comment 1

5 years ago
FYI: discussion on Tor project.
https://trac.torproject.org/projects/tor/ticket/7352
(Reporter)

Comment 2

5 years ago
These codes seems to be vulnerable. (other codes may have same problem)

http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/certutil/keystuff.c#115
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/sha512.c#1349
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/pqg.c#426
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/md5.c#209
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/drbg.c#451
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/mpi/mpmontg.c#1178


# sha512.c contains many "memset" that seemes to be removed by compiler optimization.
I'm going to move this bug to NSS given the referenced source links, but we might be relying on this behavior elsewhere in Gecko as well.
Assignee: nobody → nobody
Component: Security → Libraries
Product: Core → NSS
Version: Trunk → trunk
(Reporter)

Comment 4

5 years ago
MXR search list:
http://mxr.mozilla.org/mozilla-central/ident?i=PORT_Memset
http://mxr.mozilla.org/mozilla-central/ident?i=memset
http://mxr.mozilla.org/mozilla-central/ident?i=zmemzero

zmemzero seems to be used in safe context.
but PORT_memset is sometimes used in unsafe context.

http://hg.mozilla.org/mozilla-central/file/437dcecc6377/security/nss/lib/pk11wrap/pk11merge.c#l293
(Reporter)

Comment 5

5 years ago
VC++ also removes ZeroMemory() if it seems to be removable.
http://mxr.mozilla.org/mozilla-central/ident?i=ZeroMemory

Comment 6

5 years ago
It appears that the issue in question merely removes "defense in depth" memset when the compiler can optimize it away. If that is the case, I don't think there's any point in leaving this bug closed.
This is a pretty well known issue (e.g. https://www.securecoding.cert.org/confluence/display/cplusplus/MSC06-CPP.+Be+aware+of+compiler+optimization+when+dealing+with+sensitive+data).
Does this need to be hidden?
Keywords: sec-audit

Updated

5 years ago
Flags: needinfo?(bsmith)
I am opening up the bug.

FWIW, some code doesn't even memset at all, e.g. AESKeyWrap_DestroyContext.
Group: core-security
Flags: needinfo?(bsmith)
See Also: → bug 1316525
You need to log in before you can comment on or make changes to this bug.