Closed
Bug 1382866
Opened 8 years ago
Closed 8 years ago
CERT_ChangeCertTrust can fail if the user hasn't logged in to softoken
Categories
(Core :: Security: PSM, enhancement, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: keeler, Assigned: keeler)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(1 file)
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 hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
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 3•8 years ago
|
||
mozreview-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+
![]() |
Assignee | |
Comment 4•8 years ago
|
||
mozreview-review-reply |
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...
![]() |
Assignee | |
Comment 5•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac03688d4382
prompt for authentication when changing certificate trust fails r=Cykesiopka,jcj
Comment 8•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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.
Description
•