1001188 - mozilla::pkix's BuildForward function returns RecoverableError without calling PR_SetError

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

if (subCACount >= MAX_DEPTH - 1) {
-    return RecoverableError;
+    return Fail(Result::RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
  }

Comment 1

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

Attachment: max-cert-chain-length-fix.patch

Comment 2

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

Attachment: max-cert-chain-length-tests.patch

WHen I run this test without the fix, I get a test failure indicating that the error code was not set. With the fix applied, the tests pass.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8432151 [details] [diff] [review]
max-cert-chain-length-tests.patch

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

Looks good to me. The only reason why I am giving the r- instead of an r+ is that I dont know how these tests are plugged into our automation suite. Please give me a link or document that they are not run automatically for the r+.

Comment 4

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8432151 [details] [diff] [review]
max-cert-chain-length-tests.patch

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

Does `./mach gtest` not run this?

Comment 5

[Camilo Viecco (:cviecco)]

Comment on attachment 8432151 [details] [diff] [review]
max-cert-chain-length-tests.patch

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

From https://developer.mozilla.org/en-US/docs/GTest
"... Gtests currently run as part of 'make -C testing/gtest check' in the B section of tbpl ..."

Comment 6

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

Folded both patches together and checked in. Thanks for the quick reviews!

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

Comment 7

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

Comment on attachment 8432149 [details] [diff] [review]
max-cert-chain-length-fix.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 915930 (one of the bugs it depends on)
User impact if declined: The error message reported for a certificate check failure may not match the actual error and/or the user may not be able to override an error that is supposed to be overridable.
Testing completed (on m-c, etc.): Just landed. There are automated tests.
Risk to taking this patch (and alternatives if risky): Very low. This is a one line patch to call the PR_SetError function, plus a few hundred lines of testing.

String or IDL/UUID changes made by this patch: Never!

Comment 8

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

Comment on attachment 8432151 [details] [diff] [review]
max-cert-chain-length-tests.patch

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

See the approval request for the previous patch.

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Needs a branch patch for Fx31. Note that the uplift is on Monday, so this needs to be landed on Aurora ASAP.

Comment 11

[Camilo Viecco (:cviecco)]

The fix for the issue I have a patch, for the tests, the storty is different I need to collapse and remove several changesets. so far I know I need:
1 A 2a5624d51bd3
2 A 7a515c1d5a21
3 A 7176ac3db029
4 A 64ce640f432e
5 A dfc04fd0a41f
6 A 69d7eaad0a50
7 A d03cc4b6832c

as soon as I have a miminal change I will ask you for aproval of the tests. I this seems too much change now we can leave the tests out, but lets be hpefull and see what we really need.

Comment 12

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

(In reply to Camilo Viecco (:cviecco) from comment #11)
> The fix for the issue I have a patch, for the tests, the storty is different
> I need to collapse and remove several changesets. so far I know I need:
> 
> As soon as I have a miminal change I will ask you for aproval of the tests.
> I this seems too much change now we can leave the tests out, but lets be
> hpefull and see what we really need.

It looks like Camilo is taking care of it. Thanks Camilo! (FWIW, I think it is OK to uplift the fix without the test, since the fix is trivial.)

Comment 13

[Camilo Viecco (:cviecco)]

The test patch is big and messy. Lets land the fix only.
 https://tbpl.mozilla.org/?tree=Try&rev=99a4dc45cea9

Comment 14

[Camilo Viecco (:cviecco)]

try build looks green(of the patch wihout tests) I will push to aurora as soon as the final xpchell tests on windows finish unless you ping me not to.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Aurora's green, go for it. Please do set status-firefox31 to fixed when you do, though :)

Comment 16

[Camilo Viecco (:cviecco)]

pushed to aurora  
https://hg.mozilla.org/releases/mozilla-aurora/rev/2415441c3620