Closed Bug 1296317 Opened 8 years ago Closed 8 years ago

Stop calling PR_SetError() in VerifyCert() and VerifySSLServerCert()

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Bug 1296317 - Stop calling PR_SetError() in VerifyCert() and VerifySSLServerCert().
58 bytes, text/x-review-board-request
keeler
: review+
Details 
The PR_SetError() + PR_GetError() pattern currently used is error prone and unnecessary.
The functions involved can instead return pkix::Result, which is equally expressive and more robust.
Comment on attachment 8796786 [details]
Bug 1296317 - Stop calling PR_SetError() in VerifyCert() and VerifySSLServerCert().

https://reviewboard.mozilla.org/r/82520/#review81948

I think this is a good change to make. There are a few things (mostly nits) I'd like to see changed, though. Basically, I think we should probably be using "mozilla::pkix::type" instead of "pkix::type" unless there's a good reason not to. Similarly, it looks like most "Result"s are called `rv` (unless there's already another `nsresult rv` or something), so let's do that as much as possible too. Last, it looks like GetXPCOMFromNSSError will assert if given 0 as input, so we should watch out for that.

::: security/certverifier/CertVerifier.h:100
(Diff revision 1)
>      OCSP_STAPLING_INVALID = 4,
>    };
>  
>    // *evOidPolicy == SEC_OID_UNKNOWN means the cert is NOT EV
>    // Only one usage per verification is supported.
> -  SECStatus VerifyCert(CERTCertificate* cert,
> +  pkix::Result VerifyCert(CERTCertificate* cert,

nit: elsewhere in this file we use the full mozilla::pkix::type - we should probably be consistent

::: security/certverifier/CertVerifier.cpp:840
(Diff revision 1)
> -  if (rv != SECSuccess) {
> -    return rv;
> +  if (result != Success) {
> +    return result;
>    }
>  
>    Input peerCertInput;
> -  Result result = peerCertInput.Init(peerCert->derCert.data,
> +  result = peerCertInput.Init(peerCert->derCert.data, peerCert->derCert.len);

I think this Result was called `result` because we already had a SECStatus `rv`, but now we can just call it `rv` (which appears to be more common).

::: security/manager/ssl/nsDataSignatureVerifier.cpp:265
(Diff revision 1)
> -  return MapSECStatus(certVerifier->VerifyCert(cert,
> +  Result result = certVerifier->VerifyCert(cert,
> -                                               certificateUsageObjectSigner,
> +                                           certificateUsageObjectSigner,
> -                                               Now(), pinArg,
> +                                           Now(), pinArg,
> -                                               nullptr, // hostname
> +                                           nullptr, // hostname
> -                                               context->builtChain));
> +                                           context->builtChain);
> +  return GetXPCOMFromNSSError(MapResultToPRErrorCode(result));

If I'm reading GetXPCOMFromNSSError correctly ( https://dxr.mozilla.org/mozilla-central/rev/42c95d88aaaa7c2eca1d278399421d437441ac4d/security/manager/ssl/NSSErrorsService.cpp#79 ) if VerifyCert returns Success, this will crash.

::: security/manager/ssl/nsNSSCertificate.cpp:844
(Diff revision 1)
>    if (certVerifier->VerifyCert(mCert.get(), certificateUsageSSLServer, now,
>                                 nullptr, /*XXX fixme*/
>                                 nullptr, /* hostname */
>                                 nssChain,
> -                               CertVerifier::FLAG_LOCAL_ONLY) != SECSuccess) {
> +                               CertVerifier::FLAG_LOCAL_ONLY)
> +        != pkix::Success) {

nit: looks like this file has a mix of mozilla::pkix::type and pkix::type - I would prefer we use mozilla::pkix::type

::: security/manager/ssl/nsNSSCertificate.cpp:1317
(Diff revision 1)
>    resultOidTag = SEC_OID_UNKNOWN;
>  
>    uint32_t flags = mozilla::psm::CertVerifier::FLAG_LOCAL_ONLY |
>      mozilla::psm::CertVerifier::FLAG_MUST_BE_EV;
>    UniqueCERTCertList unusedBuiltChain;
> -  SECStatus rv = certVerifier->VerifyCert(mCert.get(),
> +  pkix::Result result = certVerifier->VerifyCert(mCert.get(),

nit: again, let's call this `rv`

::: security/manager/ssl/nsNSSCertificateDB.cpp:617
(Diff revision 1)
>      if (!node->cert) {
>        continue;
>      }
>  
>      UniqueCERTCertList certChain;
> -    SECStatus srv = certVerifier->VerifyCert(node->cert,
> +    pkix::Result result = certVerifier->VerifyCert(node->cert,

I guess since we already have an `rv` this should stay as `result`. It would be nice to use mozilla::pkix::Result, though.
Attachment #8796786 - Flags: review?(dkeeler) → review-
Comment on attachment 8796786 [details]
Bug 1296317 - Stop calling PR_SetError() in VerifyCert() and VerifySSLServerCert().

https://reviewboard.mozilla.org/r/82520/#review81948

> nit: elsewhere in this file we use the full mozilla::pkix::type - we should probably be consistent

Sure. I tend to use pkix::Result because it's shorter, but I don't mind either way.

> I think this Result was called `result` because we already had a SECStatus `rv`, but now we can just call it `rv` (which appears to be more common).

Sounds good.

> If I'm reading GetXPCOMFromNSSError correctly ( https://dxr.mozilla.org/mozilla-central/rev/42c95d88aaaa7c2eca1d278399421d437441ac4d/security/manager/ssl/NSSErrorsService.cpp#79 ) if VerifyCert returns Success, this will crash.

Oh, good catch!
I wonder why I didn't hit this during testing... Will investigate.

> nit: again, let's call this `rv`

Hmm, so the style guide says this: "local nsresult result codes should be `rv`. `rv` should not be used for bool or other result types".
I think it's fine to ignore this for methods that don't return `nsresult`, but this method still returns `nsresult`.
Should we follow the rule here?
See comment 3.
Flags: needinfo?(dkeeler)
(In reply to :Cykesiopka from comment #3)
> > If I'm reading GetXPCOMFromNSSError correctly ( https://dxr.mozilla.org/mozilla-central/rev/42c95d88aaaa7c2eca1d278399421d437441ac4d/security/manager/ssl/NSSErrorsService.cpp#79 ) if VerifyCert returns Success, this will crash.
> 
> Oh, good catch!
> I wonder why I didn't hit this during testing... Will investigate.

The only test I could find that exercised this code path was modules/libjar/test/chrome/test_bug386153.html, not any PSM tests, so ¯\_(ツ)_/¯

> > nit: again, let's call this `rv`
> 
> Hmm, so the style guide says this: "local nsresult result codes should be
> `rv`. `rv` should not be used for bool or other result types".
> I think it's fine to ignore this for methods that don't return `nsresult`,
> but this method still returns `nsresult`.
> Should we follow the rule here?

Oh, whoops - yes, for functions that return nsresult, let's not call a Result rv.
Flags: needinfo?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> The only test I could find that exercised this code path was
> modules/libjar/test/chrome/test_bug386153.html, not any PSM tests, so
> ¯\_(ツ)_/¯

Ugh. I filed Bug 1308545 to add PSM level tests.
Blocks: 1308548
Comment on attachment 8796786 [details]
Bug 1296317 - Stop calling PR_SetError() in VerifyCert() and VerifySSLServerCert().

https://reviewboard.mozilla.org/r/82520/#review82952

Great - r=me.
Attachment #8796786 - Flags: review?(dkeeler) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12c51a960f26
Stop calling PR_SetError() in VerifyCert() and VerifySSLServerCert(). r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12c51a960f26
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: