Factor out common DER sequence unwrapping logic into reusable functions

RESOLVED FIXED in mozilla36

Status

()

Core
Security: PSM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: briansmith, Assigned: mmc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 8505823 [details] [diff] [review]
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.
Attachment #8505823 - Flags: review?(mmc)
Attachment #8505823 - Flags: review?(mmc) → review+
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.
Created attachment 8510701 [details] [diff] [review]
Fix dropped return value check (
Assignee: brian → mmc
Status: NEW → ASSIGNED
Attachment #8510701 - Flags: review?(evilpies)
Attachment #8510701 - Flags: review?(dkeeler)
Attachment #8510701 - Flags: review?(dkeeler) → review+
Attachment #8510701 - Flags: review?(evilpies)
(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?
Flags: needinfo?(mmc)
I found it by looking at the diff.
https://hg.mozilla.org/mozilla-central/rev/4c4a06c58998
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Yep, we have evilpie to thank.
Flags: needinfo?(mmc)
(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.
You need to log in before you can comment on or make changes to this bug.