New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 606832 link

Starred by 3 users
Status: Fixed
Owner:
█ kapishnikov@chromium.org
Last visit > 30 days ago
Closed: Jul 2016
Cc:
est...@chromium.org
rsleevi@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

[cronet] public key pinns should not be skipped for local installed cert

Reported by kangzhan...@gmail.com, Apr 26 2016 
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.7 Safari/537.36

Steps to reproduce the problem:
1. Setup charles to inspect the https traffic. 
2. Install the charles cert to the phone.
3. Use addPublicKeyPins API to pin the cert. 
4. See the traffic can still be intercepted by chareles. 

What is the expected behavior?
Cronet fail the request because the local installed cert doesn't match the pin. 

What went wrong?
Cronet should not skip local certificates in public key pin check.

Did this work before? N/A 

Chrome version: 51.0.2704.7  Channel: n/a
OS Version: OS X 10.11.4
Flash Version: Shockwave Flash 21.0 r0

I saw this behavior was mentioned in 522275's discussion but not implemented in the final api. Can we revisit this? We are worried that third party app could trick the user to install their cert and perform mitm attack.

Comment 1 by ashej...@chromium.org, Apr 27 2016

Components: Internals>Network>SSL
Labels: Te-NeedsFurtherTriage

Comment 2 by rsleevi@chromium.org, Apr 27 2016

Components: -Internals>Network>SSL Internals>Network>Library
Labels: -OS-Mac
https://www.chromium.org/Home/chromium-security/security-faq#TOC-How-does-key-pinning-interact-with-local-proxies-and-filters-

Comment 3 by mef@chromium.org, Apr 27 2016

Cc: rsleevi@chromium.org est...@chromium.org
Labels: OS-Android
Status: Available (was: Unconfirmed)
We had an internal discussion, and agreed that we need to enforce pinning in Cronet even when it chains to a non-standard trust anchor. This will be an API option in Cronet which is not on by default.

Comment 4 by kangzhan...@gmail.com, Apr 27 2016 

Thanks man. Having an option to turn it on sounds good!

Comment 5 by mef@chromium.org, May 3 2016

Owner: rsleevi@chromium.org
Status: Assigned (was: Available)
Assigning to Ryan for underlying net implementation. We'll need to add Cronet API flag once that is done.

Comment 6 by rsleevi@chromium.org, May 3 2016

Owner: mef@chromium.org
Misha: Considering this is something Chrome would never ship, to implement anything, we first need to know your API shape. Recall that during the discussion we also tried to figure out how this API should look in N+, where such features can be controlled by the package, as it inherently makes such an API deprecated.

Assigning back to you for the requirements gathering and specification. And also the implementation, since I pointed out the book that enforces this logic.

Comment 7 by mef@chromium.org, Jun 13 2016

Labels: M-53
Owner: kapishnikov@chromium.org

Comment 8 by mef@chromium.org, Jun 13 2016

Labels: -Te-NeedsFurtherTriage

Comment 9 by sidv@chromium.org, Jun 13 2016 

Update : Andrei has sent a proposal. sleevi@ et al to provide feedback. It'd be nice to  have this in M53.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 1 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf

commit 9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf
Author: kapishnikov <kapishnikov@chromium.org>
Date: Fri Jul 01 18:29:11 2016

Enable public key pinning of local trust anchors

Provides a new API that allows Cronet client to enable public key pinning of local trust anchors, i.e. pinning of certificates added to the local user trust store.

/**
 * Enables or disables pinning of the local (user-level) trust anchors.
 *
 * @param value {@code true} to enable pinning, {@code false} to disable.
 * @return the builder to facilitate chaining.
 */
 public Builder enablePublicKeyPinsForLocalTrustAnchors(boolean value);

BUG= 606832 

Review-Url: https://codereview.chromium.org/2052363002
Cr-Commit-Position: refs/heads/master@{#403486}

[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/test/cronet_url_request_context_config_test.cc
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/test/mock_cert_verifier.cc
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/android/test/src/org/chromium/net/QuicTestServer.java
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/url_request_context_config.h
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/components/cronet/url_request_context_config_unittest.cc
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/net/http/transport_security_state.cc
[modify] https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf/net/http/transport_security_state.h

Comment 11 by kapishnikov@chromium.org, Jul 1 2016

Status: Fixed (was: Assigned)
The new API to enable/disable pinning bypass for local trust anchors. The bypass is enabled by default.

/**
 * Enables or disables public key pinning bypass for local trust anchors.
 *
 * @param value {@code true} to enable the bypass, {@code false} to disable.
 * @return the builder to facilitate chaining.
 */
public Builder enablePublicKeyPinningBypassForLocalTrustAnchors(boolean value);
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 1 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/025788d30fdf38744ebe718d8d8ecd5da161f8a9

commit 025788d30fdf38744ebe718d8d8ecd5da161f8a9
Author: kelvinp <kelvinp@chromium.org>
Date: Fri Jul 01 19:22:31 2016

Revert of Enable public key pinning of local trust anchors (patchset #5 id:80001 of https://codereview.chromium.org/2052363002/ )

Reason for revert:
Android Bot Compilation failure:

../../components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java:196: error: method createMockCertVerifier in class MockCertVerifier cannot be applied to given types;

https://build.chromium.org/p/chromium/builders/Android/builds/58499/steps/compile/logs/stdio

Original issue's description:
> Enable public key pinning of local trust anchors
>
> Provides a new API that allows Cronet client to enable public key pinning of local trust anchors, i.e. pinning of certificates added to the local user trust store.
>
> /**
>  * Enables or disables pinning of the local (user-level) trust anchors.
>  *
>  * @param value {@code true} to enable pinning, {@code false} to disable.
>  * @return the builder to facilitate chaining.
>  */
>  public Builder enablePublicKeyPinsForLocalTrustAnchors(boolean value);
>
> BUG= 606832 
>
> Committed: https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf
> Cr-Commit-Position: refs/heads/master@{#403486}

TBR=rsleevi@chromium.org,mef@chromium.org,xunjieli@chromium.org,kapishnikov@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 606832 

Review-Url: https://codereview.chromium.org/2117763004
Cr-Commit-Position: refs/heads/master@{#403498}

[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/test/cronet_url_request_context_config_test.cc
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/test/mock_cert_verifier.cc
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/android/test/src/org/chromium/net/QuicTestServer.java
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/url_request_context_config.h
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/components/cronet/url_request_context_config_unittest.cc
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/net/http/transport_security_state.cc
[modify] https://crrev.com/025788d30fdf38744ebe718d8d8ecd5da161f8a9/net/http/transport_security_state.h

Comment 13 by rsleevi@chromium.org, Jul 1 2016

Labels: -M-53 M-54
Status: Assigned (was: Fixed)
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 1 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/385aa4234b65c226458700c8c622d705c95eab50

commit 385aa4234b65c226458700c8c622d705c95eab50
Author: kapishnikov <kapishnikov@chromium.org>
Date: Fri Jul 01 20:53:02 2016

Enable public key pinning of local trust anchors

Provides a new API that allows Cronet client to enable public key pinning of local trust anchors, i.e. pinning of certificates added to the local user trust store.

/**
 * Enables or disables pinning of the local (user-level) trust anchors.
 *
 * @param value {@code true} to enable pinning, {@code false} to disable.
 * @return the builder to facilitate chaining.
 */
 public Builder enablePublicKeyPinsForLocalTrustAnchors(boolean value);

BUG= 606832 

Committed: https://crrev.com/9cf8e6f923a9b472c8b3521d52b3b6ca910f77cf
Review-Url: https://codereview.chromium.org/2052363002
Cr-Original-Commit-Position: refs/heads/master@{#403486}
Cr-Commit-Position: refs/heads/master@{#403521}

[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/test/cronet_url_request_context_config_test.cc
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/test/javaperftests/src/org/chromium/net/CronetPerfTestActivity.java
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/test/mock_cert_verifier.cc
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/android/test/src/org/chromium/net/QuicTestServer.java
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/url_request_context_config.cc
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/url_request_context_config.h
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/components/cronet/url_request_context_config_unittest.cc
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/net/http/transport_security_state.cc
[modify] https://crrev.com/385aa4234b65c226458700c8c622d705c95eab50/net/http/transport_security_state.h

Comment 15 by kapishnikov@chromium.org, Jul 1 2016

Status: Fixed (was: Assigned)
Now it should be good. I filed a bug to include the build of cronet perf test APK to "android_cronet_tester".
►

Sign in to add a comment