Closed Bug 1125261 Opened 5 years ago Closed 5 years ago

mozilla::pkix: matching a reference ID like "localhost" against a wildcard presented ID results in an assertion failure

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- unaffected
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

pkixnames.cpp:

Result
MatchPresentedDNSIDWithReferenceDNSID(
  Input presentedDNSID,
  AllowWildcards allowWildcards,
  AllowDotlessSubdomainMatches allowDotlessSubdomainMatches,
  IDRole referenceDNSIDRole,
  Input referenceDNSID,
  /*out*/ bool& matches)
{
...

  if (presented.Peek('*')) {
    if (presented.Skip(1) != Success) {
      return NotReached("Skipping '*' failed",
                        Result::FATAL_ERROR_LIBRARY_FAILURE);
    }
    do {
      uint8_t referenceByte;
      if (reference.Read(referenceByte) != Success) {
        return NotReached("invalid reference ID",
                          Result::FATAL_ERROR_INVALID_ARGS);
      }
    } while (!reference.Peek('.'));
  }

If presented is something like "*.example.com" and reference is something like "localhost", the do/while will consume reference and assert on the NotReached (or return FATAL_ERROR_INVALID_ARGS if assertions are disabled).
Attached patch patchSplinter Review
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8554061 - Flags: review?(brian)
Comment on attachment 8554061 [details] [diff] [review]
patch

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

::: security/pkix/lib/pkixnames.cpp
@@ +1162,2 @@
>        uint8_t referenceByte;
>        if (reference.Read(referenceByte) != Success) {

Please put your "if (reference.AtEnd()) block" here. This reduces the number of checks that we do in the normal case.
Attachment #8554061 - Flags: review?(brian) → review+
Comment on attachment 8554061 [details] [diff] [review]
patch

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

::: security/pkix/lib/pkixnames.cpp
@@ +1162,2 @@
>        uint8_t referenceByte;
>        if (reference.Read(referenceByte) != Success) {

Hmm...isn't "reference.AtEnd()" equivalent to "reference.Read(referenceByte) != Success"? What happens if you just substitute "matches = false; return Success;" for the "return NotReached(...)"?
Attachment #8554061 - Flags: review+ → review?(brian)
Comment on attachment 8554061 [details] [diff] [review]
patch

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

I think this is fine as it is. I remember now that we use this pattern elsewhere, and we rely on the compiler to optimize away the redundant checks. (That's one reason everything in Reader/Input is inline.)
Attachment #8554061 - Flags: review?(brian) → review+
Thanks, Brian! I did add one more test case that I thought wasn't sufficiently covered elsewhere, but those tests are fairly fast.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2445a8c795d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/58e60df8181a
https://hg.mozilla.org/mozilla-central/rev/58e60df8181a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8554061 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1107791
[User impact if declined]: assertion failures, some https pages mysteriously not loading
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: low - very little code changes with this patch
[String/UUID change made/needed]: none
Attachment #8554061 - Flags: approval-mozilla-aurora?
Comment on attachment 8554061 [details] [diff] [review]
patch

Aurora+
Attachment #8554061 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.