RFC 2459 name comparison.

net/cert/internal/verify_name_match.cc

Index: net/cert/internal/verify_name_match.cc
diff --git a/net/cert/internal/verify_name_match.cc b/net/cert/internal/verify_name_match.cc
index 3f0d7718ed0969444888cc7e9e5d1da16bcd6fee..8f172165915f2937358610836c083f9396e3233b 100644
--- a/net/cert/internal/verify_name_match.cc
+++ b/net/cert/internal/verify_name_match.cc
@@ -2,14 +2,178 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/strings/string_util.h"
 #include "net/cert/internal/verify_name_match.h"
 #include "net/der/input.h"
+#include "net/der/parser.h"
+#include "net/der/tag.h"
 
 namespace net {
 
-bool VerifyNameMatch(const der::Input& a, const der::Input& b) {
+namespace {
+
+// Advance |reader| so that the next read will return a non-space character, or
+// the end of the input. Return true if non-space characters remain in |reader|.
+bool SkipASCIISpace(der::ByteReader* reader) {
+  der::ByteReader peeker(*reader);
+  while (true) {
+    uint8_t c;
+    if (!peeker.ReadByte(&c))
+      return false;
+    if (c == ' ') {
+      if (!reader->ReadByte(&c))
+        NOTREACHED();
+    } else {
+      return true;
+    }
+  }
+}
+
+// Normalize a PrintableString value according to RFC 2459 section 4.1.2.4.
+bool NormalizePrintableString(const der::Input& in, std::string* output) {
+  der::ByteReader reader(in);
+
+  // Normalized version will always be equal or shorter than input, pre-reserve
+  // the space to avoid re-allocations.
+  output->reserve(in.Length());
+
+  // Ignore leading whitespace.
+  SkipASCIISpace(&reader);
+
+  uint8_t c;
+  while (reader.ReadByte(&c)) {
+    if (c == ' ') {
+      // If there is non-whitespace characters remaining in input, compress
+      // multiple whitespace chars to a single space, otherwise ignore trailing
+      // whitespace.
+      if (SkipASCIISpace(&reader))
+        *output += ' ';
+    } else if (c >= 'A' && c <= 'Z') {
+      // Fold case.
+      *output += base::ToLowerASCII(c);
+    } else if ((c >= 'a' && c <= 'z') || (c >= '\'' && c <= ')') ||
+               (c >= '+' && c <= ':') || c == '=' || c == '?') {
+      // Accept remaining allowed characters:
+      // a-z
+      // ' ( )
+      // + , - . / 0 1 2 3 4 5 6 7 8 9 :
+      // = ?
+      *output += c;
+    } else {
+      // Fail on any characters that are not valid for PrintableString.
+      // TODO(mattm): will we need to include '*'?
+      return false;
+    }
+  }

[Ryan Sleevi, 2015/05/21 02:20:17]

Do we know what the performance implications of this will be? I'm concerned
we'll get into a situation like https://crbug.com/457949

I'm now wondering what the implications are of the "avoid UnsafeData", and
wonder if we should instead offer some sort of ReadToString() routine for making
copies of the data.

For the short-term, it's probably OK to do

std::string output(static_cast<const char*>(in.UnsafeData()), in.Length());

for (size_t read_pos = 0, write_pos = 0, end = output.size();
     read_pos < end; ++read_pos) {
  c = output[read_pos];
  if (c == ' ')
    if (write_pos && output[write_pos - 1] != ' ')
      output[write_pos++] = ' ';
  } else if (c >= 'A' && c <= 'Z') {
    output[write_pos++] = c + ('a' - 'A');
  } else if (...)
    output[write_pos++] = c;
  } else {
    return false;
  }
}
output.resize(write_pos);


I don't know if that's worth it. It may be overkill. It's something to keep an
eye out for performance wise. The above doesn't handle the case of trailing
whitespace, and skips leading whitespace, mostly because I can't remember what's
called for.

My concern is that the reader.ReadByte() may have enough to force a de-opt for
the pointer scanning, while the above approach may be better, at least gets
string checking. I'm not sure how I feel. It could also be rewritten using
iterators, which at least gets us checked iterators, and probably should be, but
I figured I'd throw it out there.

[mattm, 2015/06/17 03:36:18]

I did some simple benchmark (release build, run unittests with
--gtest_repeat=5000 --single-process-tests, retry a few times and take the best
time). I compared ReadByte vs std::string copy like your example, and also
compared one like your example but using base::StringPiece to read from the
input (to see if the extra copy mattered.)

The version with ReadByte was about 7% slower. string vs StringPiece was pretty
much in the noise. So I'll just go with std::string since that has the advantage
of checked iterators.

+
+  return true;
+}
+
+bool VerifyAttributeValueMatch(der::Parser* a, der::Parser* b) {
+  der::Input a_value, b_value;
+
+  // Read the attribute types, which must be OBJECT IDENTIFIERs.
+  if (!a->ReadTag(der::kOid, &a_value))
+    return false;
+  if (!b->ReadTag(der::kOid, &b_value))
+    return false;
+  // Attribute types must be equal.
+  if (!a_value.Equals(b_value))
+    return false;
+
+  // Read the attribute value.
+  der::Tag a_tag, b_tag;
+  if (!a->ReadTagAndValue(&a_tag, &a_value))
+    return false;
+  if (!b->ReadTagAndValue(&b_tag, &b_value))
+    return false;
+
+  // There should be no more elements in the sequence after reading the
+  // attribute type and value.
+  if (a->HasMore() || b->HasMore())
+    return false;
+
   // TODO(mattm): use normalization as specified in RFC 5280 section 7.
-  return a.Equals(b);
+
+  // RFC 2459 section 4.1.2.4 comparison rules:
+  // Attributes encoded with different types may be assumed to be unequal.
+  if (a_tag != b_tag)
+    return false;
+  if (a_tag == der::kPrintableString) {
+    // PrintableString values should be compared case insenstive and ignoring
+    // extraneous whitespace.
+    std::string a_normalized, b_normalized;
+    if (!NormalizePrintableString(a_value, &a_normalized) ||
+        !NormalizePrintableString(b_value, &b_normalized))
+      return false;
+    return a_normalized == b_normalized;
+  } else {

[Ryan Sleevi, 2015/05/21 02:20:17]

don't else after a return

[mattm, 2015/06/17 03:36:19]

Done.

+    // Types other than PrintableString use binary comparison.
+    return a_value.Equals(b_value);
+  }
+}
+
+bool VerifyRDNMatch(der::Parser* a, der::Parser* b) {
+  // Must have at least one AttributeTypeAndValue.
+  if (!a->HasMore() || !b->HasMore())
+    return false;
+
+  while (a->HasMore() && b->HasMore()) {
+    der::Parser a_attr_type_and_value;
+    der::Parser b_attr_type_and_value;
+    if (!a->ReadSequence(&a_attr_type_and_value) ||
+        !b->ReadSequence(&b_attr_type_and_value))
+      return false;
+    if (!VerifyAttributeValueMatch(&a_attr_type_and_value,
+                                   &b_attr_type_and_value))
+      return false;
+  }
+
+  // If one of the RDNs has more elements than the other, not a match.
+  if (a->HasMore() || b->HasMore())
+    return false;
+
+  return true;
+}
+
+}  // namespace
+
+// TODO(mattm): is returning false on parsing errors ok, or should it try to
+// fall back to binary comparison on unexpected input?
+bool VerifyNameMatch(const der::Input& a, const der::Input& b) {
+  der::Parser a_parser(a);
+  der::Parser b_parser(b);
+  der::Parser a_rdn_sequence;
+  der::Parser b_rdn_sequence;
+
+  if (!a_parser.ReadSequence(&a_rdn_sequence) ||
+      !b_parser.ReadSequence(&b_rdn_sequence)) {
+    return false;
+  }
+
+  // No data should remain in the inputs after the RDN sequence.
+  if (a_parser.HasMore() || b_parser.HasMore())
+    return false;
+
+  // Must have at least one RDN.
+  if (!a_rdn_sequence.HasMore() || !b_rdn_sequence.HasMore())
+    return false;
+
+  while (a_rdn_sequence.HasMore() && b_rdn_sequence.HasMore()) {
+    der::Parser a_rdn, b_rdn;
+    if (!a_rdn_sequence.ReadConstructed(der::kSet, &a_rdn) ||
+        !b_rdn_sequence.ReadConstructed(der::kSet, &b_rdn)) {
+      return false;
+    }
+    if (!VerifyRDNMatch(&a_rdn, &b_rdn))
+      return false;
+  }
+
+  // If one of the sequences has more elements than the other, not a match.
+  if (a_rdn_sequence.HasMore() || b_rdn_sequence.HasMore())
+    return false;
+
+  return true;
 }
 
 }  // namespace net