1010581 - Document Expect/match/skip naming convention in mozilla::pkix::der and make the code more consistent in this respect

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: ClarifyExpectMatchSkip.patch

See bug 1006041 comment 3. Filing this as a separate bug because there are changes beyond the comment that keeler asked for.

In the course of making the naming more consistent I found that some of the original uses of mozilla::pkix::der were using repeated patterns that had already been abstracted into their own functions in mozilla::pkix::der, so I simplified that code in this patch too.

Comment 1

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

Attachment: fix-definition-order-issue.patch

This patch moves the definitions of ExpectTagAndGetValue ahead of the definitions of all the functions that call it. Otherwise clang and GCC are not happy (and justifiably so, according to C++11, at least).

Comment 2

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

Comment on attachment 8422776 [details] [diff] [review]
ClarifyExpectMatchSkip.patch

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

Looks great. Just one question, but otherwise r=me.

::: security/pkix/lib/pkixocsp.cpp
@@ +847,5 @@
>    if (input.Peek(der::BOOLEAN)) {
>      return der::Fail(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);
>    }
>  
> +  input.SkipToEnd();

Do we not want to at least make sure the rest of the extension is an OCTET STRING and nothing more?

Comment 3

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

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #2)
> Do we not want to at least make sure the rest of the extension is an OCTET
> STRING and nothing more?

My thinking is that mozilla::pkix is not lint, so don't check things that aren't useful to check. We're not trying to find every little problem with every OCSP response or certificate, we're only trying to figure out if the certificate is good or bad.

You've probably noticed that we have some checks to make sure we've parsed an entire (sub-)sequence that aren't strictly needed, and that causes us to add calls to SkipToEnd(). IMO, those not-strictly-needed checks are a good idea because it helps stop us from accidentally skipping the parsing of stuff. Here, we don't gain anything by checking that the things is really an octet string.

Comment 4

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

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

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

Attachment: backport-ExpectTagAndGetValue-Beta.patch

Approval Request Comment
[Feature/regressing bug #]: bug 915930
[User impact if declined]: null pointer dereference due to not being able to backport the fix for 1031542.
[Describe test coverage new/current, TBPL]: This is tested extensively using the tests in security/pkix/test/gtest. I will backport the necessary tests along with the patch.
[Risks and why]: Very low risk. This code has been working fine in tests and on Mozilla-Central and Mozilla-Aurora for several weeks.
[String/UUID change made/needed]: none

Comment 7

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

https://hg.mozilla.org/releases/mozilla-beta/rev/91835441918e