Open Bug 1330968 Opened 7 years ago Updated 2 years ago

Make OneCRL name comparisons encoding agnostic

Categories

(Core :: Security: PSM, defect, P3)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

People

(Reporter: mgoodwin, Unassigned)

Details

(Keywords: stale-bug, Whiteboard: [ca-onecrl][psm-assigned])

Attachments

(1 file)

Bug 1330968 - Make OneCRL name comparisons encoding agnostic (alternative approach)
59 bytes, text/x-review-board-request
Details 
Apparently CAs occasionally use different encodings for cert-issuer and crl-issuer values. OneCRL should take this into account.
It would probably be best to somehow make use of the preexisting mozilla::pkix code that behaves like this: https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/security/pkix/lib/pkixnames.cpp#1417
Priority: -- → P1
Whiteboard: [psm-assigned]
Comment on attachment 8827848 [details]
Bug 1330968 - Make OneCRL name comparisons encoding agnostic (alternative approach)

https://reviewboard.mozilla.org/r/105442/#review106444

This looks good, but I think we should implement the bulk of the logic in terms of preexisting code in pkixnames. What do you think?

::: security/manager/ssl/CertBlocklist.cpp:183
(Diff revision 1)
> +      // implementations are permitted to use X.500 comparison rules (e.g. see
> +      // RFC 2459 4.1.2.4). Some implementations have done this and, as a
> +      // result, there exist CRLs for which the CRL issuer and the cert issuer
> +      // differ only in encodings. Because of this, we must map between
> +      // UTF8String and PrintableString for the purposes of comparison.
> +      bool avasMatch =

A lot of this code is very similar to MatchPresentedDirectoryNameWithConstraint. The downside of reproducing it here is that any updates that affect the logic of both functions will have to be done twice (e.g. bug 1155767). What do you think about implementing MatchPresentedDirectoryNameWithConstraint in terms of another function MatchDirectoryNamePrefix (or something) that takes two Inputs directoryName and directoryNamePrefix that:
* ensures that the RDNs of directoryNamePrefix are a prefix of the RDNs of directoryName and
* counts the number of RDNs in each
(using the PrintableString/UTF8String equivalence rule we have)
Then, given the number of RDNs in each, MatchPresentedDirectoryNameWithConstraint can make the right decision (e.g. if the constraint is excludedSubtrees and the RDN count of either DN is nonzero, it will return Result::ERROR_CERT_NOT_IN_NAME_SPACE).
Then, CertBlocklistItem::operator== can reuse that function and ensure that the RDN counts are equal.

I realize that's a bit more work, but I think it's probably best in the long run.

::: security/manager/ssl/CertBlocklist.cpp:212
(Diff revision 1)
>    if (aItem.mItemMechanism != mItemMechanism) {
>      return false;
>    }
> -  if (aItem.mDNLength != mDNLength ||
> -      aItem.mOtherLength != mOtherLength) {
> +
> +  // if the non-name data do not match, the items are not equal
> +  // TODO: We should probably do a more sophisticated check for serial numbers

Is there a bug on this? What is the concern? (is this the leading zero issue?)

::: security/manager/ssl/tests/unit/test_cert_blocklist.js:357
(Diff revision 1)
>    });
>  
> +  add_test(function() {
> +    // check that RDNs with different encodings are treated as equal by the
> +    // CertBlocklist
> +    let issuer1 = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKEwlPcGVuVHJ1c3QxHTAbBgNVBAMTFE9wZW5UcnVzdCBSb290IENBIEcx";

I would add a few more testscases: 
1. where the PrintableString/UTF8String are switched (i.e. issuer2 is revoked and we check that issuer1 is considered revoked)
2. where a string type that isn't PrintableString or UTF8String is revoked and we check that a PrintableString or UTF8String isn't considered revoked and
3. where a PrintableString or UTF8String is revoked and we check to see that a string type that isn't one of those isn't considered revoked
As well as two where one DN consists of a subset of the RDNs of the other.
Attachment #8827848 - Flags: review?(dkeeler)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> This looks good, but I think we should implement the bulk of the logic in
> terms of preexisting code in pkixnames. What do you think?

That makes perfect sense. I shall do this.
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Keywords: stale-bug
Comment on attachment 8827848 [details]
Bug 1330968 - Make OneCRL name comparisons encoding agnostic (alternative approach)

https://reviewboard.mozilla.org/r/105442/#review194124

This looks like the right thing to do. I think all you should have to do to be able to call this from CertBlocklist is to declare the function in security/pkix/include/pkix.h.

::: security/manager/ssl/CertBlocklist.cpp:109
(Diff revision 2)
>      return false;
>    }
> -  if (aItem.mDNLength != mDNLength ||
> -      aItem.mOtherLength != mOtherLength) {
> +
> +  // if the non-name data do not match, the items are not equal
> +  // TODO: We should probably do a more sophisticated check for serial numbers
> +  if (aItem.mOtherLength != mOtherLength ||

What's the concern here? For serial numbers, I'm pretty sure the values we're comparing are just the bytes in the DER encoding (minus tag and length), so a byte comparison should be fine.

::: security/manager/ssl/CertBlocklist.cpp:114
(Diff revision 2)
> +  if (aItem.mOtherLength != mOtherLength ||
> +      memcmp(aItem.mOtherData, mOtherData, mOtherLength) != 0) {
> +    return false;
> +  }
> +
> +  // if the name data are identical, the items match

This is minor, but I think we could omit this memcmp - I don't think it'll be much faster than just calling MatchDirectoryPrefix once. A reasonable optimization would be to bail if `aItem.mDNLength != mDNLength`, though.

::: security/manager/ssl/CertBlocklist.cpp:133
(Diff revision 2)
> +  if (issuerThat.Init(aItem.mDNData, aItem.mDNLength) != Success) {
> +    return false;
> +  }
> +
> +  bool match;
> +  int issuerThisLength, issuerThatLength;

A couple nits: s/int/size_t/, declare these on separate lines, and also maybe `issuerThisDNCount`/`issuerThatDNCount` or something?

::: security/manager/ssl/CertBlocklist.cpp:143
(Diff revision 2)
> +    // it's reasonable to assume the this record parses correctly and therefore
> +    // a failure is a mismatch.
> +    return false;
> +  }
> +
> +  if (match && issuerThisLength == issuerThatLength) {

nit: we could just simplify this to `return match && issuerThisLength == issuerThatLength;`

::: security/manager/ssl/moz.build:168
(Diff revision 2)
>  LOCAL_INCLUDES += [
>      '/dom/base',
>      '/dom/crypto',
>      '/security/certverifier',
>      '/security/pkix/include',
> +    '/security/pkix/lib',

This might not be necessary? Not sure...

::: security/manager/ssl/tests/unit/test_cert_blocklist.js:429
(Diff revision 2)
>  
> +  add_test(function() {
> +    // check that RDNs with different encodings are treated as equal by the
> +    // CertBlocklist - first revoke PrintableString, check UTF8String
> +    let issuer1PrintableString = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKEwlPcGVuVHJ1c3QxHTAbBgNVBAMTFE9wZW5UcnVzdCBSb290IENBIEcx";
> +    let issuer2UTF8String = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKDAlPcGVuVHJ1c3QxHTAbBgNVBAMMFE9wZW5UcnVzdCBSb290IENBIEcx";

I was expecting this to be called `issuer1UTF8String` - am I missing something?

::: security/pkix/lib/pkixnames.cpp:1308
(Diff revision 2)
>      return Success;
>    });
>  }
>  
> +Result
> +MatchDirectoryNamePrefix(Input directoryName,

This might be a bit easier to review if it came after the implementation for MatchPresentedDirectoryNameWithConstraint (hopefully the diff will be a bit more concise). This will require forward-declaring this function, but since we want to export it, we'll have to do that anyway (in security/pkix/include/pkix/pkix.h, like CheckCertHostname). You might have to close and open again the anonymous namespace, but oh well (I suppose to get around that you could do this in two commits: one that adds the implementation with minimal code moving around and a second that moves it to the right place).

::: security/pkix/lib/pkixnames.cpp:1310
(Diff revision 2)
>  }
>  
> +Result
> +MatchDirectoryNamePrefix(Input directoryName,
> +                         Input directoryNamePrefix,
> +                         /*out*/ uint& directoryNameCount,

Let's use `size_t`

::: security/pkix/lib/pkixnames.cpp:1337
(Diff revision 2)
> +    // a prefix of the presented RDNs.
> +    Reader directoryNameRDN;
> +    if (prefixRDNs.AtEnd()) {
> +      while (!directoryNameRDNs.AtEnd()) {
> +        rv = der::ExpectTagAndGetValue(directoryNameRDNs, der::SET, directoryNameRDN);
> +        directoryNameCount ++;

nit: no space before ++

::: security/pkix/lib/pkixnames.cpp:1339
(Diff revision 2)
> +    if (prefixRDNs.AtEnd()) {
> +      while (!directoryNameRDNs.AtEnd()) {
> +        rv = der::ExpectTagAndGetValue(directoryNameRDNs, der::SET, directoryNameRDN);
> +        directoryNameCount ++;
> +        if (rv != Success) {
> +          return Success;

`return rv;`?

::: security/pkix/lib/pkixnames.cpp:1467
(Diff revision 2)
>                                            /*out*/ bool& matches)
>  {
> -  Reader constraintRDNs;
> -  Result rv = der::ExpectTagAndGetValueAtEnd(directoryNameConstraint,
> -                                             der::SEQUENCE, constraintRDNs);
> -  if (rv != Success) {
> +   uint presentedIDCount;
> +   uint directoryNameConstraintCount;
> +
> +   MatchDirectoryNamePrefix(presentedID, directoryNameConstraint,

check return value?

::: security/pkix/lib/pkixnames.cpp:1471
(Diff revision 2)
> -                                      presentedRDNs);
> -  if (rv != Success) {
> -    return rv;
> -  }
>  
> -  switch (subtreesType) {
> +   switch (subtreesType) {

I think with the refactoring it's a bit confusing to use a switch statement here. Also, it seems like all we really need to do is have the check for the excludedSubtrees case:

```
if (subtreeType == NameConstraintsSubtrees::excludedSubtrees &&
    (directoryNameConstraintCount != 0 || presentedIDCount != 0)) {
  return Result::ERROR_CERT_NOT_IN_NAME_SPACE;
}
```

(The value of `matches` is already taken care of by `MatchDirectoryNamePrefix`).
Attachment #8827848 - Flags: review?(dkeeler)
Comment on attachment 8827848 [details]
Bug 1330968 - Make OneCRL name comparisons encoding agnostic (alternative approach)

https://reviewboard.mozilla.org/r/105442/#review194124

> What's the concern here? For serial numbers, I'm pretty sure the values we're comparing are just the bytes in the DER encoding (minus tag and length), so a byte comparison should be fine.

This is the leading zero issue

> This is minor, but I think we could omit this memcmp - I don't think it'll be much faster than just calling MatchDirectoryPrefix once. A reasonable optimization would be to bail if `aItem.mDNLength != mDNLength`, though.

I'm keeping this for now, since some of the tests rely on memcmp (they don't use real RDN data)
Comment on attachment 8827848 [details]
Bug 1330968 - Make OneCRL name comparisons encoding agnostic (alternative approach)

https://reviewboard.mozilla.org/r/105442/#review194956

Ok - cool. I think the code changes are correct. We should add some more documentation and a few more test cases (outlined below). I proposed a slight API change - let me know what you think.

::: security/manager/ssl/CertBlocklist.cpp:117
(Diff revision 3)
> +      memcmp(aItem.mOtherData, mOtherData, mOtherLength) != 0) {
>      return false;
>    }
> -  return memcmp(aItem.mDNData, mDNData, mDNLength) == 0 &&
> -         memcmp(aItem.mOtherData, mOtherData, mOtherLength) == 0;
> +
> +  // if the name data are identical, the items match so we don't need to perform
> +  // further checks

Actually, you bring up a good point that we should document here - if we ever have to include data that doesn't parse as a valid DN, we'll correctly identify matches here (otherwise, we wouldn't because `MatchDirectoryNamePrefix` will return a failure and this function will return `false`).

::: security/manager/ssl/tests/unit/test_cert_blocklist.js:436
(Diff revision 3)
> +    certList.revokeCertByIssuerAndSerial(issuer1PrintableString, serial);
> +    ok(test_is_revoked(certList, atob(issuer1UTF8String), atob(serial)),
> +       "issuer / serial should be blocked despite different encoding");
> +
> +    // next revoke UTF8String, check PrintableString
> +    let issuer2UTF8String = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKDAlhYWFhYWFhYWExHTAbBgNVBAMMFGFhYWFhYWFhYWFhYWFhYWFhYWFh"

Looks like we've got some missing semicolon lint failures.

::: security/pkix/include/pkix/pkix.h:123
(Diff revision 3)
>  // - A wildcard in a DNS-ID may only appear as the entirety of the first label.
>  Result CheckCertHostname(Input cert, Input hostname,
>                           NameMatchingPolicy& nameMatchingPolicy);
>  
> +Result
> +MatchDirectoryNamePrefix(Input directoryName,

We should document this. In particular, if `matches` is false, `directoryNameCount` and `directoryNamePrefixCount` might not accurately reflect the DN counts (although I believe they will be non-zero if the DNs aren't empty, which is what we care about in `MatchPresentedDirectoryNameWithConstraint`). We could maybe adapt this to return two bools that describe what we really care about: a) if we encountered any DNs at all and b) if we encountered the same number in each (in the case where `matches` is `true`).

::: security/pkix/lib/pkixnames.cpp:1434
(Diff revision 3)
> +        if (rv != Success) {
> +          return rv;
> +        }
> +      }
>        matches = true;
>        return Success;

I'm pretty sure this does the right thing, but I can't find a testcase in https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/security/pkix/test/gtest/pkixnames_tests.cpp#1861 that exercises a particular case I'm concerned about: where `directoryNamePrefix` is empty but `directoryName` isn't (for the `NameConstraintsSubtrees::excludedSubtrees` case, we should reject it). Also, there doesn't seem to be a case for where both are empty...
We would need to add something like this:
```
{ RDN(CN("example.com")), NO_SAN,
  GeneralSubtree(DirectoryName(RDN())),
  Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE },
{ RDN(), NO_SAN,
  GeneralSubtree(DirectoryName(RDN())),
  Success, Success },
```
(although I'm not actually sure - I haven't compiled that to see if they're valid testcases, but you get the idea)
Attachment #8827848 - Flags: review?(dkeeler)
Moving to p3 because no activity for at least 24 weeks.
Priority: P1 → P3

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: bugs → nobody
Whiteboard: [psm-assigned] → [ca-onecrl][psm-assigned]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: