Closed Bug 808217 Opened 13 years ago Closed 12 years ago

Sporadic failure of 'certutil' to convert ASCII cert request to binary . . .

Categories

(NSS :: Tools, defect, P2)

3.13.6
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mharmsen, Assigned: KaiE)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0 Build ID: 20120828083604 Steps to reproduce: This problem sporadically occurs in 'certutil' when using the "-a" option. The Dogtag Certificate System utilizes the following 'certutil' commands to generate a certificate: * certutil -N -d /tmp/ztest/certs -f /tmp/ztest/password.conf * certutil -A -n "caadmin" -t "u,u,u" -f /tmp/ztest/password.conf -d /tmp/ztest/certs -a -i /tmp/ztest/ca_admin.cert Actual results: It has been noticed that occasionally, the following command sequence utilized to generate a certificate fails: * certutil -N -d /tmp/ztest/certs -f /tmp/ztest/password.conf * certutil -A -n "caadmin" -t "u,u,u" -f /tmp/ztest/password.conf -d /tmp/ztest/certs -a -i /tmp/ztest/ca_admin.cert error converting ascii to binary (Unrecognized Object Identifier.) certutil: unable to read input file: Unrecognized Object Identifier. Here is a sample of a failed ASCII certificate request: MIID9DCCAtygAwIBAgIBBjANBgkqhkiG9w0BAQsFADA+MRswGQYDVQQKExJVU0VS U1lTLlJFREhBVC5DT00xHzAdBgNVBAMTFkNBIFNpZ25pbmcgQ2VydGlmaWNhdGUw HhcNMTIxMDI1MjMyODU1WhcNMTQxMDE1MjMyODU1WjCBkzEbMBkGA1UEChMSVVNF UlNZUy5SRURIQVQuQ09NMSkwJwYJKoZIhvcNAQkBFhpjYWFkbWluQHVzZXJzeXMu cmVkaGF0LmNvbTEXMBUGCgmSJomT8ixkAQETB2NhYWRtaW4xMDAuBgNVBAMTJ0NB IEFkbWluaXN0cmF0b3Igb2YgSW5zdGFuY2UgcGtpLXRvbWNhdDCCASIwDQYJKoZI hvcNAQEBBQADggEPADCCAQoCggEBALz/yuUqY4oDtz/jktdrquoHItGj01w9l3BJ 8mfhmnFR9L1n7Coh4opBaJ/cVo11craeblOhq9pYrSrd6HWKsF26n1dGOGcW80kF pnbRIl/nvohPEv70T/jG2Mau0OrmIPyofeggKlfqq5ZE+E/WR6DKuACmCVDIbfg4 /i34fMCL/kWKfgwwht2jRVjbHgRGF7HCDLbxR4unqMG9CAEC/oHNgD3y5BZmw8nI xdUfxLoonD7DFFPTd3lTSweSNPQTlHOSYRkBbM9qpP2F3XNBkW0rq2o7Sp9gfcCI 7m2RL1C8G/GXafPkLt1NZ3A/zKlrmfnL+Kh1iCES5fDO+KXZVtkCAwEAAaOBpjCB ozAfBgNVHSMEGDAWgBTnj55EvB9WfBzsy/q5r5tvUOUirTBRBggrBgEFBQcBAQRF MEMwQQYIKwYBBQUHMAGGNWh0dHA6Ly9kb2d0YWcxNy1jbG9uZS51c2Vyc3lzLnJl ZGhhdC5jb206ODA4MC9jYS9vY3NwMA4GA1UdDwEB/wQEAwIE8DAdBgNVHSUEFjAU BggrBgEFBQcDAgYIKwYBBQUHAwQwDQYJKoZIhvcNAQELBQADggEBAKgZTBj5P01i AIV/xe12nT7ti4Hz6//yZbwnIWv1tOSCVULsHYzwDSLp0/HRmdux6nKiE8WBtZ59 NHi5ukT1ceNp9iJP2MP7Xx38K3PPhJfkri4ajuSWtQonjyILxvYY9bmPOgCwEKng GWec1pqn73n/Pqe/kQn42w8DOfnyA+JS+xsZi6QdZoy9iKKUUnrsJK/SMoprpbKc fANaeTsKcbA7COYCKUP8W3pMhWDw1DMlFF9o4EnzqYMTw9QfpzJNX8gR+BODaXeI A5Pbv1/UjvQn/8NUGZ99AuPrkBu1FZOnakz6Ln9rnk+2xrFMzh/FJ5CuUy6y91Zl yHiL701M/Io= Expected results: After using the Dogtag Certificate System AtoB tool to change it from ASCII to Binary, I re-ran certutil (without the '-a' option), and it worked fine: AtoB ca_admin.cert ca_bin.cert certutil -A -n "caadmin" -t "u,u,u" -f /tmp/ztest/password.conf -d /tmp/ztest/certs -i /tmp/ztest/ca_bin.cert
Are you always using the same fixed filenames? Could there potentially be a race by two parallel uses of the same filenames?
Status: UNCONFIRMED → NEW
Ever confirmed: true
I had not seen the analysis in the referenced bug, which explains the problem. I'll work on a patch.
Attached patch Patch v1Splinter Review
Reallocate and add a zero terminator if necessary.
Assignee: nobody → kaie
Attachment #738995 - Flags: review?(rrelyea)
Comment on attachment 738995 [details] [diff] [review] Patch v1 r+ rrelyea
Attachment #738995 - Flags: review?(rrelyea) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.15
Reopening, because the issue isn't fully fixed. While it had been fixed with my own test certificate, I received another test certificate that still misbehaved. I identified a bug in function SECITEM_ReallocItem. After reallocating the data of the item, the function doesn't adjust the length stored in the item! I found two places in blapitest.c that make use of SECITEM_ReallocItem, one place doesn't adjust the size and could potentially misbehave, too. The second place does adjust it, but has a comment complaining about the behaviour of SECITEM_ReallocItem. Therefore I believe it makes sense to fix SECITEM_ReallocItem to adjust the store size after reallocation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #739785 - Flags: review?(rrelyea)
Comment on attachment 739785 [details] [diff] [review] Additional patch v2, fix SECITEM_ReallocItem r- I agree it's a silly semantic, but it's also part of the ABI and I found at least one case where we depend partially on that behavior (see get_binary() in blapitest.c). I think get_binary() is still broken (with or without your update), but I don't think we can hank the rug out from under someone expecting the old semantic. I would be OK adding SECITEM_ReallocUpdateItem() which has the correct semantics. bob
Attachment #739785 - Flags: review?(rrelyea) → review-
Blocks: 863905
(In reply to Robert Relyea from comment #8) > > I would be OK adding SECITEM_ReallocUpdateItem() which has the correct > semantics. Ok, I used that approach, and documented the old behaviour.
Attachment #739785 - Attachment is obsolete: true
Attachment #739854 - Flags: review?(rrelyea)
Comment on attachment 739854 [details] [diff] [review] Additional patch v3, add and use SECITEM_ReallocUpdateItem r+ rrelyea Excelent. bob
Attachment #739854 - Flags: review?(rrelyea) → review+
There is a very good chance that blapitest.c is the only user of SECITEM_ReallocItem. SECITEM_ReallocItem was not exported until NSS 3.12.3, and it was exported to allow blapitest.c to link with the new libnssutil3.so library (bug 438870). So I think it is fine to just fix SECITEM_ReallocItem. If we want to be conservative, I suggest that the new function name be as similar to SECITEM_ReallocItem as possible, for example, SECITEM_ReallocItemV2 or SECITEM_ReallocItem2. Note that Nelson filed bug reports about SECITEM_ReallocItem before: - bug 298649: SECITEM_ReallocItem leaks mem, doesn't set item length - bug 298938: SECITEM_ReallocItem fails if newlen < oldlen and arena non-null The new function should not have those bugs, and the old bug reports should be marked as duplicate.
Comment on attachment 739854 [details] [diff] [review] Additional patch v3, add and use SECITEM_ReallocUpdateItem I like the proposed alternative name SECITEM_ReallocItemV2. I agree with with Wan-Teh that it's very unlikely that anyone depends on the broken behaviour. But apparently something being unlikely isn't sufficient to win against the stability requirements that Bob mentions. Since we cannot prove that no external consumer uses it, we should keep the old function unchanged. Note that Nelson mentions another behaviour of the old API that users might rely on in bug 298649 (behaviour on failing realloc). I propose that we introduce SECITEM_ReallocItemV2 and fix ALL errors of the old implementation. That means I'll make a new patch.
Attachment #739854 - Flags: review-
I think the new API doesn't need the "oldlen" parameter. I've copied and simplified the old implementation, and fixed the bugs that were mentioned in bug 298649 and bug 298938.
Attachment #739854 - Attachment is obsolete: true
Attachment #739967 - Flags: superreview?(wtc)
Attachment #739967 - Flags: review?(rrelyea)
Also note I added a line break and a comment to the declaration of the old function. If future consumers will "grep" for the function name, they can immediately detect that the old function is deprecated. They will also be able to find the new function in the same step, because the new name contains the old name.
I agree with your proposal Kai. The issue with SECITEM_ReallocItem is that it was released in 3.12.3, which was release a number of years ago. Due to the crypto consolidation program, we really don't have the ability to identify who is using want parts of NSS anymore. I originally thought fixing the issue was going to be inoccuous, but then I saw parts of our own code that were depending on the broken behaviour. I agree if we add a new version, we should fix all the issues, and I thank wtc for pointing the bug with all of them in it. For those reasons I agree with Kai. in creating SECITEM_ReallocItem. I also agree that not only do we not need oldlen, we shouldn't supply it. Semantically what does it mean if oldlen and item->len are not the same? re comment 14, we can mark the function deprecated on Windows and GCC compilied platforms. On gcc 4.3 we can even say what replaces it. See nss/mozilla/utils/pkcs11n.h for an example. (NOTE: functions are easier to mark deprecated than # defines) bob bob
Comment on attachment 739967 [details] [diff] [review] Additional patch v4, add and use SECITEM_ReallocItemV2 Review of attachment 739967 [details] [diff] [review]: ----------------------------------------------------------------- Kai, this patch looks good except for one problem. I suggest a fix for that problem below. ::: lib/util/secitem.c @@ +130,5 @@ > + } > + > + if (item->len >= newlen) { > + /* There's no need to realloc, and no need to potentially > + * fail on realloc, because the buffer already has sufficient It is not a problem for realloc() to fail as long as this function doesn't modify |item|. I think it can be useful to shrink item->data, so we should still call realloc() in that case. However, what you're doing here is apppropriate when |arena| is non-NULL. So I suggest we rewrite this if statement as follows: if (arena && item->len >= newlen) { /* There's no need to realloc a shorter block from the arena * because we would simply use more memory. */ item->len = newlen; return SECSuccess; } @@ +133,5 @@ > + /* There's no need to realloc, and no need to potentially > + * fail on realloc, because the buffer already has sufficient > + * size. We don't want to store the shorter len in the item, > + * because that might confuse a later call to PORT_ArenaGrow on > + * the same item? Therefore, we'll simply keep everything as is, We must set item->len to newlen whenever this function returns SECSuccess. @@ +135,5 @@ > + * size. We don't want to store the shorter len in the item, > + * because that might confuse a later call to PORT_ArenaGrow on > + * the same item? Therefore, we'll simply keep everything as is, > + * and return success. > + * This includes the case if newlen is zero. If newlen is zero, realloc() acts like free(). So I think SECITEM_ReallocItemV2() probably should free item->data in that case. ::: lib/util/secitem.h @@ +37,5 @@ > extern SECItem *SECITEM_AllocItem(PLArenaPool *arena, SECItem *item, > unsigned int len); > > /* > +** This is a legacy function cotaining bugs. However, the function is Typo: cotaining => containing We should be clear which bugs we are not fixing. I believe the only bug that we cannot fix is the failure to update item->len. @@ +48,5 @@ > ** In any case, "item" is expected to be a valid SECItem pointer; > ** SECFailure is returned if it is not. If the allocation succeeds, > ** SECSuccess is returned. > */ > +extern SECStatus SECITEM_ReallocItem( /* deprecated */ Please remove this /* deprecated */ comment. Putting this comment in the function's argument list is confusing because it seems to say only the first argument |arena| is deprecated.
Attachment #739967 - Flags: superreview?(wtc) → superreview-
> > +extern SECStatus SECITEM_ReallocItem( /* deprecated */ > > Please remove this /* deprecated */ comment. > > Putting this comment in the function's argument list is confusing > because it seems to say only the first argument |arena| is deprecated. I don't agree with your conclusion. The old function should no longer be used, I think we agree on that. If the comment is confusing, it should be clarified. The comment could be clarified by saying /* deprecated function */, then it will be obvious that the comment doesn't apply to a parameter.
> We should be clear which bugs we are not fixing. I believe the only > bug that we cannot fix is the failure to update item->len. I'm not touching the old function at all, and therefore all bugs will continue to exist in the old implementation. As Bob asks to keep the behaviour of the old function unchanged, I will not touch the old function in this bug. In order to address your request for clarification, I'll change the old function's comment to: ** This is a legacy function containing bugs. It doesn't update item->len, ** and it has other issues as described in bug 298649 and bug 298938. ** However, the function is kept unchanged for consumers that might depend ** on the broken behaviour. New code should call SECITEM_ReallocItemV2.
> If newlen is zero, realloc() acts like free(). So I think > SECITEM_ReallocItemV2() probably should free item->data in > that case. I agree and will update the patch accordingly. I saw that SECITEM_FreeItem updates both data and len members, so if !newlen then we can simply call SECITEM_FreeItem.
(In reply to Wan-Teh Chang from comment #16) > > Kai, this patch looks good except for one problem. I suggest a fix for that > problem below. You have asked for multiple changes, so it isn't clear which part you have identified as the "primary" problem that you'd like to see fixed. I'm guessing that you were referring to the following detail: > We must set item->len to newlen whenever this function returns > SECSuccess. Thank you for clarifying this detail. If you think this is a strict requirement of the API, then I agree we must add further changes to the new function. > It is not a problem for realloc() to fail as long as this function > doesn't modify |item|. > > I think it can be useful to shrink item->data, so we should > still call realloc() in that case. You argue that it's more important to usually shrink blocks of memory (that have been allocated from the heap), than to avoid a potential failure of realloc. I can agree with your preference. > However, what you're doing here is apppropriate when |arena| is > non-NULL. So I suggest we rewrite this if statement as follows: > > if (arena && item->len >= newlen) { > /* There's no need to realloc a shorter block from the arena > * because we would simply use more memory. > */ > item->len = newlen; > return SECSuccess; > } I merged this scenario in the existing other block of if/else decisions. Also, our discussion has inspired me to add another check to the beginning of the function: if (item->len == newlen) { return SECSuccess; }
Attachment #739967 - Attachment is obsolete: true
Attachment #739967 - Flags: review?(rrelyea)
Attachment #743662 - Flags: superreview?(wtc)
Attachment #743662 - Flags: review?(rrelyea)
We should try to get this into 3.15
Comment on attachment 743662 [details] [diff] [review] additional patch v5 r+ rrelyea
Attachment #743662 - Flags: review?(rrelyea) → review+
Thanks for the review, additional patch checked in, too. https://hg.mozilla.org/projects/nss/rev/d0cf1feff4dc
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Attachment #743662 - Flags: superreview?(wtc)
Priority: -- → P2
If |body| is NULL, then |trailer| must be NULL. So if (!body || !trailer) is equivalent to if (!trailer) which means the original code is correct.
Attachment #752472 - Flags: review?(kaie)
1. This specification is overly strong: If then the item is kept unchanged and SECSuccess is returned. the item already has at least the request new size, In the current implementation, this comment is correct only when we are using an arena. So I removed this comment. 2. The standard C library function realloc() behaves like malloc() if its |ptr| input argument is NULL. So I changed SECITEM_ReallocItemV2 to match that. This requires changing the test if (!item->len) { to if (!item->data) { 3. I also made a coding style fix: remove the "else" after a return statement.
Attachment #752481 - Flags: review?(kaie)
Comment on attachment 752481 [details] [diff] [review] Make small adjustments to SECITEM_ReallocItemV2 Review of attachment 752481 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/util/secitem.c @@ +132,5 @@ > > if (!newlen) { > SECITEM_FreeItem(item, PR_FALSE); > return SECSuccess; > } I just noticed that we can only call SECITEM_FreeItem when |arena| is NULL. I will change this code to: if (!newlen) { if (!arena) { PORT_Free(item->data); } item->data = NULL; item->len = 0; return SECSuccess; } Please also approve this change. Thanks.
Comment on attachment 752472 [details] [diff] [review] Revert an unnecessary change to nss/cmd/lib/secutil.c (In reply to Wan-Teh Chang from comment #25) > Created attachment 752472 [details] [diff] [review] > Revert an unnecessary change to nss/cmd/lib/secutil.c > > If |body| is NULL, then |trailer| must be NULL. > So > if (!body || !trailer) > is equivalent to > if (!trailer) > which means the original code is correct. You are right - but I think in my code it's immediately clear what the failure scenarios are. Anyway, I don't mind one way or the other, r=kaie
Attachment #752472 - Flags: review?(kaie) → review+
(In reply to Wan-Teh Chang from comment #27) > I just noticed that we can only call SECITEM_FreeItem when > |arena| is NULL. I will change this code to: > > if (!newlen) { > if (!arena) { > PORT_Free(item->data); > } > item->data = NULL; > item->len = 0; > return SECSuccess; > } > > Please also approve this change. Thanks. Thanks, I agree to this change, good catch. r=kaie Another general thought: I'm worried this is still risky code. This logic assumes that all callers are smart, and only the following scenarios (a) or (b) will happen: (a) both SECITEM_AllocItem and SECITEM_ReallocItemV2 pass NULL for arena or (b) neither SECITEM_AllocItem nor SECITEM_ReallocItemV2 passes NULL for arena But what happens if: (c) SECITEM_AllocItem passes an arena, but SECITEM_ReallocItemV2 passes NULL for arena In that case we'll attempt to call PORT_Free for memory that was taken from an arena. I wish we could eliminate that risk.
(In reply to Wan-Teh Chang from comment #26) > Created attachment 752481 [details] [diff] [review] > Make small adjustments to SECITEM_ReallocItemV2 > > 1. This specification is overly strong: > If then the item is kept unchanged and SECSuccess is returned. > the item already has at least the request new size, > In the current implementation, this comment is correct only when > we are using an arena. So I removed this comment. Agreed. I failed to update the comment after the code changes, thanks for fixing it. > 2. The standard C library function realloc() behaves like malloc() > if its |ptr| input argument is NULL. So I changed SECITEM_ReallocItemV2 > to match that. This requires changing the test > if (!item->len) { > to > if (!item->data) { Your change is OK, but I don't understand why this makes a difference, I'd assume they are always both 0/NULL. AllocItem with a zero length will keep both len and data at 0/NULL. With your other change in this patch, reallocating to len zero will set both to 0/NULL, too. r=kaie
Comment on attachment 752472 [details] [diff] [review] Revert an unnecessary change to nss/cmd/lib/secutil.c https://hg.mozilla.org/projects/nss/rev/a50886302a4c
Attachment #752472 - Flags: checked-in+
(In reply to Kai Engert (:kaie) from comment #29) > > Another general thought: > > I'm worried this is still risky code. ... > > But what happens if: > (c) SECITEM_AllocItem passes an arena, but SECITEM_ReallocItemV2 passes NULL > for arena > > In that case we'll attempt to call PORT_Free for memory that was taken from > an arena. That's correct. > I wish we could eliminate that risk. There is no field in the SECItem structure to store where the |data| pointer was allocated from. |data| can even point to a local or global variable. So the caller will need to be careful about passing a SECItem to SECITEM_ReallocItemV2 or SECITEM_FreeItem.
(In reply to Kai Engert (:kaie) from comment #30) > > > 2. The standard C library function realloc() behaves like malloc() > > if its |ptr| input argument is NULL. So I changed SECITEM_ReallocItemV2 > > to match that. This requires changing the test > > if (!item->len) { > > to > > if (!item->data) { > > Your change is OK, but I don't understand why this makes a difference, I'd > assume they are always both 0/NULL. Correct. My change makes the SECITEM_ReallocItemV2 code match the specification of realloc more closely. realloc doesn't know the old length, so the specification of realloc only takes about the data pointer input. It says: void *realloc(void *ptr, size_t size); ... If ptr is a null pointer, realloc() shall be equivalent to malloc() for the specified size. Note: the C standard allows malloc(0) to return a unique pointer that can be safely passed to free(), so in theory, if someone allocates the data of a SECItem manually as follows: SECItem item; unsigned int size = 0; item.type = siBuffer; item.data = PORT_Alloc(size); item.len = size; We could end up with data=non-null and len=0. Therefore, the assertion in the original code: if (!item->len) { /* allocate fresh block of memory */ PORT_Assert(!item->data); could fail in theory.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: