510135 - Don't need to decode policy OID in CERTPolicyInfo

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

After calling CERT_DecodeCertificatePoliciesExtension, the
policyInfos in the returned CERTCertificatePolicies already
contains the decoded policy OID (the 'oid' field):

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/polcyxtn.c&rev=1.11&mark=183,228#183

so it's not necessary to call SECOID_FindOIDTag again.

Also, use CERT_REV_M_IGNORE_MISSING_FRESH_INFO (the default)
to make it easier to understand the code.

Comment 1

[Alexei Volkov]

Comment on attachment 394189 [details] [diff] [review]
Proposed patch

r=alexei. I didn't know that we have this code in psm. :)

Comment 2

[Wan-Teh Chang]

Comment on attachment 394189 [details] [diff] [review]
Proposed patch

Pushed to mozilla-central in changeset 40ed0ff13420:
http://hg.mozilla.org/mozilla-central/rev/40ed0ff13420

Comment 3

[Wan-Teh Chang]

Attachment: Fix leaks of certificatePolicies and subjectAltName extensions

The leak of the certificatePolicies extension is more serious
because it leaks every time we verify a certificate for EV.  This
fix is worth backporting to older Firefox releases.

The leak of the subjectAltName extension occurs on out-of-memory
error paths.

Comment 4

[Alexei Volkov]

Comment on attachment 394504 [details] [diff] [review]
Fix leaks of certificatePolicies and subjectAltName extensions

r=alexei

Comment 5

[Wan-Teh Chang]

Comment on attachment 394504 [details] [diff] [review]
Fix leaks of certificatePolicies and subjectAltName extensions

I pushed this patch to mozilla-central in changeset c4beffbd4540:
http://hg.mozilla.org/mozilla-central/rev/c4beffbd4540

Comment 6

[Wan-Teh Chang]

Attachment: Fix leaks of certificatePolicies extensions (for mozilla-1.9.1)

This patch fixes leaks of decoded certificatePolicies
extensions.  This leak occurs whenever we verify a
certificate for EV, so I propose that we fix the leak
in mozilla-1.9.1.

Comment 7

[Wan-Teh Chang]

Comment on attachment 394504 [details] [diff] [review]
Fix leaks of certificatePolicies and subjectAltName extensions

The change to nsNSSIOLayer.cpp in this patch breaks
the Linux build because the jump to label 'loser'
crosses initialization of 'CERTGeneralName* current'.
So I backed out that change in changeset ef2e4699b319:
http://hg.mozilla.org/mozilla-central/rev/ef2e4699b319

Comment 8

[Wan-Teh Chang]

Attachment: Fix leaks of certificatePolicies extensions (pushed to mozilla-central)

So only this change to nsIdentityChecking.cpp is still
in mozilla-central.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 394689 [details] [diff] [review]
Fix leaks of certificatePolicies extensions (for mozilla-1.9.1)

r=kaie

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 394689 [details] [diff] [review]
Fix leaks of certificatePolicies extensions (for mozilla-1.9.1)

Needs to land in 1.9.2 first.

Comment 11

[Samuel Sidler (old account; do not CC)]

This needs approval on 1.9.2 as Dan says. Nominating it for blocking to get it on someone's radar.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 394689 [details] [diff] [review]
Fix leaks of certificatePolicies extensions (for mozilla-1.9.1)

a192=beltzner

Comment 13

[Wan-Teh Chang]

Comment on attachment 394689 [details] [diff] [review]
Fix leaks of certificatePolicies extensions (for mozilla-1.9.1)

I pushed this patch to mozilla-1.9.2 in changeset c4c08b8df2da:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c4c08b8df2da

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 394689 [details] [diff] [review]
Fix leaks of certificatePolicies extensions (for mozilla-1.9.1)

Why is there a hanging sr request on the earlier patch? Is it obsolete? Looks like it got checked in?

anyway, Approved for 1.9.1.4, a=dveditz for release-drivers on this patch.

Comment 15

[Wan-Teh Chang]

Comment on attachment 394689 [details] [diff] [review]
Fix leaks of certificatePolicies extensions (for mozilla-1.9.1)

I pushed this patch to mozilla-1.9.1 in changeset 3da4b7e1b598:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3da4b7e1b598