1083539 - Factor out common DER sequence unwrapping logic into reusable functions

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: factor-out-common-SEQUENCE-unwrapping-logic.patch

See patch. More uses of these functions will appear in the name constraints patches. In particular, NestedExpectTagAndGetValueAtEnd seems pretty specialized, but it is also used in for name constraints.

Comment 1

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4a06c58998

Thanks for the quick review!

Comment 2

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

Comment on attachment 8505823 [details] [diff] [review]
factor-out-common-SEQUENCE-unwrapping-logic.patch

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

::: security/pkix/lib/pkixder.h
@@ +249,5 @@
> +}
> +
> +// Similar to the above, but does two levels of unwrapping.
> +inline Result
> +NestedExpectTagAndGetValueAtEnd(Reader& outer, uint8_t expectedOuterTag,

I thought that I needed this NestedExpectTagAndGetValueAtEnd function for the new name constraint implementation. However, it turns out I was wrong. Therefore, I removed it from the patch before checkin.

::: security/pkix/lib/pkixocsp.cpp
@@ +411,4 @@
>      Reader certsSequence;
> +    rv = der::NestedExpectTagAndGetValueAtEnd(
> +           input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0,
> +           der::SEQUENCE, certsSequence);

Because I removed the NestedExpectTagAndGetValueAtEnd function, I replaced this call with two calls to ExpectTagAndGetValueAtEnd.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Fix dropped return value check (

Comment 4

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

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #3)
> Created attachment 8510701 [details] [diff] [review]
> Fix dropped return value check (

Thank you for noticing this, and thanks for fixing it. How did you find it? Is somebody building Gecko in a way that causes this type of error (unused assignment) to be found, in a way that isn't found on tryserver and inbound?

Comment 5

[Tom S [:evilpie]]

I found it by looking at the diff.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c4a06c58998

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Yep, we have evilpie to thank.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 9

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

(In reply to Tom Schuster [:evilpie] from comment #5)
> I found it by looking at the diff.

An incredible innovation in security! Thanks for finding this! I filed bug 1088845 so that we can make this type of bug never happen again in mozilla::pkix.

Comment 10

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

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