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)
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•6 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•6 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•6 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•6 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•6 years ago
|
||
bugherder |
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.
Description
•