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)
Tracking
(Not tracked)
RESOLVED
FIXED
3.15
People
(Reporter: mharmsen, Assigned: KaiE)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 4 obsolete files)
1.96 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
5.79 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
840 bytes,
patch
|
KaiE
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•13 years ago
|
Assignee | ||
Comment 1•13 years ago
|
||
Are you always using the same fixed filenames?
Could there potentially be a race by two parallel uses of the same filenames?
Assignee | ||
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•12 years ago
|
||
I had not seen the analysis in the referenced bug, which explains the problem. I'll work on a patch.
Assignee | ||
Comment 3•12 years ago
|
||
Reallocate and add a zero terminator if necessary.
Assignee: nobody → kaie
Attachment #738995 -
Flags: review?(rrelyea)
Comment 4•12 years ago
|
||
Comment on attachment 738995 [details] [diff] [review]
Patch v1
r+ rrelyea
Attachment #738995 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 5•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.15
Assignee | ||
Comment 6•12 years ago
|
||
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 → ---
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #739785 -
Flags: review?(rrelyea)
Comment 8•12 years ago
|
||
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-
Assignee | ||
Comment 9•12 years ago
|
||
(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 10•12 years ago
|
||
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+
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
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-
Assignee | ||
Comment 13•12 years ago
|
||
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)
Assignee | ||
Comment 14•12 years ago
|
||
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.
Comment 15•12 years ago
|
||
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 16•12 years ago
|
||
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-
Assignee | ||
Comment 17•12 years ago
|
||
> > +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.
Assignee | ||
Comment 18•12 years ago
|
||
> 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.
Assignee | ||
Comment 19•12 years ago
|
||
> 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.
Assignee | ||
Comment 20•12 years ago
|
||
(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;
}
Assignee | ||
Comment 21•12 years ago
|
||
Attachment #739967 -
Attachment is obsolete: true
Attachment #739967 -
Flags: review?(rrelyea)
Attachment #743662 -
Flags: superreview?(wtc)
Attachment #743662 -
Flags: review?(rrelyea)
Assignee | ||
Comment 22•12 years ago
|
||
We should try to get this into 3.15
Comment 23•12 years ago
|
||
Comment on attachment 743662 [details] [diff] [review]
additional patch v5
r+ rrelyea
Attachment #743662 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 24•12 years ago
|
||
Thanks for the review, additional patch checked in, too.
https://hg.mozilla.org/projects/nss/rev/d0cf1feff4dc
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Attachment #743662 -
Flags: superreview?(wtc)
Updated•12 years ago
|
Priority: -- → P2
Comment 25•12 years ago
|
||
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)
Comment 26•12 years ago
|
||
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 27•12 years ago
|
||
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.
Assignee | ||
Comment 28•12 years ago
|
||
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+
Assignee | ||
Comment 29•12 years ago
|
||
(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.
Assignee | ||
Comment 30•12 years ago
|
||
(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 31•12 years ago
|
||
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+
Comment 32•12 years ago
|
||
(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.
Comment 33•12 years ago
|
||
(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.
Comment 34•12 years ago
|
||
Attachment #752481 -
Attachment is obsolete: true
Attachment #752481 -
Flags: review?(kaie)
Attachment #752871 -
Flags: checked-in+
You need to log in
before you can comment on or make changes to this bug.
Description
•