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

Status: RESOLVED FIXED

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

Description

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

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 1

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

Attachment: bug 1382866 - prompt for authentication when changing certificate trust fails

Review commit: https://reviewboard.mozilla.org/r/160682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160682/

Comment 2

[J.C. Jones [:jcj] (he/they)]

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!

Comment 3

[:Cykesiopka]

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.

Comment 4

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

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 5

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

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.

Comment 6

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

Comment on attachment 8889647 [details]
bug 1382866 - prompt for authentication when changing certificate trust fails

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160682/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac03688d4382
prompt for authentication when changing certificate trust fails r=Cykesiopka,jcj

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac03688d4382