Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
254083007:
Auth work to support new auth protocol (Closed)
|
Created:
6 years, 7 months ago by Munjal (Google) Modified:
6 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAuth work to support new auth protocol
BUG=345759
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269445
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270007
Patch Set 1 #
Total comments: 13
Patch Set 2 : Use hard-coded public keys #
Total comments: 7
Patch Set 3 : Code review changes #Patch Set 4 : Indent #Patch Set 5 : Compile error #Patch Set 6 : Fix for ghost non-const parameter #
Messages
Total messages: 24 (0 generated)
Mark/Ryan, I still have to add / update unit tests, but can you please take an initial look?
https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:50: static const net::SHA1HashValue kICAFingerprints[] = { Note: We don't use fingerprints for any (granting) of security privileges - we always do a full comparison of the cert if the fingerprints hit, in part because certs are crazy. Instead of fingerprints, can you use the three trusted public keys from these certs? https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_channel.proto (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_channel.proto:59: RSASSA_PSS = 2; Just to confirm, you can change this symbolic name later, right? In the case of PSS, you'd also want to specify the parameters (the salt/mgf function - although collectively, just use mgf1 with SHA-whatever) Concretely: RSASSA_PKCS1v15 -> RSASSA_PKCS1v15_SHA1 RSASSA_PSS -> RSASSA_PSS_SHA256
https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:50: static const net::SHA1HashValue kICAFingerprints[] = { On 2014/04/30 01:32:13, Ryan Sleevi wrote: > Note: We don't use fingerprints for any (granting) of security privileges - we > always do a full comparison of the cert if the fingerprints hit, in part because > certs are crazy. > > Instead of fingerprints, can you use the three trusted public keys from these > certs? Let me see if I understand correctly. So you are suggesting a change to the solution we discussed over email in that instead of just checking finger prints, we check the whole public key of the intermediate cert? https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:130: } Ryan, does this logic look OK to you? https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_channel.proto (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_channel.proto:59: RSASSA_PSS = 2; On 2014/04/30 01:32:13, Ryan Sleevi wrote: > Just to confirm, you can change this symbolic name later, right? > > In the case of PSS, you'd also want to specify the parameters (the salt/mgf > function - although collectively, just use mgf1 with SHA-whatever) > > Concretely: > RSASSA_PKCS1v15 -> RSASSA_PKCS1v15_SHA1 > RSASSA_PSS -> RSASSA_PSS_SHA256 This is just a copy of the proto definition that is in the spec and is also on the server side. Yes, we can change the name in the future without problems.
Code structure comments - I'll let Ryan handle use of the crypto libraries :-) https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:116: if (response.intermediate_certificate_size() > 0) { ISTM having two helper functions that extract the public certificate from the response - one for the intermediates case, one without - would make for cleaner code. Each would return (or populate) trusted_ca_key_der_item. https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:121: VLOG(1) << "Disallowed intermdiate cert: " << ica; ica looks to be a binary blob that is parsed to yield the certificate. Please log something human readable, or failing that, the byte length of the blob. https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:126: VLOG(1) << "Could not extract public key from intermediate cert: " << ica; Ditto. https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:156: trusted_ca_key_der_item.data = reinterpret_cast<unsigned char*>( You are taking the data pointer out of ica_public_key_str, casting away the constness and giving it to a data structure that is passed to another library. This is not safe as any modification to the char array will not be reflected in the state of ica_public_key_str. IMHO it would be safer to allocate a copy for the SECItem. https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_channel.proto (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_channel.proto:59: RSASSA_PSS = 2; On 2014/05/05 00:01:46, Munjal (Google) wrote: > On 2014/04/30 01:32:13, Ryan Sleevi wrote: > > Just to confirm, you can change this symbolic name later, right? > > > > In the case of PSS, you'd also want to specify the parameters (the salt/mgf > > function - although collectively, just use mgf1 with SHA-whatever) > > > > Concretely: > > RSASSA_PKCS1v15 -> RSASSA_PKCS1v15_SHA1 > > RSASSA_PSS -> RSASSA_PSS_SHA256 > > This is just a copy of the proto definition that is in the spec and is also on > the server side. Yes, we can change the name in the future without problems. Only the value is sent on the wire so names can be changed later (obviously you don't want to change the meaning by doing so).
https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:50: static const net::SHA1HashValue kICAFingerprints[] = { On 2014/05/05 00:01:46, Munjal (Google) wrote: > On 2014/04/30 01:32:13, Ryan Sleevi wrote: > > Note: We don't use fingerprints for any (granting) of security privileges - we > > always do a full comparison of the cert if the fingerprints hit, in part > because > > certs are crazy. > > > > Instead of fingerprints, can you use the three trusted public keys from these > > certs? > > Let me see if I understand correctly. So you are suggesting a change to the > solution we discussed over email in that instead of just checking finger prints, > we check the whole public key of the intermediate cert? I apologize for the confusion. The goal is to have the same security guarantees you have now. The fingerprint approach is safe if and only if you're checking the signature against a root - that is, that there is no chance of attacks on the fingerprint alg (SHA-1) using the structure of an X.509 certificate. Since you're not - you're wanting to use the ICA as a trust anchor - you should be checking using the same method you're already using to check a trust anchor - that is, the public key. https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:156: trusted_ca_key_der_item.data = reinterpret_cast<unsigned char*>( On 2014/05/05 19:22:22, mark a. foltz wrote: > You are taking the data pointer out of ica_public_key_str, casting away the > constness and giving it to a data structure that is passed to another library. > This is not safe as any modification to the char array will not be reflected in > the state of ica_public_key_str. IMHO it would be safer to allocate a copy for > the SECItem. > This is fine/safe/blessed. NSS is slowly becoming most explicit in its const-correctness. I'll file an upstream bug to note that both the CERTSignedData and the CERTSubjectPublicKeyInfo are 'const' (implying they're safe from modification - even though the SECItem takes a char*)
https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:50: static const net::SHA1HashValue kICAFingerprints[] = { On 2014/05/05 19:35:32, Ryan Sleevi wrote: > On 2014/05/05 00:01:46, Munjal (Google) wrote: > > On 2014/04/30 01:32:13, Ryan Sleevi wrote: > > > Note: We don't use fingerprints for any (granting) of security privileges - > we > > > always do a full comparison of the cert if the fingerprints hit, in part > > because > > > certs are crazy. > > > > > > Instead of fingerprints, can you use the three trusted public keys from > these > > > certs? > > > > Let me see if I understand correctly. So you are suggesting a change to the > > solution we discussed over email in that instead of just checking finger > prints, > > we check the whole public key of the intermediate cert? > > I apologize for the confusion. The goal is to have the same security guarantees > you have now. The fingerprint approach is safe if and only if you're checking > the signature against a root - that is, that there is no chance of attacks on > the fingerprint alg (SHA-1) using the structure of an X.509 certificate. > > Since you're not - you're wanting to use the ICA as a trust anchor - you should > be checking using the same method you're already using to check a trust anchor - > that is, the public key. So are you suggesting that we hard-code a list of public keys?
On May 5, 2014 2:00 PM, <munjal@chromium.org> wrote: > > > https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... > File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc > (right): > > https://codereview.chromium.org/254083007/diff/1/chrome/browser/extensions/ap... > chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:50: > static const net::SHA1HashValue kICAFingerprints[] = { > On 2014/05/05 19:35:32, Ryan Sleevi wrote: >> >> On 2014/05/05 00:01:46, Munjal (Google) wrote: >> > On 2014/04/30 01:32:13, Ryan Sleevi wrote: >> > > Note: We don't use fingerprints for any (granting) of security > > privileges - >> >> we >> > > always do a full comparison of the cert if the fingerprints hit, > > in part >> >> > because >> > > certs are crazy. >> > > >> > > Instead of fingerprints, can you use the three trusted public keys > > from >> >> these >> > > certs? >> > >> > Let me see if I understand correctly. So you are suggesting a change > > to the >> >> > solution we discussed over email in that instead of just checking > > finger >> >> prints, >> > we check the whole public key of the intermediate cert? > > >> I apologize for the confusion. The goal is to have the same security > > guarantees >> >> you have now. The fingerprint approach is safe if and only if you're > > checking >> >> the signature against a root - that is, that there is no chance of > > attacks on >> >> the fingerprint alg (SHA-1) using the structure of an X.509 > > certificate. > >> Since you're not - you're wanting to use the ICA as a trust anchor - > > you should >> >> be checking using the same method you're already using to check a > > trust anchor - >> >> that is, the public key. > > > So are you suggesting that we hard-code a list of public keys? > > https://codereview.chromium.org/254083007/ Yes, precisely. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL. I made the changes to use public keys and I think I addressed all code review comments (some of which are not relevant since code is refactored a bit).
https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:181: return i; TYPE-CASTING: Downcasting a size_t to an int has been a source of security bugs in the past. You should be explicit that this is "ok" by doing a "static_cast<int>(i);" indicating that this is intentional, and not an accidental translation (and avoiding compiler warnings that will warn that this is dangerous) https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:250: if (response.intermediate_certificate_size() > 0) { This condition is unnecessary (it's handled by lines 242 & 244) https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:263: if (!CheckCertPublicKey(ica, *trusted_ca_key_der)) { It's not necessary to perform this check, since you used the baked-in public key (rather than the certificate's public key) to perform the signature check (on line 289-290)
lgtm Please address Ryan's comments as well. https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:181: return i; Also see checked_cast in safe_conversions.h. https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe... On 2014/05/07 23:34:49, Ryan Sleevi wrote: > TYPE-CASTING: Downcasting a size_t to an int has been a source of security bugs > in the past. > > You should be explicit that this is "ok" by doing a "static_cast<int>(i);" > indicating that this is intentional, and not an accidental translation (and > avoiding compiler warnings that will warn that this is dangerous)
https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc (right): https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:181: return i; On 2014/05/07 23:34:49, Ryan Sleevi wrote: > TYPE-CASTING: Downcasting a size_t to an int has been a source of security bugs > in the past. > > You should be explicit that this is "ok" by doing a "static_cast<int>(i);" > indicating that this is intentional, and not an accidental translation (and > avoiding compiler warnings that will warn that this is dangerous) Done. I chose to use static_cast since I know that the value will be safe to convert to an int and checked_cast's check for value is redundant. https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:250: if (response.intermediate_certificate_size() > 0) { On 2014/05/07 23:34:49, Ryan Sleevi wrote: > This condition is unnecessary (it's handled by lines 242 & 244) Good catch. Residue of a bit of refactoring. https://codereview.chromium.org/254083007/diff/10001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_auth_util_nss.cc:263: if (!CheckCertPublicKey(ica, *trusted_ca_key_der)) { On 2014/05/07 23:34:49, Ryan Sleevi wrote: > It's not necessary to perform this check, since you used the baked-in public key > (rather than the certificate's public key) to perform the signature check (on > line 289-290) Removed.
Can you add some unit tests? Just testing these APIs would be sufficient. LGTM
Let me add tests in a separate patch since this needs to go in for M36. I will definitely add them.
The CQ bit was checked by munjal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/254083007/40001
The CQ bit was unchecked by munjal@chromium.org
The CQ bit was checked by munjal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/254083007/40001
Message was sent while issue was closed.
Change committed as 269445
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/278383002/ by munjal@chromium.org. The reason for reverting is: This pathc is already reverted.
The CQ bit was checked by munjal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/254083007/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 270007 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
