Closed
Bug 439115
Opened 16 years ago
Closed 15 years ago
DB merge allows nickname conflicts in merged DB
Categories
(NSS :: Libraries, defect, P1)
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)
22.07 KB,
patch
|
nelson
:
review-
|
Details | Diff | Splinter Review |
23.88 KB,
patch
|
alvolkov.bgs
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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.
Assignee | ||
Comment 2•16 years ago
|
||
I think this bug must be fixed before Sun can ship 3.12.x for its servers.
Priority: -- → P1
Updated•16 years ago
|
Whiteboard: FIPS
Comment 3•16 years ago
|
||
don't allow setting a conflicting nickname. Handle the case where there are conflicts in merging nicknames from different databases.
Updated•16 years ago
|
Attachment #346340 -
Flags: review?(nelson)
Assignee | ||
Updated•16 years ago
|
Attachment #346340 -
Flags: review?(nelson) → review-
Assignee | ||
Comment 4•16 years ago
|
||
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•16 years ago
|
||
> 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•16 years ago
|
||
> 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•16 years ago
|
||
> 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•16 years ago
|
||
> 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•16 years ago
|
||
I'll create a patch that addresses 1, 2, 4, & 7. bob
Assignee | ||
Comment 10•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
No, I stand corrected. That *IS* NSS code. I thought it was in certutil. bob
Comment 13•16 years ago
|
||
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•16 years ago
|
||
Arg... I thought I had already attached this patch.
Attachment #346340 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #349053 -
Flags: review?(nelson)
Assignee | ||
Comment 15•16 years ago
|
||
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-
Assignee | ||
Comment 16•16 years ago
|
||
Is the fix for this bug needed for FIPS algorithm testing to begin?
Comment 17•16 years ago
|
||
> 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
Assignee | ||
Comment 18•16 years ago
|
||
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.
Assignee | ||
Comment 19•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #350249 -
Flags: review?(rrelyea) → review+
Comment 20•16 years ago
|
||
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•16 years ago
|
||
^do the^so the^ BTW neither comment needs to be addressed in the end result. bob
Assignee | ||
Comment 22•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: 3.12.1 → 3.12.3
Assignee | ||
Comment 23•16 years ago
|
||
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 → ---
Assignee | ||
Comment 24•16 years ago
|
||
I backed out the previous patch.
Assignee | ||
Comment 25•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #350249 -
Attachment description: patch v3 - different conflict names, other issues fixed (checked in) → patch v3 - different conflict names, other issues fixed (backed out)
Assignee | ||
Updated•16 years ago
|
Whiteboard: FIPS → FIPS SUN_MUST_HAVE
Assignee | ||
Comment 26•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 351452 [details] [diff] [review] patch v4 - fix leak the right way r+
Attachment #351452 -
Flags: review?(alexei.volkov.bugs) → review+
Comment 29•15 years ago
|
||
Is this bug now resolved ?
Assignee | ||
Comment 30•15 years ago
|
||
Yes, I believe it is fixed now. The checkin occurred in December.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•