Closed Bug 1411683 Opened 7 years ago Closed 7 years ago

Add methods to iterate and segment nsNSSCertList objects

Categories

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

Product:
Core
Component:
Security: PSM
Version:
58 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

Details

Attachments

(2 files)

Bug 1411683 - Add foreach and segment utility methods to nsNSSCertList
59 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1411683 - Add "requirements.txt" for pycert.py
59 bytes, text/x-review-board-request
keeler
: review+
Details 
We need to be able to manipulate nsNSSCertList objects a bit for Bug 1409259, so let's implement some methods to help do that.
Comment on attachment 8922026 [details]
Bug 1411683 - Add foreach and segment utility methods to nsNSSCertList

https://reviewboard.mozilla.org/r/193010/#review198282


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: security/manager/ssl/nsNSSCertificate.cpp:1409
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsNSSCertList::ForEachCertificateInChain(ForEachCertOperation aOperation) {

Warning: The const qualified parameter 'aoperation' is copied for each invocation; consider making it a reference [clang-tidy: performance-unnecessary-value-param]

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:18
(Diff revision 1)
> +#include "nsNSSCertificate.h"
> +#include "nsServiceManagerUtils.h"
> +
> +class psm_CertList : public ::testing::Test {
> + protected:
> +  virtual void SetUp() {

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  virtual void SetUp() {
               ^
                       override
Comment on attachment 8922026 [details]
Bug 1411683 - Add foreach and segment utility methods to nsNSSCertList

https://reviewboard.mozilla.org/r/193010/#review198282

Thanks, static analysis bot!
Comment on attachment 8922048 [details]
Bug 1411683 - Add "requirements.txt" for pycert.py

https://reviewboard.mozilla.org/r/193048/#review198312

Sounds good, but unfortunately we need to use older versions for the moment :(

::: security/manager/ssl/tests/unit/requirements.txt:2
(Diff revision 1)
> +lxml
> +pyasn1 <= 0.2.9

We should probably set these to what's in mozilla-central: 0.1.7 for pyasn1 and 0.0.5 for pyasn1_modules (yes, they're quite out of date - I've been meaning to update them but it would require some debugging because recent versions break pycert/pykey/pycms).
(In particular, if we use these versions, there's a bug where if we specify a keyUsage extension of keyEncipherment and keyCertSign it comes out as digitalSignature and dataEncipherment.)
Attachment #8922048 - Flags: review?(dkeeler) → review+
Comment on attachment 8922048 [details]
Bug 1411683 - Add "requirements.txt" for pycert.py

https://reviewboard.mozilla.org/r/193048/#review198312

Ah, gotcha. I'll note that in the commit message, too. Thanks!
Comment on attachment 8922026 [details]
Bug 1411683 - Add foreach and segment utility methods to nsNSSCertList

https://reviewboard.mozilla.org/r/193010/#review198322

Cool. I definitely think this is useful and a good way to decompose operations like how we're handling the Symantec issue, but there's a couple things we need to address first.

::: security/manager/ssl/nsNSSCertificate.h:72
(Diff revision 2)
>              const mozilla::pkix::DERArray& certArray,
>              /*out*/ mozilla::UniqueCERTCertList& certList);
>  
>  } // namespace mozilla
>  
> +typedef const std::function<nsresult(nsCOMPtr<nsIX509Cert>& aCert, bool* aContinue)> ForEachCertOperation;

Let's use references where we can. Also, using a single parameter to mean both "ForEachCertificateInChain is telling the callback that there are more certificates" and "the callback is telling ForEachCertificateInChain to continue" seems like a recipe for confusion down the line.

::: security/manager/ssl/nsNSSCertificate.h:95
(Diff revision 2)
>      const mozilla::UniqueCERTCertList& certList,
>      const nsNSSShutDownPreventionLock& proofOfLock);
>  
> +  // For each certificate in this CertList, run the operation aOperation.
> +  // To end early with NS_OK, set the boolean argument false before returning.
> +  NS_IMETHODIMP ForEachCertificateInChain(ForEachCertOperation& aOperation);

We probably want just 'nsresult' for these.

::: security/manager/ssl/nsNSSCertificate.h:97
(Diff revision 2)
>  
> +  // For each certificate in this CertList, run the operation aOperation.
> +  // To end early with NS_OK, set the boolean argument false before returning.
> +  NS_IMETHODIMP ForEachCertificateInChain(ForEachCertOperation& aOperation);
> +
> +  // Split a certificate chain into the root, intermediates (if any), and end

I mean, this all assumes the chain is in order of root first and end-entity last. That's generally true, but we might just want to note it here (and note that we don't do any kind of verification here).

::: security/manager/ssl/nsNSSCertificate.cpp:1408
(Diff revision 2)
>    }
>  
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP

'nsresult' again

::: security/manager/ssl/nsNSSCertificate.cpp:1409
(Diff revision 2)
>  
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsNSSCertList::ForEachCertificateInChain(ForEachCertOperation& aOperation) {

nit: '{' on its own line

::: security/manager/ssl/nsNSSCertificate.cpp:1445
(Diff revision 2)
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +
> +    // If the called function forces continueLoop true even when there aren't
> +    // more elements, the GetNext iterator will catch it and fail us out safely.

But then we'll return a failing error code, right? Why not just check the bool after the call and exit the loop? (Or, if we separate the bool into its two meanings, we don't have to deal with this situation.)

::: security/manager/ssl/nsNSSCertificate.cpp:1459
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsNSSCertList::SegmentCertificateChain(/* out */ nsCOMPtr<nsIX509Cert>& aRoot,
> +                          /* out */ nsCOMPtr<nsIX509CertList>& aIntermediates,
> +                          /* out */ nsCOMPtr<nsIX509Cert>& aEndEntity) {
> +  if(aRoot || aIntermediates || aEndEntity) {

nit: space after 'if'

::: security/manager/ssl/nsNSSCertificate.cpp:1466
(Diff revision 2)
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  aIntermediates = new nsNSSCertList();
> +
> +  ForEachCertificateInChain(

We should probably check the return value.

::: security/manager/ssl/nsNSSCertificate.cpp:1475
(Diff revision 2)
> +        return NS_ERROR_INVALID_POINTER;
> +      }
> +
> +      if (!aRoot) {
> +        // This is the root
> +        aRoot = aCert.forget();

I think we just want `aRoot = aCert;`

::: security/manager/ssl/nsNSSCertificate.cpp:1478
(Diff revision 2)
> +      if (!aRoot) {
> +        // This is the root
> +        aRoot = aCert.forget();
> +      } else if (!*aContinue) {
> +        // This is the end entity
> +        aEndEntity = aCert.forget();

Same.

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:80
(Diff revision 2)
> +  return NS_OK;
> +}
> +
> +static nsresult
> +AddCertFromFileToList(const char* aBasePath, const char* aFilename,
> +                      /* out */ nsIX509CertList* aCertList) {

Well, we're sort-of modifying the parameter here, so it's not clear it's an out parameter in the traditional sense.

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:93
(Diff revision 2)
> +}
> +
> +static int
> +CountCertsInList(nsCOMPtr<nsIX509CertList>& aCertList) {
> +  int counter = 0;
> +  nsNSSCertList* certList = static_cast<nsNSSCertList*>(aCertList.get());

So... this is why this API troubles me a bit. In theory, unless we declare nsIX509CertList a builtinclass (I think that's the right one, at least?) then a JS component could in theory register itself as the canonical nsIX509CertList implementation, at which point any uses of this api would cause bad things to happen. In reality, since nsIX509CertList is used off the main thread, I think Firefox will crash already if some JS tries this. In any case, from a type safety standpoint, I'd like to find a better solution.

There's two options I see:

1. add a C++-only function to nsIX509CertList that returns the underlying nsNSSCertificate pointer (i.e. the implementation returns "this" and all we have to do is figure out how to express the function in IDL - see https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509Cert.idl#256 and https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509Cert.idl#18 )

2. express the new API in IDL:

SegmentCertificateChain will probably look like:

void segmentCertificateChain(out nsIX509Cert root, out nsIX509CertList intermediates, out nsIX509Cert ee);

For ForEachCertificateInChain we'll need to define a new callback type, which is a bit of a bummer. See https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509CertDB.idl#24 and https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509CertDB.idl#264 for an example.

All that's a bit cumbersome, though, so...

I guess another option would be to use the concrete type nsNSSCertList everywhere in PSM, but that's a fairly invasive change.

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:94
(Diff revision 2)
> +
> +static int
> +CountCertsInList(nsCOMPtr<nsIX509CertList>& aCertList) {
> +  int counter = 0;
> +  nsNSSCertList* certList = static_cast<nsNSSCertList*>(aCertList.get());
> +  certList->ForEachCertificateInChain(

nit: check return value

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:109
(Diff revision 2)
> +  return counter;
> +}
> +
> +TEST_F(psm_CertList, TestInvalidSegmenting)
> +{
> +  nsNSSCertList* certList = new nsNSSCertList();

Let's wrap this in a RefPtr (or maybe nsRefPtr? I'm not sure which one to use) so it doesn't leak.

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:117
(Diff revision 2)
> +  nsCOMPtr<nsIX509CertList> intCerts;
> +  nsCOMPtr<nsIX509Cert> eeCert;
> +  nsresult rv = certList->SegmentCertificateChain(rootCert, intCerts, eeCert);
> +  ASSERT_EQ(rv, NS_ERROR_INVALID_ARG) << "Empty lists can't be segmented";
> +
> +  rv = AddCertFromFileToList("../../../security/manager/ssl/tests/gtest/test_certlist", "ca-1.pem", certList);

Make this long string a constant so all tests can use it?

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:125
(Diff revision 2)
> +  // We should need to clear out the in-vars, but let's make sure behavior is OK
> +  rv = certList->SegmentCertificateChain(rootCert, intCerts, eeCert);
> +  ASSERT_EQ(rv, NS_ERROR_UNEXPECTED) << "Don't permit already-filled-in arguments";
> +
> +  intCerts = nullptr;
> +  rootCert = eeCert = nullptr;

Let's do these operations on separate lines.

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:146
(Diff revision 2)
> +  nsCOMPtr<nsIX509CertList> intCerts;
> +  nsCOMPtr<nsIX509Cert> eeCert;
> +
> +  rv = certList->SegmentCertificateChain(rootCert, intCerts, eeCert);
> +  ASSERT_EQ(rv, NS_OK) << "Should have segmented OK";
> +  ASSERT_TRUE(rootCert) << "Root cert should be filled in";

We should probably check it has the expected subject/issuer or something.

::: security/manager/ssl/tests/gtest/NSSCertListTest.cpp:237
(Diff revision 2)
> +
> +  int counter = 0;
> +  rv = certList->ForEachCertificateInChain(
> +    [&counter] (nsCOMPtr<nsIX509Cert> aCert, bool* aContinue) {
> +      counter++;
> +      *aContinue = true; // Try to keep it going illegally

We should probably also have a "stop early" case, right?

::: security/manager/ssl/tests/gtest/moz.build:12
(Diff revision 2)
>  SOURCES += [
>      'CertDBTest.cpp',
>      'DataStorageTest.cpp',
>      'DeserializeCertTest.cpp',
>      'MD4Test.cpp',
> +    'NSSCertListTest.cpp',

Maybe just CertListTest.cpp?

::: security/manager/ssl/tests/gtest/test_certlist/ca-2.pem.certspec:2
(Diff revision 2)
> +issuer:ca
> +subject:ca-intermediate

In some tests there's a correspondence between the subject name and the filename (so either this would be 'subject:ca-2' or the file would be 'ca-intermediate.pem'), but it's not a big deal.
Attachment #8922026 - Flags: review?(dkeeler) → review-
Comment on attachment 8922026 [details]
Bug 1411683 - Add foreach and segment utility methods to nsNSSCertList

https://reviewboard.mozilla.org/r/193010/#review198322

Thanks for the detailed review. This is bigger, but much cleaner now.

> But then we'll return a failing error code, right? Why not just check the bool after the call and exit the loop? (Or, if we separate the bool into its two meanings, we don't have to deal with this situation.)

Yeah, laziness doesn't pay. I've changed to using two bools, one in, one out: 
`typedef const std::function<nsresult(nsCOMPtr<nsIX509Cert>& aCert, bool aHasMore, /* out */ bool& aContinue)> ForEachCertOperation;`

> We should probably check the return value.

Oops.

> Same.

Interesting. Remind me to ask you how that refcounting works in this case.

> Well, we're sort-of modifying the parameter here, so it's not clear it's an out parameter in the traditional sense.

Thanks; I swear it was, once... too many interruptions.

> So... this is why this API troubles me a bit. In theory, unless we declare nsIX509CertList a builtinclass (I think that's the right one, at least?) then a JS component could in theory register itself as the canonical nsIX509CertList implementation, at which point any uses of this api would cause bad things to happen. In reality, since nsIX509CertList is used off the main thread, I think Firefox will crash already if some JS tries this. In any case, from a type safety standpoint, I'd like to find a better solution.
> 
> There's two options I see:
> 
> 1. add a C++-only function to nsIX509CertList that returns the underlying nsNSSCertificate pointer (i.e. the implementation returns "this" and all we have to do is figure out how to express the function in IDL - see https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509Cert.idl#256 and https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509Cert.idl#18 )
> 
> 2. express the new API in IDL:
> 
> SegmentCertificateChain will probably look like:
> 
> void segmentCertificateChain(out nsIX509Cert root, out nsIX509CertList intermediates, out nsIX509Cert ee);
> 
> For ForEachCertificateInChain we'll need to define a new callback type, which is a bit of a bummer. See https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509CertDB.idl#24 and https://dxr.mozilla.org/mozilla-central/rev/a124f4901430f6db74cfc7fe3b07957a1c691b40/security/manager/ssl/nsIX509CertDB.idl#264 for an example.
> 
> All that's a bit cumbersome, though, so...
> 
> I guess another option would be to use the concrete type nsNSSCertList everywhere in PSM, but that's a fairly invasive change.

Great advice. Thanks. I've gone with Option 1.
Comment on attachment 8922026 [details]
Bug 1411683 - Add foreach and segment utility methods to nsNSSCertList

https://reviewboard.mozilla.org/r/193010/#review198696

LGTM! Just a couple of comments.

::: security/manager/ssl/nsIX509CertList.idl:20
(Diff revision 3)
> +%{C++
> +class nsNSSCertList;
> +%}
> +[ptr] native nsNSSCertListPtr(nsNSSCertList);
> +
>  [scriptable, uuid(ae74cda5-cd2f-473f-96f5-f0b7fff62c68)]

We should probably still add `builtinclass` here.

::: security/manager/ssl/nsIX509CertList.idl:47
(Diff revision 3)
>     */
>    [must_use]
>    boolean equals(in nsIX509CertList other);
> +
> +  /**
> +   * Retrieves the NSS certificate object wrapped by this interface

Well, more accurately it's the PSM helper class that wraps the NSS certificate list.

::: security/manager/ssl/nsNSSCertificate.h:95
(Diff revision 3)
>    static mozilla::UniqueCERTCertList DupCertList(
>      const mozilla::UniqueCERTCertList& certList,
>      const nsNSSShutDownPreventionLock& proofOfLock);
>  
> +  // For each certificate in this CertList, run the operation aOperation.
> +  // To end early with NS_OK, set the `aContinue` argument false before

I guess we could add that we'll also terminate early with an error return value if aOperation returns an error return value.

::: security/manager/ssl/tests/gtest/CertListTest.cpp:292
(Diff revision 3)
> +
> +  int counter = 0;
> +  rv = certList->ForEachCertificateInChain(
> +    [&counter] (nsCOMPtr<nsIX509Cert> aCert, bool aHasMore, bool& aContinue) {
> +      counter++;
> +      aHasMore = true; // Try to keep it going

aHasMore isn't relevant here, right?

::: security/manager/ssl/tests/gtest/test_certlist/moz.build:11
(Diff revision 3)
> +
> +# Temporarily disabled. See bug 1256495.
> +#test_certificates = (
> +#    'ca-1.pem',
> +#    'ca-2.pem',
> +#    'ca-3.pem',

These just need to use the new names now.
Attachment #8922026 - Flags: review?(dkeeler) → review+
Comment on attachment 8922026 [details]
Bug 1411683 - Add foreach and segment utility methods to nsNSSCertList

https://reviewboard.mozilla.org/r/193010/#review198696

> aHasMore isn't relevant here, right?

True.

> These just need to use the new names now.

Oops! Thanks.
Most recent try run is here, and it looks good!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff227f92314f7ad5c993ffffae98b6bcc1688fec&selectedJob=140241687

This took some iterations to get right, but thanks for all the reviews and advice. Let's go!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/21a17ab8b0fc
Add "requirements.txt" for pycert.py r=keeler
https://hg.mozilla.org/integration/autoland/rev/9d579c7e46b9
Add foreach and segment utility methods to nsNSSCertList r=keeler
Keywords: checkin-needed
Backed out for build bustage in security/manager/ssl/tests/gtest/CertListTest.cpp:

https://hg.mozilla.org/integration/autoland/rev/af98112b1831843fbe5e127e2e755201da136cdd

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9d579c7e46b983e483575ee5eb9bd3e1252d3aac&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=140316273&repo=autoland

[task 2017-10-27T21:14:07.936Z] 21:14:07     INFO -  /builds/worker/workspace/build/src/security/manager/ssl/tests/gtest/CertListTest.cpp: In member function 'virtual void psm_CertList_TestValidSegmenting_Test::TestBody()':
[task 2017-10-27T21:14:07.936Z] 21:14:07     INFO -  /builds/worker/workspace/build/src/security/manager/ssl/tests/gtest/CertListTest.cpp:210:16: error: aggregate 'nsAutoString rootCn' has incomplete type and cannot be defined
[task 2017-10-27T21:14:07.936Z] 21:14:07     INFO -     nsAutoString rootCn;
[task 2017-10-27T21:14:07.936Z] 21:14:07     INFO -                  ^~~~~~
Flags: needinfo?(jjones)
Huh. Sorry about that.

Added another #include and checked again with a try run[1] which shows OK for the gtest from linux64-opt, so I guess let's try again!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5f2013f52f1add8ed5c2a00d89c56f28cd01534

*crosses fingers*
Flags: needinfo?(jjones)
Keywords: checkin-needed
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/17f0b07c7f4a
Add "requirements.txt" for pycert.py r=keeler
https://hg.mozilla.org/integration/autoland/rev/9daa1c97952b
Add foreach and segment utility methods to nsNSSCertList r=keeler
Keywords: checkin-needed
Blocks: 1412994
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: