Closed Bug 439115 Opened 16 years ago Closed 15 years ago

DB merge allows nickname conflicts in merged DB

Categories

(NSS :: Libraries, defect, P1)

3.12

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: nelson, Assigned: nelson)

References

Details

(Whiteboard: FIPS SUN_MUST_HAVE)

Attachments

(2 files, 2 obsolete files)

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.
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.
I think this bug must be fixed before Sun can ship 3.12.x for its servers.
Priority: -- → P1
Blocks: FIPS2008
Whiteboard: FIPS
Attached patch handle nickname conflicts (obsolete) — Splinter Review
don't allow setting a conflicting nickname.
Handle the case where there are conflicts in merging nicknames from different databases.
Attachment #346340 - Flags: review?(nelson)
Attachment #346340 - Flags: review?(nelson) → review-
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?)
> 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'.
> 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
> 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.
> 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
I'll create a patch that addresses 1, 2, 4, & 7.

bob
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
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
No, I stand corrected. That *IS* NSS code. I thought it was in certutil.

bob
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.
Arg... I thought I had already attached this patch.
Attachment #346340 - Attachment is obsolete: true
Attachment #349053 - Flags: review?(nelson)
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
Attachment #349053 - Flags: review?(nelson) → review-
Is the fix for this bug needed for FIPS algorithm testing to begin?
> 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
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 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.
Attachment #350249 - Flags: review?(rrelyea)
Attachment #350249 - Flags: review?(rrelyea) → review+
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
^do the^so the^

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

bob
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
Attachment #350249 - Attachment description: patch v3 - different conflict names, other issues fixed → patch v3 - different conflict names, other issues fixed (checked in)
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: 3.12.1 → 3.12.3
Reopening.  That patch works fine on windows, but not on Unix/Linux.
Don't know why it makes any difference.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I backed out the previous patch.
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.
Assignee: rrelyea → nelson
Attachment #350249 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #351452 - Flags: review?(alexei.volkov.bugs)
Attachment #350249 - Attachment description: patch v3 - different conflict names, other issues fixed (checked in) → patch v3 - different conflict names, other issues fixed (backed out)
Whiteboard: FIPS → FIPS SUN_MUST_HAVE
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 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 on attachment 351452 [details] [diff] [review]
patch v4 - fix leak the right way

r+
Attachment #351452 - Flags: review?(alexei.volkov.bugs) → review+
Is this bug now resolved ?
Yes, I believe it is fixed now.  The checkin occurred in December.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.