Closed Bug 1006710 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Sgtm. Does this mean we get to change the string attached to the message, too?
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
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.
Sounds good.
Assignee: nobody → nobody
Component: Libraries → Security: PSM
Product: NSS → Core
Summary: add an (NSS) error indicating a key pinning failure → use PR_ErrorInstallTable to add an error value/string description for key pinning failures
Version: trunk → Trunk
Attached patch patch (obsolete) — Splinter Review
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!
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8428152 - Flags: review?(brian)
Comment on attachment 8428152 [details] [diff] [review]
patch

Feedback from Camilo would be appreciated as well.
Attachment #8428152 - Flags: feedback?(cviecco)
Comment on attachment 8428152 [details] [diff] [review]
patch

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

Great work, I really like this.
Attachment #8428152 - Flags: feedback?(cviecco) → feedback+
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.
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}.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8428152 - Attachment is obsolete: true
Attachment #8428152 - Flags: review?(brian)
Attachment #8429623 - Flags: review?(brian)
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.
Attachment #8429623 - Flags: feedback+
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?
Attachment #8429623 - Flags: review?(brian) → review+
Attached patch patch v2.1Splinter Review
Thanks, Brian! Addressed comments, carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=8c5dd90b5c8f
Attachment #8429623 - Attachment is obsolete: true
Attachment #8430378 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6dcd584751cc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Duplicate of this bug: 1006713
You need to log in before you can comment on or make changes to this bug.