Last Comment Bug 439115 - DB merge allows nickname conflicts in merged DB
: DB merge allows nickname conflicts in merged DB
Status: RESOLVED FIXED
FIPS SUN_MUST_HAVE
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All All
: P1 major (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
:
Mentors:
Depends on:
Blocks: FIPS2008
  Show dependency treegraph
 
Reported: 2008-06-13 13:16 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-02-06 18:10 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
handle nickname conflicts (21.80 KB, patch)
2008-11-04 14:50 PST, Robert Relyea
nelson: review-
Details | Diff | Splinter Review
Address review comments. (22.07 KB, patch)
2008-11-19 14:53 PST, Robert Relyea
nelson: review-
Details | Diff | Splinter Review
patch v3 - different conflict names, other issues fixed (backed out) (23.19 KB, patch)
2008-11-26 17:08 PST, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
Details | Diff | Splinter Review
patch v4 - fix leak the right way (23.88 KB, patch)
2008-12-04 14:42 PST, Nelson Bolyard (seldom reads bugmail)
alvolkov.bgs: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2008-06-13 13:16:14 PDT
I'm filing this bug based on a discussion of the new Shareable DB merge code 
that I had with Bob Relyea.

Today, if you have a cert in your DB with a certain subject name (say,
Subject-A) and a certain nickname (say, Server-Cert) and you attempt to 
import a cert from a PKCS#12 file, a cert with a different subject name 
(say, Subject-B) but the same nickname (Server-Cert) into it, NSS will not 
allow the import to succeed because of a "nickname conflict" (or "nickname 
collision").  NSS requires that each nickname uniquely identify a single subject name.  (In NSS 3.12, NSS will attempt to create a new Nickname for
the newly imported cert, but the user cannot choose the new name, and the 
new name may also conflict, in which case it will fail to import.)

But, when merging two DBs together into a shareable DB, this nickname conflict 
will go undetected.  If you have two cert DBs, one with a cert with Subject 
name Subject-A, and the other with a cert with Subject Name Subject-B, but 
both with a common nickname (Server-Cert), and you merge those DBs into a 
single sharable cert DB, you will end up with both certs in the same DB, but 
with the same nickname.  The nickname will no longer uniquely identify a 
single subject name.  Consequently, servers whose DBs were merged, and who 
share that merged DB, will not necessarily get a cert with the subject name they expect when they request a cert with that nickname.

Today, it is common for servers to all use the same nickname (Server-Cert) 
in their separate respective cert DBs.  Consequently, nickname collisions 
like the one described above are expected to be common.  

Today, NSS offers no way to change the nickname of a cert in a DB or in a 
PKCS#12 file to another value of the user's choice.  It is possible to 
change a nickname only by exporting the cert from the DB (as a plain cert, 
not in a PKCS#12 file) and then import it into another cert DB by a 
different nickname that the user can choose.  This is the only way that a
sysadmin can avoid a nickname conflict while merging two DBs.  

A sysadmin would need to first detect a nickname conflict by making a list
of each DB's nicknames, and then comparing the two lists for duplicates,
and then checking each duplicate to see if they represent a conflict or not.

So, this bug says that the DB merge code should detect nickname conflicts 
and not create a merged DB with multiple subject names for a common nickname.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2008-06-13 13:21:01 PDT
Christophe, it would be good for us to have QA test scripts for these issues.
We should have one test for merging a cert8 into a cert9, and another for 
merging one cert9 with another cert9.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2008-07-31 19:02:17 PDT
I think this bug must be fixed before Sun can ship 3.12.x for its servers.
Comment 3 Robert Relyea 2008-11-04 14:50:04 PST
Created attachment 346340 [details] [diff] [review]
handle nickname conflicts

don't allow setting a conflicting nickname.
Handle the case where there are conflicts in merging nicknames from different databases.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2008-11-16 21:20:03 PST
Comment on attachment 346340 [details] [diff] [review]
handle nickname conflicts

r-

1. The new loop added to pk11_mergeCert leaks the value of tokenNickname 
if it goes through the loop more than once. i.e., when SEC_CertNickname 
returns a non-zero (true) value and pk11_IncrementNickname doesn't fail.  

2. The new function:
static char * 
pk11_mkTokenNickname(const char *tokenName, const char *nickname)

is supposed to return a new string that is "tokenName:nickname", 
but it actually returns a string that is a copy of nickname, 
the second argument.  

That may be OK in the case where tokenName names the internal token, 
and the nickname doesn't contain any embedded ':' character.
Apparently, the new test cases only test such cases where the token is 
the internal token and the nickname contains no ':' character.  But 
that's not the general case for which we want this merge code to work. 

I suggest that, rather than trying to fix that function, just toss it,
and use an existing NSPR function instead.  e.g. in pk11_mergeCert
change
-	    tokenNickname = pk11_mkTokenNickname(tokenName, nickname);
+           tokenNickname = PR_smprintf("%s:%s", tokenName, nickname);
and change
-	    PORT_Free(tokenNickname);
+	    PR_smprintf_free(tokenNickname);

3. NSS already has a convention for resolving nickname conflicts.  This
patch invents yet another different convention.  I have two issues with
it:
a) it's not the old convention. :)
b) it increments a trailing number that is part of the nickname, even if
that number doesn't have the appearance of being a conflict resolution.

If a cert with nickname "Bob2008" exists and someone tries to add a cert 
with a second subject name with nickname "Bob2008", this patch would give 
the new cert the nickname "Bob2009".

The old convention was to add " #%d" to the nickname, where the value of
the variable was never less than 2.  It would give the new cert the 
nickname "Bob2008 #2", and if that conflicted, then "Bob2008 #3" and so on.  
I think that old convention is a good one.

So, I suggest that the code should see if the string ends in " #[0-9]+"
and if so, attempt to increment it, but if not, to append " #2" to it,
and then check that for conflicts and increment it if needed.

BTW, the old convention is implemented in the function named 
CERT_MakeCANickname, which is also used for non-CA names.

4.  problems in code comments

    s/tempararies/temporaries/
    s/conflictin /conflicting /
    g/the Subject's clearly/s/Subject's/Subjects/
    s/samecompare/same. Compare/
- - -
/* if we've failed for some other reason then conflict, make sure we return
 * an error code other than (NOTE: neither sdb_FindObjectsInit 
                           ^ word missing here.
 * nor sdb_GetAttributeValue should return CKR_ATTRIBUTE_VALUE_INVALID, 
 * so the following is paranoia).
- - -
     *  1) there were certain conflicts (like trying to the same nickname on
                                                       ^ missing verb here
     * two different subjects) that would return an error.
     *  2) Importing the 'same' object would silently update that object.
     * The following 2 functions mimics the desirable effects of these two
                                      ^ should be mimic
- - -
 # make sure bob has
                     ^ missing word(s) here.  
- - -
  # contains a certificates with different nicknames
             a certificate ?
           two certificates?

5. This new code only checks for nickname conflicts on certs.
I know it may seem unlikely, but might we have a conflict with key 
objects, where we import a second key with a different value but the
same CKA_ID as an existing key?   That would be bad, right?

6. This patch contains two complete copies of the new code that implements
the "increment name" logic.  That's a maintenance problem.  At the very
least there should be comments in both places saying "if you change this,
change the other copy in <otherfile.c>"  If course, it would be best to 
have just one copy of this function, at most one copy of the source,
maybe in its own source file.  Even if that source file gets compiled 
twice, one source is better than two.

7. The new test script has comments that suggest that it is possible to
change a nickname on an existing cert.  The comment claims that this is 
attempted by trying to import the cert a second time with a different 
nickname.  I think this is going to be the source of problems.  People
are searching for a way to change nicknames on an existing cert (or set 
of certs), and this comment will lead many in the wrong direction, I fear.
(How DOES one change a nickname?)
Comment 5 Robert Relyea 2008-11-17 14:50:20 PST
> 3. NSS already has a convention for resolving nickname conflicts.  This
> patch invents yet another different convention.  I have two issues with
> it:
> a) it's not the old convention. :)

Actually NSS had no such convention. There is no code in NSS that resolves nickname conflicts. We've always pushed it back up to the application.

> b) it increments a trailing number that is part of the nickname, even if
> that number doesn't have the appearance of being a conflict resolution.
> 
> If a cert with nickname "Bob2008" exists and someone tries to add a cert 

I actually thought about this case. I decided given the time pressure that the most important to make sure the nicknames were 'Unique' rather than 'Pretty'.
Comment 6 Robert Relyea 2008-11-17 14:56:12 PST
> 5. This new code only checks for nickname conflicts on certs.
> I know it may seem unlikely, but might we have a conflict with key 
> objects, where we import a second key with a different value but the
> same CKA_ID as an existing key?   That would be bad, right?

While CKA_ID's can be 'free-form', In our databases they are always generated based on the key itself. If 2 public/private/keys have the same CKA_ID's, then they are the same key. The merge code already handles that. Multiple certificates Can already point to the same CKA_ID. CKA_ID collision with symmetric keys is already handled.

If you happened to import 2 keys with different CKA_ID's but they are the same key. NSS would simply end up treating them as two different Keys.

bob
Comment 7 Robert Relyea 2008-11-17 14:58:06 PST
> 6. This patch contains two complete copies of the new code that implements
> the "increment name" logic. 

I started on a combined version. The input/output/memory usage requirements of the two made the combined version quite complicated, so I went back to separate ones. The function itself was simple enough.
Comment 8 Robert Relyea 2008-11-17 15:00:39 PST
> 7. The new test script has comments that suggest that it is possible to
change a nickname on an existing cert. 

I'll update the comment, but you did ask a question...

> (How DOES one change a nickname?)

There are PK11_ calls to change the nickname an any of our PKCS #11 objects, including certs. 

bob
Comment 9 Robert Relyea 2008-11-17 15:02:10 PST
I'll create a patch that addresses 1, 2, 4, & 7.

bob
Comment 10 Nelson Bolyard (seldom reads bugmail) 2008-11-17 15:03:50 PST
Bob, there absolutely IS code in NSS that resolves nickname conflicts.
I cited it in comment 4.  Here is a URL for the code in question:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certdb.c&rev=1.92&mark=2049,2058-2062,2069-2073#2048
Comment 11 Robert Relyea 2008-11-17 15:15:45 PST
That's 'application' code...

It also has the disadvantage of creating stuff like "Bob's Cert #1 #1 #1" in the case of multiple merges.

bob
Comment 12 Robert Relyea 2008-11-17 15:18:23 PST
No, I stand corrected. That *IS* NSS code. I thought it was in certutil.

bob
Comment 13 Robert Relyea 2008-11-17 15:21:40 PST
OK, so actually this code is not 'conflict' code. It's just code to generate a
unique Certificate. It's not code that deals with a given nickname that
conflicts with one in the database.
Comment 14 Robert Relyea 2008-11-19 14:53:21 PST
Created attachment 349053 [details] [diff] [review]
Address review comments.

Arg... I thought I had already attached this patch.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2008-11-19 17:47:21 PST
Comment on attachment 349053 [details] [diff] [review]
Address review comments.

Bob, 
I think we really don't want to change the nickname serverCert2008 into 
serverCert2009.  If we provided a tool with which the user could change
the name to something more to his liking, then it might be OK, but 
we still haven't provided a tool with which a user can change nicknames.

If the code for resolving nickname conflicts was purely outside of the 
softoken, then I'd say we don't have to get it perfect right now, but 
since one copy of this code IS in softoken, and we really don't want
to be changing softoken very often, I think we want to take the little
extra time to get it right here. 

Here are some other minor changes, mostly cosmetic.  
One is not purely cosmetic.

>+ * Changes to this should be mirror in it's sister version in
                               mirrored in its 

>+    /* no existing trailing number... */
>+    if (! hasDigit) {
>+	/* ... append ' 1' to the name */
>+	newNickname = PORT_Realloc(nickname, len+sizeof(" #1"));
>+	if (!newNickname) {
>+	    PORT_Free(nickname);
>+	    return NULL;
>+	}
>+	PORT_Strcat(newNickname, " #1");

The code we're tying to mimic in CERT_MakeCANickname starts with #2.  
Let's do that here too, using 2 instead of 1 in 3 places above.  
Do that in both copies of this code. :(

>+    /* The database code will prevent nickname collisons for certs with
                                                 collisions

>+    /* currently only conflict is to nicknames point to the same subject.
                                    with         pointing

>+    /* Ok we've both subjects, make sure they are the same. \
                 ^ word missing here.  got?                   


>+    /* If we've failed for some other reason then conflict, make sure we return
                                               than
I suggest                         reason other than

>+  # in the upgrade mode (dbm->sql), make sure out test databases
                                                our
Comment 16 Nelson Bolyard (seldom reads bugmail) 2008-11-19 19:56:16 PST
Is the fix for this bug needed for FIPS algorithm testing to begin?
Comment 17 Robert Relyea 2008-11-20 09:50:04 PST
> I think we really don't want to change the nickname serverCert2008 into 
> serverCert2009.

I would prioritize this much lower than the other bugs I'm looking at. If you are willing to wait this fix for this semantic, I'm OK with put off this patch until I have time to deal with this.


> The code we're tying to mimic in CERT_MakeCANickname starts with #2.

That sounds like a bug in CERT_MakeCANickname...

> If the code for resolving nickname conflicts was purely outside of the 
> softoken.

The code lives in both places for 2 different causes. One of those is softoken.

> Is the fix for this bug needed for FIPS algorithm testing to begin?

No, it's priority was based on other considerations.

bob
Comment 18 Nelson Bolyard (seldom reads bugmail) 2008-11-26 17:08:41 PST
Created attachment 350249 [details] [diff] [review]
patch v3 - different conflict names, other issues fixed (backed out)

This patch is based on the previous version.  
it changes the conflict resolution algorithm to work as follows:
- if the conflicting name does not end with " #n*" (that is, with 
a space, a sharp, and one or more decimal ASCII digits), then 
append " #2" to it, so that the names will be, e.g. "Alice" and 
"Alice #2" and then check for conflict again.
- if the conflicting name already does end in " #n*", then 
increment n, adding another digit as necessary.  

After I coded that patch, I found a number of other issues with the 
previous patch (before I changed it) that seemed serious, such as 
problems with pointers to const and non-const.  Also one function
didn't have the required number of arguments!  

So, this patch attempts to address all those issues.
Comment 19 Nelson Bolyard (seldom reads bugmail) 2008-11-26 17:14:13 PST
Comment on attachment 350249 [details] [diff] [review]
patch v3 - different conflict names, other issues fixed (backed out)

This patch also fixed a few leaks.
Bob, please review this patch after reading the preceding
comments in this bug. Thanks.
Comment 20 Robert Relyea 2008-11-26 17:53:40 PST
Comment on attachment 350249 [details] [diff] [review]
patch v3 - different conflict names, other issues fixed (backed out)

r+

Some comments...
In sftkdb.c We allocated sizeof(num2) and we copy the entire amount. This added extra NULL in the attribute. This is not a problem, since ulValueLen is set correctly (without the NULL), so the 'waste' of one byte isn't critical.

In the tests:
The "Alice #1" nickname is meant to test a 'generated' conflict, where we test Alice, then Alice generated 'Alice #1' which requires another loop in out algorithm. In the new code it, however it generated 'Alice #2' which doesn't conflict.

My first thought is we should change it to Alice #2, however it's probably OK as is since the second database will conflict it's Alice with the new 'Alice #2', do the code is sufficiently tested.

bob
Comment 21 Robert Relyea 2008-11-26 17:54:50 PST
^do the^so the^

BTW neither comment needs to be addressed in the end result.

bob
Comment 22 Nelson Bolyard (seldom reads bugmail) 2008-12-03 00:07:33 PST
Comment on attachment 350249 [details] [diff] [review]
patch v3 - different conflict names, other issues fixed (backed out)

checked in on trunk
lib/pk11wrap/pk11merge.c; new revision: 1.3;  previous revision: 1.2
lib/softoken/pkcs11u.c;   new revision: 1.78; previous revision: 1.77
lib/softoken/sftkdb.c;    new revision: 1.14; previous revision: 1.13
lib/softoken/sftkdb.h;    new revision: 1.7;  previous revision: 1.6
tests/dbtests/dbtests.sh; new revision: 1.17; previous revision: 1.16
tests/merge/merge.sh;     new revision: 1.4;  previous revision: 1.3
Comment 23 Nelson Bolyard (seldom reads bugmail) 2008-12-03 14:21:41 PST
Reopening.  That patch works fine on windows, but not on Unix/Linux.
Don't know why it makes any difference.
Comment 24 Nelson Bolyard (seldom reads bugmail) 2008-12-03 14:40:40 PST
I backed out the previous patch.
Comment 25 Nelson Bolyard (seldom reads bugmail) 2008-12-04 14:42:30 PST
Created attachment 351452 [details] [diff] [review]
patch v4 - fix leak the right way

Alexei discovered the cause of the problem in the previous patch.

I think I now understand why it behaved differently on Windows than on Unix.
The different is in the behavior of realloc() when the buffer passed to 
realloc is already large enough to hold the newly increased size.  
Apparently, on Windows, realloc returns a different buffer than the original, 
but on Unix, it may return the original buffer, the same buffer as passed in.
On Windows, the bug would cause the old buffer to be double-freed, but would
not corrupt the new buffer returned by realloc.  On unix, the code would free
the buffer returned by realloc, causing it to be overwritten, and would 
return the address of the freed buffer to its caller.  

I have not yet tested this patch on Unix yet, and intende to do so before 
I commit.  But I want to ask Alexei to review this now.
Comment 26 Nelson Bolyard (seldom reads bugmail) 2008-12-04 21:06:17 PST
Comment on attachment 351452 [details] [diff] [review]
patch v4 - fix leak the right way

Checked in on trunk  (oops)
lib/pk11wrap/pk11merge.c; new revision: 1.5;  previous revision: 1.4
lib/softoken/pkcs11u.c;   new revision: 1.80; previous revision: 1.79
lib/softoken/sftkdb.c;    new revision: 1.16; previous revision: 1.15
lib/softoken/sftkdb.h;    new revision: 1.9;  previous revision: 1.8
tests/dbtests/dbtests.sh; new revision: 1.19; previous revision: 1.18
tests/merge/merge.sh;     new revision: 1.6;  previous revision: 1.5
Comment 27 Alexei Volkov 2008-12-04 22:19:25 PST
Comment on attachment 351452 [details] [diff] [review]
patch v4 - fix leak the right way

I didn't have time to review the whole patch today, but difference between two patches shows that new patch fixes double free of a memory pointed by variable "nickname" in pk11merge.c:pk11_IncrementNickname function. We discussed this fix though AM. r+ for this change.

I'll do the rest tomorrow.
Comment 28 Alexei Volkov 2008-12-05 16:27:01 PST
Comment on attachment 351452 [details] [diff] [review]
patch v4 - fix leak the right way

r+
Comment 29 Julien Pierre 2009-02-06 17:40:22 PST
Is this bug now resolved ?
Comment 30 Nelson Bolyard (seldom reads bugmail) 2009-02-06 18:10:36 PST
Yes, I believe it is fixed now.  The checkin occurred in December.

Note You need to log in before you can comment on or make changes to this bug.