1006710 - use PR_ErrorInstallTable to add an error value/string description for key pinning failures

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

For key pinning, we should have an error specific to key pinning failures. I'm thinking something like SEC_ERROR_KEY_PINNING_VIOLATION but I'm open to suggestions.

Comment 1

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

Sgtm. Does this mean we get to change the string attached to the message, too?

Comment 2

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

Yes, I think part of adding the new error is also adding a string to describe it in https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/SECerrs.h

Comment 3

[Ryan Sleevi]

I'm mixed on this.

Key Pinning is not something that is implemented in NSS, in the same way that NSS doesn't implement cookies. The handling and storage of pins is something that's done at a layer higher, in the protocol-specific land of HTTP.

Note that the current implementation of Key Pinning in Mozilla is not even part of NSS, but the mozilla::pkix. It was, prior to that, being explored as an "application callback".

I'm supportive of providing descriptive errors, but perhaps this error code (and it's associated message) should be handled by the existing NSPR functionality to register your own error codes, rather than shoehorned into NSS.

Comment 4

[Ryan Sleevi]

Specifically, through the use of http://mxr.mozilla.org/nspr/source/pr/include/prerror.h#261

Comment 5

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

Sounds good.

Comment 6

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

Attachment: patch

Brian, I'd appreciate it if you would have a look at this. It's ready for review if you agree with the approach. We can discuss more if need be.
The idea is to add a class of errors (PSM_ERROR_...) in addition to the already-present SEC_ERROR_... and SSL_ERROR_... errors that would behave very similarly. A lot of the naming conventions don't entirely make sense anymore (e.g. IsNSSErrorCode), but I'm not sure what else to call these errors (maybe IsSecurityLayerErrorCode?)
The first error introduced identifies a key pinning failure.
Thanks!

Comment 7

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

Comment on attachment 8428152 [details] [diff] [review]
patch

Feedback from Camilo would be appreciated as well.

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 8428152 [details] [diff] [review]
patch

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

Great work, I really like this.

Comment 9

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

Comment on attachment 8428152 [details] [diff] [review]
patch

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

::: security/manager/ssl/src/NSSErrorsService.cpp
@@ +91,2 @@
>  
> +  *aXPCOMErrorCode = mozilla::psm::GetXPCOMFromNSSError(aNSPRCode);

See also PRErrorCode_to_nsresult in ScopedNSSTypes.h

::: security/manager/ssl/src/NSSErrorsService.h
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef nsINSSErrorsService_h
> +#define nsINSSErrorsService_h

Why not NSSErrorsService_h, since the header file is named NSSErrorsService.h?

::: security/manager/ssl/src/PSMErrors.cpp
@@ +8,5 @@
> +
> +namespace mozilla { namespace psm {
> +
> +static const struct PRErrorMessage PSMErrorTableText[] = {
> +  { "PSM_ERROR_KEY_PINNING_FAILURE", "The peer did not present an authorized key for this site" }

Please work with UX (perhaps in a separate bug) to improve this error message. This error message doesn't make any sense to anybody except us.

Shouldn't the error message end with a period?

Perhaps we should just replace "peer" with "server" since this is Gecko-specific already.

::: security/manager/ssl/src/PSMErrors.h
@@ +10,5 @@
> +#include "prerror.h"
> +
> +namespace mozilla { namespace psm {
> +
> +const int32_t PSM_ERROR_BASE = -0x4000;

How did you pick -0x4000 and how did you ensure that there are no collisions with other stuff in Gecko?

@@ +14,5 @@
> +const int32_t PSM_ERROR_BASE = -0x4000;
> +const int32_t PSM_ERROR_LIMIT = PSM_ERROR_BASE + 1000;
> +
> +enum PSMErrorCodes {
> +  PSM_ERROR_KEY_PINNING_FAILURE                   = (PSM_ERROR_BASE + 0)

Let's avoid all the extra spaces and just agree that we're not even going to try to do any pretty alignment.

Comment 10

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

Thanks for the review, Brian!

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #9)

> See also PRErrorCode_to_nsresult in ScopedNSSTypes.h

Thanks - I updated that too.

> ::: security/manager/ssl/src/NSSErrorsService.h
> @@ +2,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#ifndef nsINSSErrorsService_h
> > +#define nsINSSErrorsService_h
> 
> Why not NSSErrorsService_h, since the header file is named
> NSSErrorsService.h?

Yep - that was just a mistake.

> ::: security/manager/ssl/src/PSMErrors.cpp
> @@ +8,5 @@
> > +
> > +namespace mozilla { namespace psm {
> > +
> > +static const struct PRErrorMessage PSMErrorTableText[] = {
> > +  { "PSM_ERROR_KEY_PINNING_FAILURE", "The peer did not present an authorized key for this site" }
> 
> Please work with UX (perhaps in a separate bug) to improve this error
> message. This error message doesn't make any sense to anybody except us.

There's bug 1011638 for improving the pinning error UI. I don't think this error string will be exposed to end-users in the long run. I'm definitely open to suggestions for improving it, however.

> Shouldn't the error message end with a period?

Sounds good.

> Perhaps we should just replace "peer" with "server" since this is
> Gecko-specific already.

Ok.

> ::: security/manager/ssl/src/PSMErrors.h
> @@ +10,5 @@
> > +#include "prerror.h"
> > +
> > +namespace mozilla { namespace psm {
> > +
> > +const int32_t PSM_ERROR_BASE = -0x4000;
> 
> How did you pick -0x4000 and how did you ensure that there are no collisions
> with other stuff in Gecko?

I added some more documentation (and actually moved where this gets defined to nsINSSErrorsService.idl).
Long story short is the values for each Gecko error module has to fit in 16 bits. So, all we have to do here is make sure this doesn't overlap with existing NSS error codes (which it doesn't) and that it will fit in 16 bits when the Gecko error is synthesized (by negating the value and putting it in the NS_ERROR_MODULE_SECURITY module) (which it will).

> @@ +14,5 @@
> > +const int32_t PSM_ERROR_BASE = -0x4000;
> > +const int32_t PSM_ERROR_LIMIT = PSM_ERROR_BASE + 1000;
> > +
> > +enum PSMErrorCodes {
> > +  PSM_ERROR_KEY_PINNING_FAILURE                   = (PSM_ERROR_BASE + 0)
> 
> Let's avoid all the extra spaces and just agree that we're not even going to
> try to do any pretty alignment.

Sounds good.

I also got rid of PSMErrors.{h,cpp} since it basically served the same purpose as NSSErrorsService.{h,cpp}.

Comment 11

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

Attachment: patch v2

Comment 12

[Camilo Viecco (:cviecco)]

Comment on attachment 8429623 [details] [diff] [review]
patch v2

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

::: security/certverifier/CertVerifier.cpp
@@ +274,3 @@
>                                     verifyLog);
>          }
> +        PR_SetError(PSM_ERROR_KEY_PINNING_FAILURE, 0);

We should also probably note that we are not putting this error in the verifylog when using libpkix.

::: security/manager/ssl/src/NSSErrorsService.cpp
@@ +15,5 @@
>  namespace mozilla {
>  namespace psm {
>  
> +static const struct PRErrorMessage PSMErrorTableText[] = {
> +  { "PSM_ERROR_KEY_PINNING_FAILURE", "The server did not present an authorized key for this site." }

After reading Brian'comment I think a better error is:

The server certificate cannot be verified against the authorized keys for this site.

Comment 13

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

Comment on attachment 8429623 [details] [diff] [review]
patch v2

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

::: security/manager/ssl/src/NSSErrorsService.cpp
@@ +15,5 @@
>  namespace mozilla {
>  namespace psm {
>  
> +static const struct PRErrorMessage PSMErrorTableText[] = {
> +  { "PSM_ERROR_KEY_PINNING_FAILURE", "The server did not present an authorized key for this site." }

I suggest we word this in a way where at least somebody who knows about key pinning knows what the message is about. Right now, it isn't clear that this is really about key pinning. I suggest "The server uses key pinning (HPKP) and no certificate chain could be constructed that matches the pins. Key pinning violations cannot be overridden." It's still jargon, but at least *somebody* might understand it, even if end-users won't.

Thanks for letting me know about the "improve the key pinning UI" bug.

@@ +36,5 @@
> +IsPSMError(PRErrorCode error)
> +{
> +  return (error >= nsINSSErrorsService::PSM_ERROR_BASE &&
> +          error < nsINSSErrorsService::PSM_ERROR_LIMIT);
> +}

It may be better to just inline IsPSMError into IsNSSErrorCode so that nobody is tempted to call IsPSMError instead of IsNSSErrorCode.

::: security/manager/ssl/src/NSSErrorsService.h
@@ +20,5 @@
> +};
> +
> +void RegisterPSMErrorTable();
> +
> +bool IsPSMError(PRErrorCode error);

We don't need to declare this function in the header file. Nobody should call IsPSMError except IsNSSErrorCode; external callers should use IsNSSErrorCode.

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ -62,1 @@
>  }

Why not just delete PRErrorCode_to_nsresult, converting everything to use GetXPCOMFromNSSError?

Comment 14

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

Attachment: patch v2.1

Thanks, Brian! Addressed comments, carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=8c5dd90b5c8f

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dcd584751cc

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/6dcd584751cc