Closed Bug 1010581 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

Details

Attachments

(3 files)

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.
Attachment #8422776 - Flags: review?(dkeeler)
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).
Attachment #8422877 - Flags: review?(dkeeler)
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?
Attachment #8422776 - Flags: review?(dkeeler) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/fe7bffe6bb06
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Attachment #8447582 - Flags: review+
Attachment #8447582 - Flags: approval-mozilla-beta?
Attachment #8447582 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.