Closed Bug 1382866 Opened 6 years ago Closed 6 years ago

CERT_ChangeCertTrust can fail if the user hasn't logged in to softoken

Categories

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

Product:
Core
Component:
Security: PSM
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

bug 1382866 - prompt for authentication when changing certificate trust fails
59 bytes, text/x-review-board-request
Cykesiopka
: review+
jcj
: review+
Details 
CERT_ChangeCertTrust can fail with SEC_ERROR_TOKEN_NOT_LOGGED_IN if the user hasn't logged in to the softoken. This is particularly significant when we switch to the sql-backed softoken. We can probably just call PK11_Authenticate like certutil does: https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/security/nss/cmd/certutil/certutil.c#151
Comment on attachment 8889647 [details]
bug 1382866 - prompt for authentication when changing certificate trust fails

https://reviewboard.mozilla.org/r/160682/#review166006

This looks good to me.

::: security/manager/ssl/nsNSSCertificateDB.cpp
(Diff revision 1)
> -    // always start with untrusted and move up
> -    trust.SetValidPeer();
> +      trust.SetValidPeer();
> -    trust.AddPeerTrust(0, !!(trusted & nsIX509CertDB::TRUSTED_EMAIL), 0);
> +      trust.AddPeerTrust(0, !!(trusted & nsIX509CertDB::TRUSTED_EMAIL), 0);
> -    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(),
> -                               nsscert.get(),
> +      break;
> +    default:
> -                               trust.GetTrust());

Nit... Might want to keep the comment about ignoring user certs?

::: security/manager/ssl/nsNSSCertificateDB.cpp:1259
(Diff revision 1)
>    if (isAlreadyShutDown()) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
>    nsNSSCertTrust trust;
> -  if (CERT_DecodeTrustString(trust.GetTrust(), PromiseFlatCString(aTrust).get())
> +  if (CERT_DecodeTrustString(&trust.GetTrust(), PromiseFlatCString(aTrust).get())

Thanks, compiler, for letting us get away with this until now!
Attachment #8889647 - Flags: review?(jjones) → review+
Comment on attachment 8889647 [details]
bug 1382866 - prompt for authentication when changing certificate trust fails

https://reviewboard.mozilla.org/r/160682/#review166880

Looks good.

::: security/manager/ssl/nsNSSCertificateDB.cpp:240
(Diff revision 1)
> +SECStatus
> +ChangeCertTrustWithPossibleAuthentication(const UniqueCERTCertificate& cert,
> +                                          CERTCertTrust& trust, void* ctx)
> +{
> +  // NSS ignores the first argument to CERT_ChangeCertTrust
> +  SECStatus srv = CERT_ChangeCertTrust(nullptr, cert.get(), &trust);

Maybe add an assert that `cert` is not-null?

::: security/manager/ssl/nsNSSCertificateDB.cpp:858
(Diff revision 1)
> -                               nsscert.get(),
> +    case nsIX509Cert::SERVER_CERT:
> -                               trust.GetTrust());
> -  } else if (type == nsIX509Cert::SERVER_CERT) {
> -    // always start with untrusted and move up
> -    trust.SetValidPeer();
> +      trust.SetValidPeer();
> -    trust.AddPeerTrust(trusted & nsIX509CertDB::TRUSTED_SSL, 0, 0);
> +      trust.AddPeerTrust(trusted & nsIX509CertDB::TRUSTED_SSL, 0, 0);

Optional: Would be nice to use `false` instead of `0` here and below.
Attachment #8889647 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8889647 [details]
bug 1382866 - prompt for authentication when changing certificate trust fails

https://reviewboard.mozilla.org/r/160682/#review166006

Thanks!

> Nit... Might want to keep the comment about ignoring user certs?

Sounds good.

> Thanks, compiler, for letting us get away with this until now!

Not quire sure what you mean here - I changed GetTrust to return a reference instead of a pointer so I could make the CERTCertTrust parameter to ChangeCertTrustWithPossibleAuthentication more clearly mandatory. I don't think this particular line was ever incorrect...
Comment on attachment 8889647 [details]
bug 1382866 - prompt for authentication when changing certificate trust fails

https://reviewboard.mozilla.org/r/160682/#review166880

Thanks!

> Maybe add an assert that `cert` is not-null?

Sounds good.

> Optional: Would be nice to use `false` instead of `0` here and below.

Good idea.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac03688d4382
prompt for authentication when changing certificate trust fails r=Cykesiopka,jcj
https://hg.mozilla.org/mozilla-central/rev/ac03688d4382
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.