Closed Bug 522580 Opened 15 years ago Closed 15 years ago

NSS uses PORT_Memcmp for comparing secret data.

Categories

(NSS :: Libraries, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
3.12.5

People

(Reporter: agl, Assigned: agl)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — 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?
Assignee: nobody → agl
Status: NEW → ASSIGNED
Target Milestone: --- → 3.12.5
Attached patch Adam's patch v2 (obsolete) — Splinter Review
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 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 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?
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.
> 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.
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: