1029341 - Factor out extension parsing code from mozilla::pkix's pkixocsp into a reusable function

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: factor-out-OptionalExtensions.patch

I think we're testing extension-related stuff quite extensively (sorry) already, but let me know if you think we need more tests.

Comment 1

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

Comment on attachment 8444954 [details] [diff] [review]
factor-out-OptionalExtensions.patch

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

Cool. I'm not sure Bind2_2nd is necessary. Also, OptionalExtensions should be consistent about using/not using the der:: namespace. r=me with those comments addressed.
We do have pretty good test coverage, but it's not complete. When we get OCSP gtests, I think we'd be in a good position to add more.

::: security/pkix/include/pkix/bind.h
@@ +92,5 @@
>    void operator=(const Bind2&) /*= delete*/;
>  };
>  
> +template <typename R, typename B1, typename P1, typename B2>
> +class Bind2_2nd

I'm not seeing where this is used.

::: security/pkix/lib/pkixder.h
@@ +670,5 @@
> +    Input tagged;
> +    if (ExpectTagAndGetValue(input, tag, tagged) != der::Success) {
> +      return Failure;
> +    }
> +    if (ExpectTagAndGetValue(tagged, der::SEQUENCE, extensions) != der::Success) {

We're already in the der:: namespace, right? 'der::SEQUENCE', 'der::Success' etc. shouldn't be necessary unless for stylistic decisions (in which case, each Failure, OIDTag, etc. should be 'der::Failure', 'der::OIDTag', etc. to be consistent).

Comment 2

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

I removed the bind() overload I added. It was for a different implementation of this patch that used der::Nested. I also removed all the der:: namespace qualifiers. Thanks for catching these oversights and thanks for the quick review!

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

Comment 3

[Carsten Book [:Tomcat]]

sorry had to back this out in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=152f6b19ad9c since one of this 2 csets seems to caused a bustage in device builds and B2g Emulator like https://tbpl.mozilla.org/php/getParsedLog.php?id=42422444&tree=Mozilla-Inbound

Comment 4

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

(In reply to Carsten Book [:Tomcat] from comment #3)
> sorry had to back this out in
> https://tbpl.mozilla.org/?tree=Mozilla-
> Inbound&onlyunstarred=1&rev=152f6b19ad9c since one of this 2 csets seems to
> caused a bustage in device builds and B2g Emulator like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42422444&tree=Mozilla-
> Inbound

The backout was caused by another patch that landed at the same time. Relanding:
https://hg.mozilla.org/integration/mozilla-inbound/rev/904975e569b5

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/904975e569b5

Comment 6

[Gary [:streetwolf52]]

Regression checking has lead me to this patch which is causing problems with LastPass.  I get a message that LP was unable to login to it's secure server.  Furthermore, when I try to install the latest version of LP via this link https://lastpass.com/lppre.xpi I get the following error message:

-------
Secure Connection Failed

An error occurred during a connection to lastpass.com. The response from the OCSP server was corrupted or improperly formed. (Error code: sec_error_ocsp_malformed_response)

The page you are trying to view cannot be shown because the authenticity of the received data could not be verified.
Please contact the website owners to inform them of this problem.
------

Comment 7

[Gary [:streetwolf52]]

Regression...

Good:  https://hg.mozilla.org/integration/mozilla-inbound/rev/11ce0e5823f4

Bad:   https://hg.mozilla.org/integration/mozilla-inbound/rev/904975e569b5

Comment 8

[Gary [:streetwolf52]]

Filed new bug report https://bugzilla.mozilla.org/show_bug.cgi?id=1031022