Closed
Bug 522580
Opened 15 years ago
Closed 15 years ago
NSS uses PORT_Memcmp for comparing secret data.
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.5
People
(Reporter: agl, Assigned: agl)
Details
Attachments
(1 file, 2 obsolete files)
6.86 KB,
patch
|
Details | Diff | Splinter Review |
TLS aborts the connection when a MAC mismatch is detected, so maybe this isn't worth the effort, but the standard PORT_Memcmp leaking timing information about the length of the matching prefix. If an attacker could launch an adaptive attack they could use timing measurements to attack a MAC byte-by-byte. I added a PORT_SecureMemcmp and switched a few of the sensitive calls in lib/ssl to use it.
Attachment #406546 -
Flags: review?
Updated•15 years ago
|
Assignee: nobody → agl
Status: NEW → ASSIGNED
Target Milestone: --- → 3.12.5
Comment 1•15 years ago
|
||
Adam, thanks for the patch. Your patch looks good to me, although I didn't check whether the places where you changed to call PORT_SecureMemcmp are all comparing security-sensitive data. I made two minor changes to your patch. 1. I changed the 'int n' parameter of PORT_SecureMemcpy to 'size_t n' to match the standard memcmp prototype: http://www.opengroup.org/onlinepubs/000095399/functions/memcmp.html 2. I moved PORT_SecureMemcpy to a new NSSUTIL_3.12.5 section in nssutil.def.
Attachment #406546 -
Attachment is obsolete: true
Attachment #406590 -
Flags: review?(nelson)
Attachment #406546 -
Flags: review?
Comment 2•15 years ago
|
||
Comment on attachment 406590 [details] [diff] [review] Adam's patch v2 I was going to suggest using a different prefix than PORT_ but I don't feel strongly about it. r=nelson
Attachment #406590 -
Flags: review?(nelson) → review+
Comment 3•15 years ago
|
||
Comment on attachment 406590 [details] [diff] [review] Adam's patch v2 Among the other prefixes used in nss/lib/util, only NSS_ and SEC_ are suitable. I chose PORT_ partly because secport.h is the most suitable header for this function. But I see that NSS_PutEnv is declared in secport.h. Do you want me to use the NSS_ prefix?
Comment 4•15 years ago
|
||
Another reason I chose PORT_ was that we already have PORT_Memcmp. We may want to add a XXX_SecureMemset or XXX_SecureZero function that won't be optimized out by compilers.
Comment 5•15 years ago
|
||
> Do you want me to use the NSS_ prefix?
Yes, I think that's preferable.
Background:
The PORT_ prefix was originally created for a set of functions for which there existed multiple implementations on some platforms (e.g. NSPR1 vs NSPR2), or
for which different implementations were used on different platforms (e.g.
libc on some platforms, and NSPR's PL_ on others). Using the PORT_ macros
and functions accomplished a similar purpose to NSPR, it allowed the code
that called these platform-dependent functions to be free of ifdefs, keeping
platform dependencies concentrated in the secport header files and related .c
files.
For example, strcasecmp is called strcmpi on some platforms. Defining a macro named PORT_strcasecmp that expands to strcasecmp on some platforms and strcmpi on others avoids LOTS of ifdefs in all the callers of that function. NSS
could originally be built to use NSPR1 (which used an XP_ prefix) or NSPR2 (which used a PR_ prefix). Defining PORT_ macros enabled NSS code to be
sometimes built to use NSPR1 and sometimes built to use NSPR2 without having
ifdefs all over the NSS code.
In some cases, use of these PORT_ prefixes continued LONG after the need for
them had ended. For example, PORT_GetError, which wraps PR_GetError, was
originally needed to resolve the differences between NSPR1 vs NSPR2 (XP_GetError vs PR_GetError). When we stopped supporting NSPR1 altogether,
we could/should have stopped using PORT_GetError and switched to PR_GetError
but we didn't. The NSS team has previously agreed that we'd like to stop
using PORT_ macros for functions that now have only one implementation on all platforms, but we've never come up with a plan to do that.
Now, here is a case where a new function is being written that, AFAIK, has
only one implementation anywhere from the beginning. So the original
rationale for the PORT_ prefix doesn't apply to this code at all. I'd prefer that we not use PORT_ for new functions that have a single implementation.
Comment 6•15 years ago
|
||
There are exceptions to the rule you described for the PORT_ prefix, for example, the functions declared in portreg.h. Also, the comment at the top of rev. 1.1 of secport.h says: /* * secport.h - portability interfaces for security libraries * * This file abstracts out libc functionality that libsec depends on * * NOTE - These are not public interfaces * * $Id: secport.h,v 1.1 2000/03/31 19:41:24 relyea%netscape.com Exp $ */ with no reference to NSPR in the entire file. So one can interpret that secport.h is just a regular portability layer. OK. The function is renamed NSS_SecureMemcmp. It is still declared in secport.h. I checked in the patch on the NSS trunk (NSS 3.12.5). Checking in lib/ssl/ssl3con.c; /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v <-- ssl3con.c new revision: 1.117; previous revision: 1.116 done Checking in lib/ssl/sslcon.c; /cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v <-- sslcon.c new revision: 1.37; previous revision: 1.36 done Checking in lib/ssl/sslgathr.c; /cvsroot/mozilla/security/nss/lib/ssl/sslgathr.c,v <-- sslgathr.c new revision: 1.10; previous revision: 1.9 done Checking in lib/util/nssutil.def; /cvsroot/mozilla/security/nss/lib/util/nssutil.def,v <-- nssutil.def new revision: 1.10; previous revision: 1.9 done Checking in lib/util/secport.c; /cvsroot/mozilla/security/nss/lib/util/secport.c,v <-- secport.c new revision: 1.25; previous revision: 1.24 done Checking in lib/util/secport.h; /cvsroot/mozilla/security/nss/lib/util/secport.h,v <-- secport.h new revision: 1.22; previous revision: 1.21 done
Attachment #406590 -
Attachment is obsolete: true
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•