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

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
TEST=`chromeos_unittests`
TEST=manually configure EAP-TLS network in CPanel, then watch
     the `freeradius -X` logs during connection

Committed: https://crrev.com/969c512689fa55c1b3a4be0d82c2f2024bb0699a
Cr-Commit-Position: refs/heads/master@{#379056}

[Kevin Cernekee, 2016-02-27 19:10:52]

Description was changed from

==========
Allow ${CERT_SAN_EMAIL} in the ONC Identity field

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
==========

to

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
==========

[Kevin Cernekee, 2016-02-27 21:55:44]

Description was changed from

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
==========

to

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
TEST=`chromeos_unittests`
TEST=manually configure EAP-TLS network in CPanel, then watch
     the `freeradius -X` logs during connection
==========

[Kevin Cernekee, 2016-02-27 21:59:09]

cernekee@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2016-02-29 19:04:20]

Please also find someone familiar with certificates to review this. Maybe
someone on the enterprise team? If not, maybe ask sleevi@?

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:193: void ReplaceSAN(std::string*
identity,
Output parameters should be passed in last.

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:197: if (identity->find(var_name, 0) !=
std::string::npos) {
invert and early exit

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:199:
GetSubjectAltName(cert.os_cert_handle(), type, &names);
Is this intended to be x509_util::GetSubjectAltName? That seems to have a
different signature though. I'm confused.

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:544: shill::kEapIdentityProperty,
it->identity);
{}

[Kevin Cernekee, 2016-02-29 21:50:28]

cernekee@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Kevin Cernekee, 2016-02-29 21:50:30]

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:193: void ReplaceSAN(std::string*
identity,
On 2016/02/29 19:04:20, stevenjb wrote:
> Output parameters should be passed in last.

Done.

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:197: if (identity->find(var_name, 0) !=
std::string::npos) {
On 2016/02/29 19:04:20, stevenjb wrote:
> invert and early exit

Done.

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:199:
GetSubjectAltName(cert.os_cert_handle(), type, &names);
On 2016/02/29 19:04:20, stevenjb wrote:
> Is this intended to be x509_util::GetSubjectAltName? That seems to have a
> different signature though. I'm confused.

Yes, I added another variant in crrev.com/1720653002

(Is there a way to mark it as a dependent CL?  I've seen others that do this but
haven't figured out how.)

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:544: shill::kEapIdentityProperty,
it->identity);
On 2016/02/29 19:04:20, stevenjb wrote:
> {}

Done.

[stevenjb, 2016-02-29 22:06:21]

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:199:
GetSubjectAltName(cert.os_cert_handle(), type, &names);
On 2016/02/29 21:50:29, Kevin Cernekee wrote:
> On 2016/02/29 19:04:20, stevenjb wrote:
> > Is this intended to be x509_util::GetSubjectAltName? That seems to have a
> > different signature though. I'm confused.
> 
> Yes, I added another variant in crrev.com/1720653002
> 
> (Is there a way to mark it as a dependent CL?  I've seen others that do this
but
> haven't figured out how.)
It looks like this should still be in the x509_util namespace? I'm unclear how
this compiles...

WRT dependent CLs:

If you have a branch A that has been uploaded, and branch B is branched from A
(or has the upstream set to A with 'git cl upstream'), then 'git cl upload'
should take care of that. Just make sure that if you rebase A that you re-upload
A, then rebase B off A, then re-upload B.

[Kevin Cernekee, 2016-02-29 22:53:49]

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/40001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:199:
GetSubjectAltName(cert.os_cert_handle(), type, &names);
On 2016/02/29 22:06:21, stevenjb wrote:
> On 2016/02/29 21:50:29, Kevin Cernekee wrote:
> > On 2016/02/29 19:04:20, stevenjb wrote:
> > > Is this intended to be x509_util::GetSubjectAltName? That seems to have a
> > > different signature though. I'm confused.
> > 
> > Yes, I added another variant in crrev.com/1720653002
> > 
> > (Is there a way to mark it as a dependent CL?  I've seen others that do this
> but
> > haven't figured out how.)
> It looks like this should still be in the x509_util namespace? I'm unclear how
> this compiles...

Switched to new name, and added the net::x509_util:: prefix.

[stevenjb, 2016-02-29 23:00:28]

lgtm

[Ryan Sleevi, 2016-02-29 23:51:52]

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:58: // substitution.
What does this mean?

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:192: // with the SAN.
Comment: This comment is somewhat grammatically confusing. It also leaves
ambiguous a variety of things - such as that a cert may have multiple
subjectAltName extensions (not supposed to, but can)

// Replaces the token |var_name| in |identity| with the first value
// of a subjectAltName whose type is |type| from the cert |cert|.
// Note: If |cert| does not contain a subjectAltName of type |type|,
// no substitution will be performed. If |cert| contains multiple
// subjectAltName extensions, only the first extension will be considered.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:200:
net::x509_util::GetSubjectAltNameByType(cert.os_cert_handle(), type, &names);
API DESIGN: Why do you return all names, if you're clearly only interested in
the first?

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:202:
base::ReplaceSubstringsAfterOffset(identity, 0, var_name, names.at(0));
DESIGN: Why do you use this method of substitution?

The API contract is a little weird for this method, so I'm trying to grok what
the logic was.

For example, in line 197, you already found |var_name| within identity, so you
know the exact offset and length of the substring to replace, so you could just
using string->replace - but that'd only replace the first instance. But you've
also coupled checking whether or not |var_name| exists within |identity| to this
method, but that naturally feels like it should be a precondition of the caller
that checks before they call this method (that is; your API allows |identity| to
contain |var_name| or not, and that seems weird)

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:243: // stuff into the shill
properties.
Comment nit: Avoid "we" in comments; it's ambiguous. (Line 236-237 make me sad,
but let's leave them alone)

// If the policy specifies an identity containing either
// ${CERT_SAN_EMAIL} or ${CERT_SAN_UPN}, then replace that with the
// equivalent subjectAltName.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:246: net::x509_util::SAN_RFC822_NAME,
&identity);
API DESIGN: When the SAN is absent and it just contains the email within the
subject, do customers expect this to work? That is, it's entirely reasonable to
say "That's not supported", but it's unclear from the bug and design doc whether
that's been requested.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:248: net::x509_util::SAN_UPN,
&identity);
So when there's no match, you leave the |identity| string as simply containing
"${CERT_SAN_EMAIL}" (due to line 201). Is that a reasonable API contract?
Seems... less than stellar.

Concrete design questions:
1) Should the caller be allowed to specify an identity of
"${CERT_SAN_UPN}${CERT_SAN_EMAIL}" ?
2) Should replacement be mutually exclusive
3) What should the behaviour be if a field isn't present.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:221: void
SetupPolicyMatchingIssuerPEM(const std::string& identity) {
Add a param = document the param.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:221: void
SetupPolicyMatchingIssuerPEM(const std::string& identity) {
Suggestion: Make these methods separate, since it's clear you're adding this
param for one test, and one test only. For example, because of the approach
you've chosen, it logically flows that "Identity: "" is a valid Identity for an
ONC template - which seems strange/wrong/broken (it implies you're not
validating the ONC template / no identity is OK)

This helps you separate out the "EAP" section doesn't contain an identity vs
"EAP" section contains a valid identity.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver_unittest.cc:307:
SetupPolicyMatchingIssuerPEM("");
nit: std::string() [It's slightly more performant in that it makes the compiler
do less 'clever work', and slightly more clearer as to the API expectations]

Or you could update the method to take a "const char* identity" and not worry
about it, since it seems you only use fixed strings anyways.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
File chromeos/network/client_cert_util.h (right):

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_util.h:50: // The value of kIdentity, to enable
substitutions.
This documentation is entirely unclear. I'm not even sure how to improve it -
because I'm not sure what you're trying to accomplish.

[Kevin Cernekee, 2016-03-02 21:38:03]

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:58: // substitution.
On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> What does this mean?

Done.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:192: // with the SAN.
On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> Comment: This comment is somewhat grammatically confusing. It also leaves
> ambiguous a variety of things - such as that a cert may have multiple
> subjectAltName extensions (not supposed to, but can)
> 
> // Replaces the token |var_name| in |identity| with the first value
> // of a subjectAltName whose type is |type| from the cert |cert|.
> // Note: If |cert| does not contain a subjectAltName of type |type|,
> // no substitution will be performed. If |cert| contains multiple
> // subjectAltName extensions, only the first extension will be considered.

Done.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:200:
net::x509_util::GetSubjectAltNameByType(cert.os_cert_handle(), type, &names);
On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> API DESIGN: Why do you return all names, if you're clearly only interested in
> the first?

Someday I might care about other names.  The callee already has to iterate
through the SAN list to find the one that it cares about, so it's not a big deal
to have it populate a vector rather than a string.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:202:
base::ReplaceSubstringsAfterOffset(identity, 0, var_name, names.at(0));
On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> DESIGN: Why do you use this method of substitution?

It mirrors what was done for ${LOGIN_ID} and ${LOGIN_EMAIL}.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:243: // stuff into the shill
properties.
On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> Comment nit: Avoid "we" in comments; it's ambiguous. (Line 236-237 make me
sad,
> but let's leave them alone)
> 
> // If the policy specifies an identity containing either
> // ${CERT_SAN_EMAIL} or ${CERT_SAN_UPN}, then replace that with the
> // equivalent subjectAltName.

Done.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:246: net::x509_util::SAN_RFC822_NAME,
&identity);
On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> API DESIGN: When the SAN is absent and it just contains the email within the
> subject, do customers expect this to work? That is, it's entirely reasonable
to
> say "That's not supported", but it's unclear from the bug and design doc
whether
> that's been requested.

Documented this in the bug.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:248: net::x509_util::SAN_UPN,
&identity);
On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> So when there's no match, you leave the |identity| string as simply containing
> "${CERT_SAN_EMAIL}" (due to line 201). Is that a reasonable API contract?
> Seems... less than stellar.

I like it.  If a user can't get on the network and the admin sees
"${CERT_SAN_EMAIL}" in the RADIUS logs, he'll google it and figure out that it
was supposed to substitute the email address from the cert.

> Concrete design questions:
> 1) Should the caller be allowed to specify an identity of
> "${CERT_SAN_UPN}${CERT_SAN_EMAIL}" ?

Yes, this should work if the cert has both fields.

> 2) Should replacement be mutually exclusive
> 3) What should the behaviour be if a field isn't present.

This is a trivial substitution.  Let's not overcomplicate things.

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

I defer to stevenjb. I've tried to make some suggestions, I think there are
issues in performance, security, and API contracts but... this is not code I'm
an OWNER to and I admit I don't know the significance of these concerns, other
than expressing I'm concerned.

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/80001/chromeos/network/client...
chromeos/network/client_cert_resolver.cc:200:
net::x509_util::GetSubjectAltNameByType(cert.os_cert_handle(), type, &names);
On 2016/03/02 21:38:03, Kevin Cernekee wrote:
> On 2016/02/29 23:51:51, Ryan Sleevi wrote:
> > API DESIGN: Why do you return all names, if you're clearly only interested
in
> > the first?
> 
> Someday I might care about other names.

But today you don't.

>  The callee already has to iterate
> through the SAN list to find the one that it cares about, so it's not a big
deal
> to have it populate a vector rather than a string.

No, the callee wouldn't - you could short-circuit at the first SAN.

I'm a strong believer in "write what you need, not what you might need" - for
the case of dNS/IP, we need all names, so we return all names. For the case of
rfc822Name/UPN, you only need the first, so returning the first saves memory and
time.

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:235: (*cert_it->cert).os_cert_handle(),
&names);
cert_it->cert->os_cert_handle

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:238: std::string::npos) {
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)

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:240: &identity, 0,
::onc::substitutes::kCertSANEmail, names.at(0));
We strongly discourage .at() [it throws] and strongly recommend [0]

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:548: }
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]

Unfortunately I am not very familiar with the details of certificates, and
pneubeck@ is no longer working on Chrome OS, so I very much appreciate the
additional set of eyes, especially WRT any security concerns.

None of this code will be called frequently (i.e. it should be O(once)), so I am
less concerned about performance, but that is certainly no reason to be too lazy
about it either :)

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:59: // |identity| stores a copy of this
string after the substitution
s/string/property/? (I assume 'string' refers to WiFi.EAP.Identity?)

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:238: std::string::npos) {
On 2016/03/02 21:57:29, Ryan Sleevi wrote:
> 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)

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

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:240: &identity, 0,
::onc::substitutes::kCertSANEmail, names.at(0));
On 2016/03/02 21:57:28, Ryan Sleevi wrote:
> We strongly discourage .at() [it throws] and strongly recommend [0]

+1

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:548: }
On 2016/03/02 21:57:28, Ryan Sleevi wrote:
> 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.

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]

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:548: }
On 2016/03/02 22:34:59, stevenjb wrote:
> On 2016/03/02 21:57:28, Ryan Sleevi wrote:
> > 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.
> 
> 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?).

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]

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:235: (*cert_it->cert).os_cert_handle(),
&names);
On 2016/03/02 21:57:29, Ryan Sleevi wrote:
> cert_it->cert->os_cert_handle

Done.

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:238: std::string::npos) {
On 2016/03/02 21:57:29, Ryan Sleevi wrote:
> 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)

Done.

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:240: &identity, 0,
::onc::substitutes::kCertSANEmail, names.at(0));
On 2016/03/02 21:57:28, Ryan Sleevi wrote:
> We strongly discourage .at() [it throws] and strongly recommend [0]

Done.

https://codereview.chromium.org/1717123002/diff/100001/chromeos/network/clien...
chromeos/network/client_cert_resolver.cc:548: }
On 2016/03/02 21:57:28, Ryan Sleevi wrote:
> 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.

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

[stevenjb, 2016-03-03 19:19:32]

lgtm

[Kevin Cernekee, 2016-03-03 19:20:39]

The CQ bit was checked by cernekee@chromium.org

[commit-bot: I haz the power, 2016-03-03 19:21:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717123002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717123002/120001

[commit-bot: I haz the power, 2016-03-03 19:28:38]

Description was changed from

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
TEST=`chromeos_unittests`
TEST=manually configure EAP-TLS network in CPanel, then watch
     the `freeradius -X` logs during connection
==========

to

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
TEST=`chromeos_unittests`
TEST=manually configure EAP-TLS network in CPanel, then watch
     the `freeradius -X` logs during connection
==========

[commit-bot: I haz the power, 2016-03-03 19:28:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-03 19:45:07]

Description was changed from

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
TEST=`chromeos_unittests`
TEST=manually configure EAP-TLS network in CPanel, then watch
     the `freeradius -X` logs during connection
==========

to

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

Currently enterprise customers can specify ${LOGIN_ID} or ${LOGIN_EMAIL}
to tell Chrome OS to substitute user identity information into an EAP
configuration.  However, in some installations, the login ID for the
Chromebook does not match the login ID for the EAP-TLS wireless network;
instead, the EAP-TLS identity is stored in the subjectAltName field
in the client certificate.  Add code to Chrome to allow this
field to be extracted if so configured in CPanel.

BUG=549659
TEST=`chromeos_unittests`
TEST=manually configure EAP-TLS network in CPanel, then watch
     the `freeradius -X` logs during connection

Committed: https://crrev.com/969c512689fa55c1b3a4be0d82c2f2024bb0699a
Cr-Commit-Position: refs/heads/master@{#379056}
==========

[commit-bot: I haz the power, 2016-03-03 19:45:09]

Patchset 7 (id:??) landed as
https://crrev.com/969c512689fa55c1b3a4be0d82c2f2024bb0699a
Cr-Commit-Position: refs/heads/master@{#379056}