1040446 - Improve error code names/descriptions for invalid basic constraints

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Now that we have an easy-to-customize error reporting mechanism in pkix/Result.h and pkix/pkixnss.h, we should improve the error codes returned for the case of invalid basic constraints. The badness is described by this comment in the source code:

      // We use Result::ERROR_CA_CERT_INVALID here so we can distinguish
      // this error from other errors, given that NSS does not have a "CA cert
      // used as end-entity" error code since it doesn't have such a
      // prohibition. We should add such an error code and stop abusing
      // Result::ERROR_CA_CERT_INVALID this way.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

Brian, let me know if you have time to review patches like this. If not, I can find another reviewer.

Comment 2

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

Comment on attachment 8471025 [details] [diff] [review]
patch

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

I don't have an opinion on the wording of the error message text, other than it seems better than the error message text that is currently being presented for this problem.

Comment 3

[Dana Keeler (she/her) [:keeler]]

Great - thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8b92c9ad63

Comment 4

[Ed Morley [:emorley]]

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

Comment 5

[Dana Keeler (she/her) [:keeler]]

Attachment: patch for beta

Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix (this is needed for bug 1034124, which fixes a significant compatibility issue)
[User impact if declined]: see bug 1034124
[Describe test coverage new/current, TBPL]: there is adequate test coverage
[Risks and why]: low
[String/UUID change made/needed]: none (the lack of localizable strings is handled in bug 1052529)

Comment 6

[Dana Keeler (she/her) [:keeler]]

[Tracking Requested - why for this release]: see comment 5

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8486075 [details] [diff] [review]
patch for beta

Approving this prereq for bug 1034124 and bug 1039064 for beta.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8486075 [details] [diff] [review]
patch for beta

This doesn't apply to beta at all.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8486075 [details] [diff] [review]
patch for beta

Ah, this applies on top of bug 1039064. Still holding off on pushing this until bug 1034124 comment 25 is sorted out, though.

Comment 10

[Dana Keeler (she/her) [:keeler]]

Yes, sorry - looks like you've got it figured out, but in any case the order should be bug 1039064, then this bug, then bug 1034124.

Comment 11

[Dana Keeler (she/her) [:keeler]]

https://hg.mozilla.org/releases/mozilla-beta/rev/15c382469fd1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta per bug 1039064 comment 43.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/93cd4a068e9d