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..1d80f438fb3f5ca50d47c4f37d57d7f6739b1e10 100644
--- a/net/cert/internal/verify_name_match.cc
+++ b/net/cert/internal/verify_name_match.cc
@@ -2,14 +2,175 @@
 // 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 {
+
+class PrintableStringNormalizer {

[Ryan Sleevi, 2015/05/13 01:27:52]

Document :)

[mattm, 2015/05/13 03:24:05]

Acknowledged.

+ public:
+  explicit PrintableStringNormalizer(const der::Input& in)
+      : reader_(in), in_begin_(true) {}
+
+  bool ReadByte(uint8_t* out) {

[Ryan Sleevi, 2015/05/13 01:27:52]

Because these are non-trivial, my suggestion would be to move the
implementations of these methods below their class declaration.

[mattm, 2015/05/13 03:24:05]

Acknowledged.

+    if (in_begin_) {
+      // Ignore leading whitespace.
+      SkipWhitespace();
+      in_begin_ = false;
+    }
+
+    uint8_t c;
+    if (!reader_.ReadByte(&c))
+      return false;
+
+    if (c == ' ') {
+      if (SkipWhitespace()) {
+        // If there is non-whitespace characters remaining in input, compress
+        // multiple whitespace chars to a single space.
+        *out = c;
+        return true;
+      } else {
+        // If there is trailing whitespace, ignore it.
+        return false;
+      }
+    }
+
+    *out = base::ToLowerASCII(c);

[Ryan Sleevi, 2015/05/13 01:27:52]

So this doesn't enforce that |c| is validly encoded according to the
PrintableString rules (with possible exceptions)

See http://www.itu.int/rec/T-REC-X.680-200811-I/en

A-Z
a-z
0-9
space
' ( ) + , - . / : = ?

are the valid characters

(That said, "*" is so frequently mis-encoded due to OpenSSL I suspect we'll need
to support it)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1155767 for Mozilla's bug on
this.

[mattm, 2015/05/13 03:24:05]

Done.

+    return true;
+  }
+
+ private:
+  // Skip whitespace, if any. Return true if characters remain in input.
+  bool SkipWhitespace() {
+    der::ByteReader peaker(reader_);
+    while (true) {
+      uint8_t c;
+      if (!peaker.ReadByte(&c))
+        return false;
+      if (c == ' ') {
+        if (!reader_.ReadByte(&c))
+          NOTREACHED();
+      } else {
+        return true;
+      }
+    }
+  }
+
+  der::ByteReader reader_;
+  bool in_begin_;
+};

[Ryan Sleevi, 2015/05/13 01:27:52]

So as a design point, this won't scale when it comes to RFC 3280/5280 rules,
because you'll be comparing separate (but known) encodings like UTF-8 to
PrintableString.

My suggestion here would be that rather than treat this as a parser functor,
treat this as a normalization function. That is, for a given der::Input, yield
the "canonical" form (or failure). For PrintableString, the canonical form would
be a case-normalized, whitespace-eliminated form (data & length)

Then you can easily introduce additional string types by adding to the
normalization.

[mattm, 2015/05/13 03:24:05]

Done.

+
+// Compare two PrintableString values according to RFC 2459 section 4.1.2.4.
+bool PrintableStringMatch(const der::Input& a, const der::Input& b) {
+  PrintableStringNormalizer a_reader(a);
+  PrintableStringNormalizer b_reader(b);
+
+  while (true) {
+    uint8_t a_byte, b_byte;
+    bool a_done = !a_reader.ReadByte(&a_byte);
+    bool b_done = !b_reader.ReadByte(&b_byte);
+
+    if (a_done && b_done)
+      return true;
+
+    if (a_done || b_done)
+      return false;
+
+    if (a_byte != b_byte)
+      return false;

[Ryan Sleevi, 2015/05/13 01:27:52]

This ends up being pretty inefficient in a tight loop, which is why
pre-normalizing can help.

[mattm, 2015/05/13 03:24:05]

Acknowledged.

+  }
+}
+
+bool VerifyAttributeValueMatch(der::Parser* a, der::Parser* b) {
+  der::Tag a_tag, b_tag;
+  der::Input a_value, b_value;
+
+  // Read the attribute type.
+  if (!a->ReadTagAndValue(&a_tag, &a_value))
+    return false;
+  if (!b->ReadTagAndValue(&b_tag, &b_value))
+    return false;
+  // Type of "Attribute type" must be OBJECT IDENTIFIER.
+  if (a_tag != der::kOid || b_tag != der::kOid)
+    return false;
+  // Attribute types must be equal.
+  if (!a_value.Equals(b_value))
+    return false;
+
+  // Read the attribute value.
+  if (!a->ReadTagAndValue(&a_tag, &a_value))
+    return false;
+  if (!b->ReadTagAndValue(&b_tag, &b_value))
+    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.
+    return PrintableStringMatch(a_value, b_value);
+  } else {
+    // Types other than PrintableString use binary comparison.
+    return a_value.Equals(b_value);
+  }
+}
+
+bool VerifyRDNMatch(der::Parser* a, der::Parser* b) {
+  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;

[Ryan Sleevi, 2015/05/13 01:27:52]

braces

[mattm, 2015/05/13 03:24:05]

Done.

+
+  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;

[Ryan Sleevi, 2015/05/13 01:27:52]

braces

[mattm, 2015/05/13 03:24:06]

Done.

+    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