978528 - The wrong error code is returned when no candidate issuers can be found when using insanity::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

AppTrustDomain::FindPotentialIssuers and NSSCertDBTrustDomain::FindPotentialIssuers both incorrectly assume that CERT_CreateSubjectCertList calls PR_SetError when it returns nullptr. However, it doesn't (always) do that, because nullptr is a valid success result meaning "no potential issuers found."

This is the cause of bug 978117.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Return the correct error message when no potential issuers are found during path bulding in insanitY::pkix, r?keeler

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8384341 [details] [diff] [review]
Return the correct error message when no potential issuers are found during path bulding in insanitY::pkix, r?keeler

Review of attachment 8384341 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with check reloation or explanation.

::: security/insanity/lib/pkixbuild.cpp
@@ +230,5 @@
>    ScopedCERTCertList candidates;
>    if (trustDomain.FindPotentialIssuers(&subject.GetNSSCert()->derIssuer, time,
>                                         candidates) != SECSuccess) {
>      return MapSECStatus(SECFailure);
>    }

move this assert after the check for !candidates. or make the !candidates condition check also check for !canidates.get()

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8384341 [details] [diff] [review]
Return the correct error message when no potential issuers are found during path bulding in insanitY::pkix, r?keeler

Review of attachment 8384341 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/insanity/lib/pkixbuild.cpp
@@ +230,5 @@
>    ScopedCERTCertList candidates;
>    if (trustDomain.FindPotentialIssuers(&subject.GetNSSCert()->derIssuer, time,
>                                         candidates) != SECSuccess) {
>      return MapSECStatus(SECFailure);
>    }

The overloaded operator!() on ScopedPtr makes (!candidates) equivalent to (!candidates.get()) already.

Thanks for the review!

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d16477b89f70

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8384341 [details] [diff] [review]
Return the correct error message when no potential issuers are found during path bulding in insanitY::pkix, r?keeler

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 878932.
User impact if declined: Confusing error messages. Less automated test coverage for certificate verification for TLS certificates and for packaged app signature verification.
Testing completed (on m-c, etc.): This is covered by the automated tests in bug 978117.
Risk to taking this patch (and alternatives if risky): Very low risk. This is a very simple change.
String or IDL/UUID changes made by this patch: Never!

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d16477b89f70

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Wes, don't hesitate to update status-firefox30 when you merge the commit in m-c.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Wes, don't hesitate to update status-firefox30 when you merge the commit in
> m-c.

We use automated marking tools that currently only set it to fixed if it's tracking that release and don't give us the option otherwise. So easier said than done ;)

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8384341 [details] [diff] [review]
Return the correct error message when no potential issuers are found during path bulding in insanitY::pkix, r?keeler

This didn't get uplifted in time. Requesting approval for beta.

[Approval Request Comment]
see comment 5

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/releases/mozilla-beta/rev/816c209eaa71

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android xpcshell failures.
https://hg.mozilla.org/releases/mozilla-beta/rev/e5d922ae5641

https://tbpl.mozilla.org/php/getParsedLog.php?id=36678630&tree=Mozilla-Beta

I tried pinging you on IRC and couldn't get ahold of you. A reminder that the tree rules explicitly say that *you* are responsible for watching your pushes to the release branches.
https://wiki.mozilla.org/Tree_Rules

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

David, there are so many bugs in mozilla::pkix that won't get fixed in Firefox 29 that I don't think it is worth spending more time on this. I suggest we WONTFIX for Firefox 29.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> I tried pinging you on IRC and couldn't get ahold of you. A reminder that
> the tree rules explicitly say that *you* are responsible for watching your
> pushes to the release branches.
> https://wiki.mozilla.org/Tree_Rules

I'll keep a closer watch in the future.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #12)
> David, there are so many bugs in mozilla::pkix that won't get fixed in
> Firefox 29 that I don't think it is worth spending more time on this. I
> suggest we WONTFIX for Firefox 29.

I agree.