Allow ${CERT_SAN_EMAIL} and ${CERT_SAN_UPN} in the ONC Identity field

chromeos/network/client_cert_resolver.cc

Index: chromeos/network/client_cert_resolver.cc
diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc
index 3d438d8843c018caac5ec1a9018a0ce0ec639532..acbd657e806d8c5f167d6a2d4634296918f16d88 100644
--- a/chromeos/network/client_cert_resolver.cc
+++ b/chromeos/network/client_cert_resolver.cc
@@ -14,6 +14,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/strings/string_util.h"
 #include "base/task_runner.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/clock.h"
@@ -25,6 +26,8 @@
 #include "dbus/object_path.h"
 #include "net/cert/scoped_nss_types.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util_nss.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
@@ -34,11 +37,13 @@ struct ClientCertResolver::NetworkAndMatchingCert {
   NetworkAndMatchingCert(const std::string& network_path,
                          client_cert::ConfigType config_type,
                          const std::string& cert_id,
-                         int slot_id)
+                         int slot_id,
+                         const std::string& configured_identity)
       : service_path(network_path),
         cert_config_type(config_type),
         pkcs11_id(cert_id),
-        key_slot_id(slot_id) {}
+        key_slot_id(slot_id),
+        identity(configured_identity) {}
 
   std::string service_path;
   client_cert::ConfigType cert_config_type;
@@ -48,6 +53,12 @@ struct ClientCertResolver::NetworkAndMatchingCert {
 
   // The id of the slot containing the certificate and the private key.
   int key_slot_id;
+
+  // The ONC WiFi.EAP.Identity field can contain variables like
+  // ${CERT_SAN_EMAIL} which are expanded by ClientCertResolver.
+  // |identity| stores a copy of this string after the substitution

[stevenjb, 2016/03/02 22:34:59]

s/string/property/? (I assume 'string' refers to WiFi.EAP.Identity?)

+  // has been done.
+  std::string identity;
 };
 
 typedef std::vector<ClientCertResolver::NetworkAndMatchingCert>
@@ -197,6 +208,8 @@ void FindCertificateMatches(const net::CertificateList& certs,
                      MatchCertWithPattern(it->cert_config.pattern));
     std::string pkcs11_id;
     int slot_id = -1;
+    std::string identity;
+
     if (cert_it == client_certs.end()) {
       VLOG(1) << "Couldn't find a matching client cert for network "
               << it->service_path;
@@ -211,9 +224,36 @@ void FindCertificateMatches(const net::CertificateList& certs,
         // the worst case the user can remove the problematic cert.
         continue;
       }
+
+      // If the policy specifies an identity containing ${CERT_SAN_xxx},
+      // see if the cert contains a suitable subjectAltName that can be
+      // stuffed into the shill properties.
+      identity = it->cert_config.policy_identity;
+      std::vector<std::string> names;
+
+      net::x509_util::GetRFC822SubjectAltNames(
+          (*cert_it->cert).os_cert_handle(), &names);

[Ryan Sleevi, 2016/03/02 21:57:29]

cert_it->cert->os_cert_handle

[Kevin Cernekee, 2016/03/02 22:57:43]

Done.

+      if (!names.empty()) {
+        if (identity.find(::onc::substitutes::kCertSANEmail, 0) !=
+            std::string::npos) {

[Ryan Sleevi, 2016/03/02 21:57:29]

This is still quite inefficient.

size_t email_offset = identity.find(kCertSANEmail, 0);
if (email_offset != std::string::npos) {
  std::vector<std::string> names;
  net::x509_util::GetRFC822SubjectAltNames(cert_it->cert->os_cert_handle(),
&names);
  if (!names.empty()) {
    base::ReplaceSubstringsAfterOffset(&identity, email_offset, kCertSANEmail,
names[0]);
  }
}


(Or even better if you just return the first name)

[stevenjb, 2016/03/02 22:34:59]

+1 to suggested optimization. I am less concerned about an optimized
GetRFC822SubjectAltNames() to get just the first name.

[Kevin Cernekee, 2016/03/02 22:57:43]

Done.

+          base::ReplaceSubstringsAfterOffset(
+              &identity, 0, ::onc::substitutes::kCertSANEmail, names.at(0));

[Ryan Sleevi, 2016/03/02 21:57:28]

We strongly discourage .at() [it throws] and strongly recommend [0]

[stevenjb, 2016/03/02 22:34:59]

+1

[Kevin Cernekee, 2016/03/02 22:57:43]

Done.

+        }
+      }
+      net::x509_util::GetUPNSubjectAltNames((*cert_it->cert).os_cert_handle(),
+                                            &names);
+      if (!names.empty()) {
+        if (identity.find(::onc::substitutes::kCertSANUPN, 0) !=
+            std::string::npos) {
+          base::ReplaceSubstringsAfterOffset(
+              &identity, 0, ::onc::substitutes::kCertSANUPN, names.at(0));
+        }
+      }
     }
+
     matches->push_back(ClientCertResolver::NetworkAndMatchingCert(
-        it->service_path, it->cert_config.location, pkcs11_id, slot_id));
+        it->service_path, it->cert_config.location, pkcs11_id, slot_id,
+        identity));
   }
 }
 
@@ -502,6 +542,10 @@ void ClientCertResolver::ConfigureCertificates(NetworkCertMatches* matches) {
                                       it->key_slot_id,
                                       it->pkcs11_id,
                                       &shill_properties);
+      if (!it->identity.empty()) {
+        shill_properties.SetStringWithoutPathExpansion(
+            shill::kEapIdentityProperty, it->identity);
+      }

[Ryan Sleevi, 2016/03/02 21:57:28]

So, I tried to highlight it to you with the suggested comment reword, but

SECURITY: You allow passing embedded NULs in the identity down to Shill. Shill
interfaces with the WPASupplicant, but it translates things to .c_str()

(see EapCredentials::PopulateSupplicantProperties in Shill). I don't know the
extent to which this is meaningful, but this has been a repeated source of
security bugs in the past.

[stevenjb, 2016/03/02 22:34:59]

That sounds to me like a security issue we should deal with in Shill. We pass a
lot of user provided utf8 strings to Shill, I don't think it is practical to
audit each instance.

If this is impractical to handle in Shill, maybe we can address this at a lower
level in the Chrome network configuration stack, e.g. in
ShillClientHelper::AppendServicePropertiesDictionary. I'm stretched pretty think
right now, but it it's a real vulnerability it should be straightforward to
fixup any strings where this would be unexpected (e.g. maybe excepting cert
strings?).

[Ryan Sleevi, 2016/03/02 22:46:17]

Mostly, I don't know to what extent the identity is trusted in Shill - is this
just advisory, or is this security relevant?

Consider a certificate which had an embedded nul in the UPN, which appears
(escape) as

"user@example.com\0user2@example.com"

We'd end up passing to WPASupplicant that the identity was "user@example.com" -
that is, truncating the UPN. However, if that "user@example.com" has to be
backed up by some other facet - such as the EAP-TLS client certificate (which
shows them as "user@example.com\0user2@example.com"), then it's not really a
security bug, it's merely a functional bug.

More interesting would be the case if an empty identity created any issues
(which comes up with the previous CL suggestion about whether "Identity: """ was
valid in the ONC config), which is can an empty Shill identity create any issues
when passed to WPASupplicant (that is, we'd pass a c_str() that just contains
NUL). I don't know there either - that depends on WPASupplicant.

I agree that, given Shill is exposed via DBus, it makes much more sense to put
the logic in Shill to validate incoming parameters. But I did want to highlight
how we'd end up passing stuff, potentially hostile, and that care should be
taken to figure out whether or not that's OK. I didn't really see a reply to
this previously, so the answer is "I don't know"

[Kevin Cernekee, 2016/03/02 22:57:43]

Is there a standard way to truncate or detect strings with embedded NUL bytes in
Chrome?

I would be equally happy with either of the following:

1) Truncate at the first NUL (if any) and reject if the resultant string is
empty
2) Reject if any embedded NULs are present

     }
     network_properties_changed_ = true;
     DBusThreadManager::Get()->GetShillServiceClient()->