The default bug view has changed. See this FAQ.

don't expose the NSS certificate nickname API in PSM interfaces

RESOLVED FIXED in Firefox 53

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: mkaply, Assigned: keeler)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla53
addon-compat
Points:
---

Firefox Tracking Flags

(firefox24-, firefox53 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
The API for addCertFromBase64 takes a parameter aName - name of the cert for display purposes.

If you look at the code:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#1559

You'll see it is completely ignored.

The nickname is generated from the cert.
(Reporter)

Comment 1

4 years ago
Created attachment 739180 [details] [diff] [review]
Use nickname if it is there
Attachment #739180 - Flags: review?(bsmith)
(Reporter)

Updated

4 years ago
Assignee: nobody → mozilla
(Reporter)

Comment 2

4 years ago
bsmith: any chance you could take a look at this?
Comment on attachment 739180 [details] [diff] [review]
Use nickname if it is there

Review of attachment 739180 [details] [diff] [review]:
-----------------------------------------------------------------

A couple of things:
1. You'll probably need 8 lines of context in your patch before Brian will review it
2. We're working on a lot of other things that impact security in ways this patch doesn't, so I imagine this change isn't seen as a high priority.
3. I wouldn't be surprised if Brian has plans to just remove the name parameter from the API.
4. Looks good otherwise
Attachment #739180 - Flags: feedback+
(Reporter)

Comment 4

4 years ago
> I wouldn't be surprised if Brian has plans to just remove the name parameter from the API.

I hope not. It would actually be really useful because then you could easily delete a certificate that you added. Right now it's a pain in the butt.

In a managed environment with multiple users logging on to the same machine, there are use cases for adding a cert at startup and removing it at shutdown.
(Reporter)

Comment 5

4 years ago
Created attachment 751216 [details] [diff] [review]
New patch with more context
Attachment #751216 - Flags: review?
(Reporter)

Updated

4 years ago
Attachment #739180 - Flags: review?(bsmith)
(Reporter)

Updated

4 years ago
Attachment #751216 - Flags: review? → review?(bsmith)
(Reporter)

Comment 6

4 years ago
> 2. We're working on a lot of other things that impact security in ways this patch doesn't, so I imagine this change isn't seen as a high priority.

The more I think about this, the more I am bothered by this comment. What you are saying is that your work is more important than outside contributors to the project.

Mozilla is open source. As such, I would expect that everyone at Mozilla would respect the contributions of anyone.

This patch will take all of 1 minute to review.

As a sidenote, I'd like this for the next ESR.
tracking-firefox24: --- → ?
You're right - that wasn't very helpful. I'll see if I can get Brian's attention on this, or if he'll delegate the review to me. Another option would be to ask another PSM peer for a review (https://wiki.mozilla.org/Modules/All#Security_-_Mozilla_PSM_Glue ). Honza would probably be your best bet. Normally a PSM patch requires review from two peers or review from the module owner, but in this case I imagine we can go with one reviewer.
Additionally (and I should have mentioned this earlier), it would be great to have an xpcshell test for this (obviously we don't have one now). Let me know if you need any pointers on that.
(Reporter)

Comment 8

4 years ago
> Additionally (and I should have mentioned this earlier), it would be great to have an xpcshell test for this (obviously we don't have one now). Let me know if you need any pointers on that.

Any thoughts on what to use for a sample CA?

I see various certs in security/nss/cmd/samples/, but there is no documentation around them as to what they are.

I'm assuming the test will basically be - addcert, then call findcertbynickname and make sure I got a cert back.

Also, I assume this would go in security/manager/ssl/tests/unit, since that is the only directory that has xpcshell tests?
(In reply to Michael Kaply (mkaply) from comment #8)
> Any thoughts on what to use for a sample CA?

I think it would be simplest to generate a bogus sample cert using certutil or openssl, get its base64 representation and store that as a string in the test itself.

> I see various certs in security/nss/cmd/samples/, but there is no
> documentation around them as to what they are.

Yeah, I wouldn't rely on those - we probably want to keep the test distinct and self-contained.

> I'm assuming the test will basically be - addcert, then call
> findcertbynickname and make sure I got a cert back.

Sounds good. I would also recommend checking that you got the cert you were expecting.

> Also, I assume this would go in security/manager/ssl/tests/unit, since that
> is the only directory that has xpcshell tests?

Perfect.
(In reply to Michael Kaply (mkaply) from comment #4)
> > I wouldn't be surprised if Brian has plans to just remove the name parameter from the API.
> 
> I hope not. It would actually be really useful because then you could easily
> delete a certificate that you added. Right now it's a pain in the butt.
> 
> In a managed environment with multiple users logging on to the same machine,
> there are use cases for adding a cert at startup and removing it at shutdown.

You cannot remove a certificate you added at shutdown because shutdown code might never run. And, code that does disk I/O, like this, generally should not run at shutdown. Generally, the approach of modifying a permanent database and then un-modifying it at shutdown is poor for this reason--especially when that database a security-critical database like this. BTW, at least rsleevi agrees with me on this, based on the conversation on #nss I had with him where I tried to find an alternative solution for you.

My concern with this patch is that, for now, it will work. But, if/when we move to alternate root CA trust mechanisms (not trust bits in the NSS database), then code that depends on this will break. So, I don't want to give anybody the false impression that this mechanism will continue to work for any length of time. In other words, the nickname mechanism should be considered an implementation detail, not a feature. This is why I think we should WONTFIX this and file a bug for removing the parameter.

If your goal is to have "per-session" certificate trust, then why not just implement a custom cert override service that returns "override cert error" for untrusted certs for the issuer of the certificate that you want to trust? That would solve your problem and also prevent the fundamental problem with this approach.

(In reply to Michael Kaply (mkaply) from comment #6)
> The more I think about this, the more I am bothered by this comment. What
> you are saying is that your work is more important than outside contributors
> to the project.

I would say this is less the issue than, simply, I think this is a bad idea. I agree it would be better for us to be more responsive.

FWIW, pretty much all of the certificate-related APIs in PSM need to be revamped. There will be a lot of growing pains in the next few months while things get sorted.
Comment on attachment 751216 [details] [diff] [review]
New patch with more context

I think this code seems to do what it purports to do, but I don't think it is a good idea. Please r?bsmith again if you still think it should go in.

If we decide to do this, then we should have a test, so that we know right away when (not if) we break this in the future, so we can notify addon authors that are relying on this.
Attachment #751216 - Flags: review?(bsmith)
(Reporter)

Comment 12

4 years ago
> I think this code seems to do what it purports to do, but I don't think it is a good idea.

Can you explain why?

Your earlier explanation was about the shutdown problem, and I'll accept that. But why is it wrong for this API to use a nickname? Other NSS APIs take nicknames for certs. What exactly is the downside here?
(In reply to Michael Kaply (mkaply) from comment #12)
> Your earlier explanation was about the shutdown problem, and I'll accept
> that. But why is it wrong for this API to use a nickname? Other NSS APIs
> take nicknames for certs. What exactly is the downside here?

"In other words, the nickname mechanism should be considered an implementation detail, not a feature. This is why I think we should WONTFIX this and file a bug for removing the parameter."

Basically, I am still open to adding APIs if they are needed and we realistically expect to be able to support them long-term. But, I don't think we can realistically support the nickname mechanism long-term. You are right that some NSS APIs use nicknames, but I'm not sure we'll be using NSS's certificate trust APIs/databases long-term. If we can avoid adding APIs with NSS-isms, I'd like to do that.
(Reporter)

Comment 14

4 years ago
Why do you think nicknames are NSS specific? Other platforms support nicknames for certificates simply because the regular certificate name is quite painful to type every time.

Honestly, I don't care one way or another. I saw a bug. I reported it.

It's your component. Do what you want with it.

Comment 15

4 years ago
> I saw a bug. I reported it.

You did more. You contributed a patch. In true open-source spirit.
The reactions are just sad.
This in not an ESR specific regression or a recent feature regression,specific crash or a sec-crit bug hence does not fall in our tracking criteria.
tracking-firefox24: ? → -
(Assignee)

Comment 17

11 months ago
We should probably just remove that parameter. If referring to certificates by nickname is an important use-case, we should improve the APIs so they're actually usable for that purpose (for instance, if you did add a certificate with a nickname, but there happened to be a preexisting certificate by that nickname, it's not clear what nsIX509CertDB.findCertByNickname will return).
Whiteboard: [psm-cleanup]
Supporting the NSS certificate nickname API is hindering architectural improvements to PSM. To address this, I intend to have PSM not expose anything that can modify or make use of the nickname API. If my understanding of the use-cases for this API is correct, consumers should be able to use either the dbKey API (nsIX509Cert.dbKey / nsIX509CertDB.findCertByDBKey) for a way to uniquely identify a certificate or the (to be added) nsIX509Cert.displayName API for a friendly display name. Please let me know if I've overlooked a use-case.
Assignee: mozilla → dkeeler
Priority: -- → P1
Summary: addCertFromBase64 ignores the passed in nickname → don't expose the NSS certificate nickname API in PSM interfaces
Whiteboard: [psm-cleanup] → [psm-assigned]
(Assignee)

Updated

4 months ago
Attachment #739180 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #751216 - Attachment is obsolete: true
(Reporter)

Comment 19

4 months ago
Isn't displayName effectively nickname?
It can be, but it isn't guaranteed to be. For instance, you could have two certificates that return the same value of displayName. nsIX509CertDB.findCertByNickname will only return one of those certificates, and it might not be the expected one. The aim of the changes in this bug is in part to reshape the nsIX509Cert/nsIX509CertDB APIs so that there isn't ambiguous behavior of this kind.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
See Also: → bug 1319185

Comment 25

4 months ago
mozreview-review
Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review94950

::: security/manager/ssl/nsNSSCertificate.cpp:411
(Diff revision 1)
> +    nsAutoCString fullNickname(mCert->nickname);
> +    nsCString::const_iterator start;
> +    fullNickname.BeginReading(start);
> +    nsCString::const_iterator matchEnd;
> +    fullNickname.EndReading(matchEnd);
> +    nsCString::const_iterator delimiter;

Unused variable
Attachment #8812893 - Flags: review?(jjones) → review+

Comment 26

4 months ago
mozreview-review
Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review94952

::: security/manager/ssl/tests/unit/test_nsIX509Cert_utf8.js
(Diff revision 1)
>      "v9jIf3twECyxx/RVzONVcob7nPePESHiKoG4FbtcuUh0wSHvCmTwRIQqPDCIuHcF" +
>      "StSt3Jr9iXcbXEhe4mSccOZ8N+r7Rv3ncKcevlRl7uFfDKDTyd43SZeRS/7J8KRf" +
>      "hD/h2nawrCFwc5gJW10aLJGFL/mcS7ViAIT9HCVk23j4TuBjsVmnZ0VKxB5edux+" +
>      "LIEqtU428UVHZWU/I5ngLw==");
>  
> -  equal(cert.nickname, "(no nickname)",

No test for the displayName?
Attachment #8812894 - Flags: review?(jjones) → review+
(Assignee)

Updated

4 months ago
Keywords: addon-compat

Comment 27

4 months ago
mozreview-review
Comment on attachment 8812895 [details]
bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

https://reviewboard.mozilla.org/r/94450/#review94958
Attachment #8812895 - Flags: review?(jjones) → review+

Comment 28

4 months ago
mozreview-review
Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review96778

Looks good!

::: security/manager/pki/resources/content/certViewer.js:52
(Diff revision 1)
>    var child = document.getElementById(node);
>    var currCert;
>    var displayVal;
>    for (let i = chain.length - 1; i >= 0; i--) {
>      currCert = chain.queryElementAt(i, nsIX509Cert);
> -    if (currCert.commonName) {
> +    displayVal = currCert.displayName;

Nit: Let's make this `let displayValue = ...` instead.
Same for `currCert` above.

::: security/manager/ssl/nsNSSCertHelper.cpp:1976
(Diff revision 1)
> -  nsresult rv = GetWindowTitle(title);
> +  nsresult rv = GetDisplayName(displayName);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
>  
> -  sequence->SetDisplayName(title);
> +  sequence->SetDisplayName(displayName);

Wouldn't hurt to add a retval check while we're here, but not a big deal.

::: security/manager/ssl/nsNSSCertificate.cpp:378
(Diff revision 1)
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> -  aWindowTitle.Truncate();
> +  aDisplayName.Truncate();
>  
> +  MOZ_ASSERT(mCert, "mCert should not be null in GetDisplayName");

This should have a corresponding `#include "mozilla/Assertions.h"`.

::: security/manager/ssl/nsNSSCertificate.cpp:407
(Diff revision 1)
> +    nsCString::const_iterator start;
> +    fullNickname.BeginReading(start);
> +    nsCString::const_iterator matchEnd;
> +    fullNickname.EndReading(matchEnd);
> +    nsCString::const_iterator delimiter;
> +    if (FindInReadable(NS_LITERAL_CSTRING(":"), start, matchEnd)) {
> +      // matchEnd now points to just after the ':'.
> +      nsCString::const_iterator end;
> +      fullNickname.EndReading(end);
> +      builtInRootNickname = Substring(matchEnd, end);
> +    }

I think we can replace this code with something simpler actually:
```cpp
int32_t index = fullNickname.Find(":");
if (index != kNotFound) {
  // Substring() safely handles an out of range starting position.
  builtInRootNickname = Substring(fullNickname,
                                  AssertedCast<uint32_t>(index + 1));
}
```

::: security/manager/ssl/tests/unit/test_nsIX509Cert_utf8.js:60
(Diff revision 1)
>          "Actual and expected commonName should match");
>    equal(cert.organization, "",
>          "Actual and expected organization should match");
>    equal(cert.organizationalUnit, "",
>          "Actual and expected organizationalUnit should match");
> -  equal(cert.windowTitle, "Luděk Rašek",
> +  equal(cert.displayName, "Luděk Rašek",

We should probably add more test cases for `displayName` somewhere.
Follow-up part or bug I guess.
Attachment #8812893 - Flags: review?(cykesiopka.bmo) → review+

Comment 29

4 months ago
mozreview-review
Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review97138

LGTM.

::: security/manager/ssl/tests/unit/test_local_cert.js:52
(Diff revision 1)
>  add_task(function* () {
>    // No master password, so no prompt required here
>    ok(!certService.loginPromptRequired);
>  
>    let certA = yield getOrCreateCert(gNickname);
> -  equal(certA.nickname, gNickname);
> +  equal(certA.displayName, gNickname);

Maybe add a comment here that nsIX509Cert no longer provides a way to get at the nicknames of certs, which is why `displayName` is being used instead?
Attachment #8812894 - Flags: review?(cykesiopka.bmo) → review+

Comment 30

4 months ago
mozreview-review
Comment on attachment 8812895 [details]
bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

https://reviewboard.mozilla.org/r/94450/#review97140

Looks good, but the following files still call `addCert` or `addCertFromBase64` with three arguments, which would be nice to fix:
```
netwerk/test/unit/test_altsvc.js
netwerk/test/unit/test_http2.js
netwerk/test/unit/test_immutable.js
security/manager/ssl/tests/mochitest/browser/head.js
security/manager/ssl/tests/unit/test_cert_signatures.js
```

::: security/manager/ssl/nsIX509CertDB.idl:345
(Diff revision 1)
>     *                indicating SSL, Email, and Obj signing trust
> -   * @param aName name of the cert for display purposes.
> +   * @return nsIX509Cert the resulting certificate
> -   *              TODO(bug 857627): aName is currently ignored. It should either
> -   *              not be ignored, or be removed.
>     */
> -  void addCert(in ACString certDER, in ACString aTrust, in AUTF8String aName);
> +  nsIX509Cert addCert(in ACString certDER, in ACString aTrust);

Nit: Let's rename `aTrust` to `trust` instead while we're here.
Same below.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1337
(Diff revision 1)
>  NS_IMETHODIMP
>  nsNSSCertificateDB::AddCertFromBase64(const nsACString& aBase64,
>                                        const nsACString& aTrust,
> -                                      const nsACString& /*aName*/)
> +                                      nsIX509Cert** addedCertificate)
>  {
> +  MOZ_ASSERT(addedCertificate);

Needs a corresponding `#include "mozilla/Assertions.h"`.

::: security/manager/ssl/tests/unit/test_add_preexisting_cert.js:41
(Diff revision 1)
> -  // certificate already exists.
> -  let int_cert = certDB.findCertByNickname("int-limited-depth");
>    notEqual(int_cert, null, "Intermediate cert should be in the cert DB");
>    let base64_cert = btoa(getDERString(int_cert));
> -  certDB.addCertFromBase64(base64_cert, "p,p,p", "ignored_argument");
> +  let returnedEE = certDB.addCertFromBase64(base64_cert, "p,p,p",
> +                                            "ignored_argument");

We can get rid of this argument now.

::: security/manager/ssl/tests/unit/test_cert_trust.js:186
(Diff revision 1)
> +    'ca',
> +    'int',
> +    'ee',

Nit: Let's use double quotes instead here and below.
Attachment #8812895 - Flags: review?(cykesiopka.bmo) → review+

Comment 31

4 months ago
mozreview-review
Comment on attachment 8812896 [details]
bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

https://reviewboard.mozilla.org/r/94452/#review97142

Ripping out code -> :)

::: security/manager/ssl/LocalCertService.cpp:30
(Diff revision 1)
> +// Given a name, searches the internal certificate/key database for a
> +// self-signed certificate with subject and issuer distinguished name equal to
> +// "CN={name}". This assumes that the user has already authenticated to the
> +// internal DB if necessary.
> +static nsresult
> +FindLocalCertByName(const nsACString& aName, UniqueCERTCertificate& result)

Nit: We should probably be consistent here with use of the `a` prefix.
Attachment #8812896 - Flags: review?(cykesiopka.bmo) → review+

Comment 32

4 months ago
mozreview-review
Comment on attachment 8812896 [details]
bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

https://reviewboard.mozilla.org/r/94452/#review97148

LGTM.
Attachment #8812896 - Flags: review?(jjones) → review+
(Assignee)

Updated

4 months ago
Blocks: 1321608
(Assignee)

Comment 33

4 months ago
mozreview-review-reply
Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review96778

Thanks for the reviews!

> Wouldn't hurt to add a retval check while we're here, but not a big deal.

Sure.

> This should have a corresponding `#include "mozilla/Assertions.h"`.

Added.

> I think we can replace this code with something simpler actually:
> ```cpp
> int32_t index = fullNickname.Find(":");
> if (index != kNotFound) {
>   // Substring() safely handles an out of range starting position.
>   builtInRootNickname = Substring(fullNickname,
>                                   AssertedCast<uint32_t>(index + 1));
> }
> ```

Good call - that's much simpler.

> We should probably add more test cases for `displayName` somewhere.
> Follow-up part or bug I guess.

Good idea. I think at this point I'd prefer this as a follow-up bug (filed as bug 1321608).
(Assignee)

Comment 34

4 months ago
mozreview-review-reply
Comment on attachment 8812893 [details]
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName)

https://reviewboard.mozilla.org/r/94446/#review94950

> Unused variable

Turns out there was a much easier way of doing this, anyway.
(Assignee)

Comment 35

4 months ago
mozreview-review-reply
Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review94952

> No test for the displayName?

It's on line 58 :)
(Assignee)

Comment 36

4 months ago
mozreview-review-reply
Comment on attachment 8812894 [details]
bug 857627 - 2/4: remove nsIX509Cert.nickname

https://reviewboard.mozilla.org/r/94448/#review97138

> Maybe add a comment here that nsIX509Cert no longer provides a way to get at the nicknames of certs, which is why `displayName` is being used instead?

Sounds good - I added an explainer of why it works and what's going on behind the scenes.
(Assignee)

Comment 37

4 months ago
mozreview-review-reply
Comment on attachment 8812895 [details]
bug 857627 - 3/4: have nsIX509CertDB.addCert* functions return the added certificate

https://reviewboard.mozilla.org/r/94450/#review97140

Whoops - good catch.

> Nit: Let's rename `aTrust` to `trust` instead while we're here.
> Same below.

Sounds good. I also updated the comments.

> Needs a corresponding `#include "mozilla/Assertions.h"`.

Added.

> We can get rid of this argument now.

Good catch.

> Nit: Let's use double quotes instead here and below.

Fixed (I actually updated all single-quotes to double-quotes in that file).
(Assignee)

Comment 38

4 months ago
mozreview-review-reply
Comment on attachment 8812896 [details]
bug 857627 - 4/4: remove nickname-related APIs from nsIX509CertDB

https://reviewboard.mozilla.org/r/94452/#review97142

:D

> Nit: We should probably be consistent here with use of the `a` prefix.

Good call. I also annotated it as /*out*/.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks for all the reviews!
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=372593f6c097f93b1605b7be3d32601bba22b8e8

Comment 44

4 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d20e7d8bb510
1/4: improve nsIX509Cert.windowTitle (and rename to displayName) r=Cykesiopka,jcj
https://hg.mozilla.org/integration/autoland/rev/16f6abade0e7
2/4: remove nsIX509Cert.nickname r=Cykesiopka,jcj
https://hg.mozilla.org/integration/autoland/rev/eda8fdc7c21a
3/4: have nsIX509CertDB.addCert* functions return the added certificate r=Cykesiopka,jcj
https://hg.mozilla.org/integration/autoland/rev/0932c3f7208b
4/4: remove nickname-related APIs from nsIX509CertDB r=Cykesiopka,jcj

Comment 45

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d20e7d8bb510
https://hg.mozilla.org/mozilla-central/rev/16f6abade0e7
https://hg.mozilla.org/mozilla-central/rev/eda8fdc7c21a
https://hg.mozilla.org/mozilla-central/rev/0932c3f7208b
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.