178898 - ASN.1 decoder used without arena in lib/crmf

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Robert Relyea]

The use of SEC_ASN1Decode and SECASN1DecodeItem should be examined in lib/crmf
to see if we can use the quick decoder instead.

Note: be very careful to get these right. all.sh has little or no coverage of
the crmf functionality.

Comment 1

[Julien Pierre]

Taking bug.

Comment 2

[Julien Pierre]

Performance enhancement. Reprioritizing to P3.

Comment 3

[Julien Pierre]

I reviewed the two calls to SEC_ASN1DecodeItem in crmf. Both are without arenas,
and it would be intrusive to add them for QuickDER, so I'm going to close this bug.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Julien,  do we know that these calls never leak memory?

Perhaps this bug should be reopenened and morph into a bug about 
plugging leaks in the CRMF lib's use of the decoders ?

Comment 5

[Julien Pierre]

No, we don't know that. They could leak memory if invalid data is fed, or if the
freeing code for CRMF is incomplete. In this regard, it would make sense to
convert the code to arenas . If you want to do this, feel free to reopen and
take the bug, since you have been working on the CRMF code lately.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

As Julien reported above, the CRMF lib uses the ASN.1 decoder without 
an arenapool which potentially leaks memory.  Since I'm working on 
the CRMF lib, I will take this and fix it.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

taking  (wish I doulc do this in less than 3 steps).

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch does 2 things:
a) ensures that we always have a non-null pool pointer when calling the
   ASN1 Decoder, and
b) rewrites crmf_get_public_value to ensure that it always detects 
   allocation failures, and returns NULL when they occur.  

Julien, do you thin the changes to crmf_decode_params were unnecessary?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151996 [details] [diff] [review]
patch v1

Bob, please review

Comment 10

[Robert Relyea]

Comment on attachment 151996 [details] [diff] [review]
patch v1

Looks fine, though I would like the first change in asn1cmn.c changed as
follows:

1) move the test for poolp not NULL to the begining of the case (just before
the line "inCertOrEncCert->cert.encrytpedCert = ..." (prefered). or remove the
new test altogether. Rationale: if we are going to test the validity of the
poolp, we should do so before we first use it;).

I'll leave it to Julian to answer the question on crmf_decode_params. If the
changes are necessare the given changes look correct to me.

bob

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/crmf/asn1cmn.c,v  <--  asn1cmn.c
new revision: 1.9; previous revision: 1.8

/cvsroot/mozilla/security/nss/lib/crmf/challcli.c,v  <--  challcli.c
new revision: 1.5; previous revision: 1.4

/cvsroot/mozilla/security/nss/lib/crmf/crmfcont.c,v  <--  crmfcont.c
new revision: 1.7; previous revision: 1.6

/cvsroot/mozilla/security/nss/lib/crmf/crmfdec.c,v  <--  crmfdec.c
new revision: 1.4; previous revision: 1.3

/cvsroot/mozilla/security/nss/lib/crmf/crmfi.h,v  <--  crmfi.h
new revision: 1.3; previous revision: 1.2


tested with
crmftest -d /tmp/client -p TestUser -s TestCA crmf dsa decode cmmf

Comment 12

[Julien Pierre]

Nelson,

Sorry I'm late to the party here ... But I had this comment about the patch :

I find your use of SECITEM_ArenaDupItem(NULL, ...) a little confusing .

If you aren't going to use an arena, you may as well call SECITEM_DupItem(...)
instead, which will be the same thing, but more readable.

On the other hand, since part of these interfaces are static, perhaps it's worth
changing more of the code and not return a SECITEM allocated from the global
heap. Maybe the next time this code gets looked at.