diff --git a/api/java/org/openyolo/api/internal/CredentialRetrieveActivity.java b/api/java/org/openyolo/api/internal/CredentialRetrieveActivity.java index e1130af..ca30cf9 100644 --- a/api/java/org/openyolo/api/internal/CredentialRetrieveActivity.java +++ b/api/java/org/openyolo/api/internal/CredentialRetrieveActivity.java @@ -19,8 +19,8 @@ import android.app.Activity; import android.content.Context; import android.content.Intent; -import android.os.BadParcelableException; import android.os.Bundle; +import android.support.annotation.NonNull; import android.support.annotation.Nullable; import android.util.Log; import android.view.WindowManager.LayoutParams; @@ -29,13 +29,11 @@ import com.google.bbq.QueryResponse; import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; import org.openyolo.protocol.CredentialRetrieveRequest; import org.openyolo.protocol.CredentialRetrieveResult; import org.openyolo.protocol.Protobufs; -import org.openyolo.protocol.Protobufs.CredentialRetrieveBbqResponse; +import org.openyolo.protocol.ProtocolConstants; import org.openyolo.protocol.internal.IntentUtil; /** @@ -79,7 +77,7 @@ public void onCreate(@Nullable Bundle savedInstanceState) { .queryFor( CREDENTIAL_DATA_TYPE, request.toProtocolBuffer(), - new CredentialRetrieveQueryCallback()); + new CredentialRetrieveQueryCallback(request)); } @Override @@ -90,10 +88,16 @@ protected void onDestroy() { private class CredentialRetrieveQueryCallback implements QueryCallback { + + private CredentialRetrieveRequest mRetrieveRequest; + + CredentialRetrieveQueryCallback(@NonNull CredentialRetrieveRequest retrieveRequest) { + mRetrieveRequest = retrieveRequest; + } + @Override public void onResponse(long queryId, List queryResponses) { ArrayList retrieveIntents = new ArrayList<>(); - Map protoResponses = new HashMap<>(); for (QueryResponse queryResponse : queryResponses) { Protobufs.CredentialRetrieveBbqResponse response; try { @@ -104,28 +108,37 @@ public void onResponse(long queryId, List queryResponses) { continue; } - protoResponses.put(queryResponse.responderPackage, response); - - if (response.getRetrieveIntent().isEmpty()) { - continue; - } - - Intent retrieveIntent; - try { - retrieveIntent = + // a provider indicates that it has credentials via the credentials_available field + // on the response proto. Prior 0.3.0, the provider would directly return an intent. + // For backwards compatibility this is currently inspected as a fallback for the + // absence of the credentials_available field. + + if (response.getCredentialsAvailable()) { + Intent retrieveIntent = new Intent( + ProtocolConstants.RETRIEVE_CREDENTIAL_ACTION); + retrieveIntent.setPackage(queryResponse.responderPackage); + retrieveIntent.putExtra( + ProtocolConstants.EXTRA_RETRIEVE_REQUEST, + mRetrieveRequest.toProtocolBuffer().toByteArray()); + + if (!response.getPreloadedData().isEmpty()) { + retrieveIntent.putExtra(ProtocolConstants.EXTRA_RETRIEVE_PRELOADED_DATA, + response.getPreloadedData().toByteArray()); + } + + retrieveIntents.add(retrieveIntent); + } else if (!response.getRetrieveIntent().isEmpty()) { + // TODO: remove backwards compatibility with retrieve_intent after 0.3.0 + Intent retrieveIntent = IntentUtil.fromBytes(response.getRetrieveIntent().toByteArray()); - } catch (BadParcelableException ex) { - Log.w(LOG_TAG, "Failed to parse intent from bytes"); - continue; - } - if (!queryResponse.responderPackage.equals( - retrieveIntent.getComponent().getPackageName())) { - Log.w(LOG_TAG, "Package mismatch between provider and retrieve intent"); - continue; + if (!queryResponse.responderPackage.equals( + retrieveIntent.getComponent().getPackageName())) { + Log.w(LOG_TAG, "Package mismatch between provider and retrieve intent"); + } else { + retrieveIntents.add(retrieveIntent); + } } - - retrieveIntents.add(retrieveIntent); } if (retrieveIntents.isEmpty()) { diff --git a/demoproviders/barbican/java/org/openyolo/demoprovider/barbican/provider/CredentialQueryReceiver.java b/demoproviders/barbican/java/org/openyolo/demoprovider/barbican/provider/CredentialQueryReceiver.java index 3be6944..5776987 100644 --- a/demoproviders/barbican/java/org/openyolo/demoprovider/barbican/provider/CredentialQueryReceiver.java +++ b/demoproviders/barbican/java/org/openyolo/demoprovider/barbican/provider/CredentialQueryReceiver.java @@ -15,7 +15,6 @@ package org.openyolo.demoprovider.barbican.provider; import android.content.Context; -import android.content.Intent; import android.support.annotation.NonNull; import android.util.Log; import com.google.bbq.Protobufs.BroadcastQuery; @@ -28,7 +27,6 @@ import org.openyolo.protocol.AuthenticationDomain; import org.openyolo.protocol.CredentialRetrieveRequest; import org.openyolo.protocol.Protobufs.CredentialRetrieveBbqResponse; -import org.openyolo.protocol.internal.IntentUtil; import org.openyolo.spi.BaseCredentialQueryReceiver; /** @@ -85,15 +83,11 @@ protected void processCredentialRequest( + query.getRequestingApp() + ": " + credentialsFound); - if (credentialsFound) { - Intent retrieveIntent = RetrieveCredentialActivity.createIntent(context, request); - CredentialRetrieveBbqResponse response = - CredentialRetrieveBbqResponse.newBuilder() - .setRetrieveIntent(IntentUtil.toByteString(retrieveIntent)) - .build(); - responseBytes = response.toByteArray(); - } - - responseSender.sendResponse(query, responseBytes); + responseSender.sendResponse( + query, + CredentialRetrieveBbqResponse.newBuilder() + .setCredentialsAvailable(credentialsFound) + .build() + .toByteArray()); } } diff --git a/demoproviders/trapdoor/java/org/openyolo/demoprovider/trapdoor/CredentialQueryReceiver.java b/demoproviders/trapdoor/java/org/openyolo/demoprovider/trapdoor/CredentialQueryReceiver.java index 9e1377a..3c4017b 100644 --- a/demoproviders/trapdoor/java/org/openyolo/demoprovider/trapdoor/CredentialQueryReceiver.java +++ b/demoproviders/trapdoor/java/org/openyolo/demoprovider/trapdoor/CredentialQueryReceiver.java @@ -20,13 +20,11 @@ import android.util.Log; import com.google.bbq.Protobufs.BroadcastQuery; import com.google.bbq.QueryResponseSender; -import com.google.protobuf.ByteString; import java.util.Set; import org.openyolo.protocol.AuthenticationDomain; import org.openyolo.protocol.AuthenticationMethods; import org.openyolo.protocol.CredentialRetrieveRequest; import org.openyolo.protocol.Protobufs.CredentialRetrieveBbqResponse; -import org.openyolo.protocol.internal.IntentUtil; import org.openyolo.spi.BaseCredentialQueryReceiver; /** @@ -67,7 +65,7 @@ protected void processCredentialRequest( Intent retrieveIntent = RetrieveActivity.createIntent(applicationContext); CredentialRetrieveBbqResponse response = CredentialRetrieveBbqResponse.newBuilder() - .setRetrieveIntent(ByteString.copyFrom(IntentUtil.toBytes(retrieveIntent))) + .setCredentialsAvailable(true) .build(); Log.i(LOG_TAG, "Accepting credential request for claimed caller: " diff --git a/protocol/java/org/openyolo/protocol/ProtocolConstants.java b/protocol/java/org/openyolo/protocol/ProtocolConstants.java index adb893e..b4d110c 100644 --- a/protocol/java/org/openyolo/protocol/ProtocolConstants.java +++ b/protocol/java/org/openyolo/protocol/ProtocolConstants.java @@ -34,6 +34,15 @@ public final class ProtocolConstants { */ public static final String EXTRA_RETRIEVE_RESULT = "org.openyolo.credential.retrieve.result"; + /** + * The extra value key used to carry preloaded data as part of a credential retrieve Intent. + * This data is supplied by the credential provider during the broadcast phase of the retrieve + * flow, and sent back verbatim. The provider _should not_ implicitly trust this data, as it + * is possible for the client to tamper with it. + */ + public static final String EXTRA_RETRIEVE_PRELOADED_DATA = + "org.openyolo.credential.retrieve.preloaded_data"; + /** * The extra key value used to carry a hint request. */ diff --git a/protocol/java/org/openyolo/protocol/RetrieveBbqResponse.java b/protocol/java/org/openyolo/protocol/RetrieveBbqResponse.java deleted file mode 100644 index 497d1bf..0000000 --- a/protocol/java/org/openyolo/protocol/RetrieveBbqResponse.java +++ /dev/null @@ -1,216 +0,0 @@ -/* - * Copyright 2016 The OpenYOLO Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except - * in compliance with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the - * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either - * express or implied. See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.openyolo.protocol; - -import static org.hamcrest.CoreMatchers.notNullValue; -import static org.valid4j.Assertive.require; - -import android.content.Intent; -import android.os.Parcel; -import android.os.Parcelable; -import android.support.annotation.NonNull; -import android.support.annotation.Nullable; -import com.google.protobuf.InvalidProtocolBufferException; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; -import java.util.Set; -import org.openyolo.protocol.Protobufs.CredentialRetrieveBbqResponse; -import org.openyolo.protocol.internal.ByteStringConverters; -import org.openyolo.protocol.internal.CollectionConverter; -import org.openyolo.protocol.internal.IntentUtil; - -/** - * A response to a {@link CredentialRetrieveRequest credential retrieve request}. - */ -public class RetrieveBbqResponse implements Parcelable { - - /** - * Parcelable reader for {@link RetrieveBbqResponse} instances. - * @see android.os.Parcelable - */ - public static final Creator CREATOR = new RetrieveResultCreator(); - - @Nullable - private final Intent mRetrieveIntent; - - @NonNull - private final Map mResponses; - - private RetrieveBbqResponse( - @Nullable Intent retrieveIntent, - @NonNull Map responses) { - mRetrieveIntent = retrieveIntent; - mResponses = responses; - } - - /** - * An intent that can be used to retrieve a credential for the calling app. If no credentials - * are available, this will be {@code null}. User input may or may not be required to retrieve - * the credential, at the discretion of the credential provider. The retrieve intent - * should be fired when the app is ready to retrieve the credential - typically immediately - * after the retrieve result is received. - */ - @Nullable - public Intent getRetrieveIntent() { - return mRetrieveIntent; - } - - /** - * The package names of responding credential providers that indicated they may have a usable - * credential. - */ - @NonNull - public Set getResponderPackageNames() { - return mResponses.keySet(); - } - - /** - * Retrieves the intent for the specified responder package name, if available. - */ - @NonNull - public Intent getRetrieveIntentForResponder(@NonNull String responderPackageName) { - require(responderPackageName, notNullValue()); - CredentialRetrieveBbqResponse response = mResponses.get(responderPackageName); - if (response == null) { - throw new IllegalArgumentException(responderPackageName + " is not a responder"); - } - - return IntentUtil.fromBytes(response.getRetrieveIntent().toByteArray()); - } - - /** - * Retrieves the additional, non-standard response parameters for the specified responder - * package name, if available. - */ - @NonNull - public Map getAdditionalPropsForResponder(String responderPackageName) { - CredentialRetrieveBbqResponse response = mResponses.get(responderPackageName); - if (response == null) { - throw new IllegalArgumentException(responderPackageName + " is not a responder"); - } - - return CollectionConverter.convertMapValues( - response.getAdditionalPropsMap(), - ByteStringConverters.BYTE_STRING_TO_BYTE_ARRAY); - } - - /** - * Creates {@link RetrieveBbqResponse} instances. - */ - public static final class Builder { - - @Nullable - private Intent mIntent; - - @NonNull - private Map mProtoResponses; - - /** - * Starts the process of describing a credential retrieve result. - */ - public Builder() { - mIntent = null; - mProtoResponses = Collections.emptyMap(); - } - - /** - * Specifies the map of protocol buffer responses received for the credential request, - * keyed by the package name of the responder. - */ - @NonNull - public Builder setProtoResponses( - @NonNull Map protoResponses) { - require(protoResponses, notNullValue()); - for (CredentialRetrieveBbqResponse value : protoResponses.values()) { - require(value, notNullValue()); - } - mProtoResponses = protoResponses; - return this; - } - - /** - * Specifies the retrieve intent, that can be used to retrieve existing credentials from - * the provider, if available. - */ - @NonNull - public Builder setRetrieveIntent(@Nullable Intent intent) { - mIntent = intent; - return this; - } - - /** - * Creates a {@link RetrieveBbqResponse} with the properties described to the builder. - */ - @NonNull - public RetrieveBbqResponse build() { - return new RetrieveBbqResponse(mIntent, mProtoResponses); - } - } - - @Override - public void writeToParcel(Parcel dest, int flags) { - dest.writeParcelable(mRetrieveIntent, 0); - dest.writeInt(mResponses.size()); - - for (String key : mResponses.keySet()) { - byte[] responseBytes = mResponses.get(key).toByteArray(); - dest.writeString(key); - dest.writeInt(responseBytes.length); - dest.writeByteArray(responseBytes); - } - } - - @Override - public int describeContents() { - return 0; - } - - static final class RetrieveResultCreator implements Creator { - @Override - public RetrieveBbqResponse createFromParcel(Parcel in) { - Intent retrieveIntent = in.readParcelable(Intent.class.getClassLoader()); - int numResponses = in.readInt(); - - Map responses = new HashMap<>(numResponses); - for (int i = 0; i < numResponses; i++) { - String key = in.readString(); - int responseSize = in.readInt(); - byte[] responseBytes = new byte[responseSize]; - in.readByteArray(responseBytes); - - CredentialRetrieveBbqResponse response; - try { - response = CredentialRetrieveBbqResponse.parseFrom(responseBytes); - } catch (InvalidProtocolBufferException ex) { - throw new IllegalArgumentException("Unable to parse response proto"); - } - - responses.put(key, response); - } - - return new RetrieveBbqResponse.Builder() - .setRetrieveIntent(retrieveIntent) - .setProtoResponses(responses) - .build(); - } - - @Override - public RetrieveBbqResponse[] newArray(int size) { - return new RetrieveBbqResponse[size]; - } - } - -} diff --git a/protocol/javatests/org/openyolo/protocol/RetrieveResultTest.java b/protocol/javatests/org/openyolo/protocol/RetrieveResultTest.java deleted file mode 100644 index c8f2c9a..0000000 --- a/protocol/javatests/org/openyolo/protocol/RetrieveResultTest.java +++ /dev/null @@ -1,110 +0,0 @@ -/* - * Copyright 2016 The OpenYOLO Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package org.openyolo.protocol; - -import static junit.framework.TestCase.assertNotNull; -import static org.assertj.core.api.Java6Assertions.assertThat; - -import android.content.Intent; -import android.os.Parcel; -import java.util.HashMap; -import java.util.Map; -import java.util.Set; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.openyolo.protocol.internal.IntentUtil; -import org.robolectric.RobolectricTestRunner; -import org.robolectric.annotation.Config; -import org.valid4j.errors.RequireViolation; - -/** - * Battery of tests for the RetrieveIntent class - */ -@RunWith(RobolectricTestRunner.class) -@Config(manifest = Config.NONE) -public class RetrieveResultTest { - - private RetrieveBbqResponse underTest; - - @Before - public void setUp() throws Exception { - Intent fromIntent = new Intent("com.openyolo.ACTION"); - fromIntent.setPackage("com.openyolo"); - byte[] fromBytes = IntentUtil.toBytes(fromIntent); - Intent retrieveIntent= IntentUtil.fromBytes(fromBytes); - - Map protoResponses = new HashMap<>(); - - underTest = new RetrieveBbqResponse.Builder() - .setProtoResponses(protoResponses) - .setRetrieveIntent(retrieveIntent) - .build(); - } - - @Test - public void testGetRetrieveIntent(){ - Intent intent = underTest.getRetrieveIntent(); - assertNotNull(intent); - } - - @Test - public void testGetResponderPackageNames(){ - Set responders = underTest.getResponderPackageNames(); - assertNotNull(responders); - } - - @Test(expected = RequireViolation.class) - public void testGetRetrieveIntentForResponder_null(){ - underTest.getRetrieveIntentForResponder(null); - } - - @Test(expected = IllegalArgumentException.class) - public void testGetRetrieveIntentForResponder_illegal(){ - underTest.getRetrieveIntentForResponder("BLahblah"); - } - - @Test - public void testWriteAndRead() { - Parcel p = Parcel.obtain(); - underTest.writeToParcel(p, 0); - p.setDataPosition(0); - - RetrieveBbqResponse result = RetrieveBbqResponse.CREATOR.createFromParcel(p); - checkIntentsEquivalent(underTest.getRetrieveIntent(), result.getRetrieveIntent()); - assertThat(result.getResponderPackageNames()) - .isEqualTo(underTest.getResponderPackageNames()); - - for (String responder : result.getResponderPackageNames()) { - checkIntentsEquivalent( - underTest.getRetrieveIntentForResponder(responder), - result.getRetrieveIntentForResponder(responder)); - - assertThat(result.getAdditionalPropsForResponder(responder)) - .isEqualTo(underTest.getAdditionalPropsForResponder(responder)); - } - } - - private void checkIntentsEquivalent(Intent expected, Intent actual) { - // NOTE: we don't check the full intent, just the basic properties - assertThat(actual.getPackage()) - .isEqualTo(expected.getPackage()); - assertThat(actual.getAction()) - .isEqualTo(expected.getAction()); - } -} diff --git a/protocol/proto/openyolo.proto b/protocol/proto/openyolo.proto index 80bfbd1..11349e9 100644 --- a/protocol/proto/openyolo.proto +++ b/protocol/proto/openyolo.proto @@ -1,5 +1,5 @@ /* - * Copyright 2016 Google Inc. All Rights Reserved. + * Copyright 2016 The OpenYOLO Authors. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -33,8 +33,40 @@ message CredentialRetrieveRequest { } message CredentialRetrieveBbqResponse { - bytes retrieve_intent = 1; + /** + * Prior to OpenYOLO 0.3.0, a provider would directly supply the Intent for the client to + * invoke via this retrieve_intent field. This exposes the client to a number of possible + * security vulnerabilities: + * + * - A cross-app request forgery attack if the Intent is targeted at a different app. + * - A code injection attack exploiting deserialization bugs (see CVE-2015-3837) + * + * As such, this has now been deprecated in favor of the credentials_available and + * preloaded_data fields. The client will now construct the Intent itself if the provider + * sets credentials_available to true. The preloaded_data field should be treated as an opaque + * byte array from the client's perspective, and if provided should be handed back verbatim to + * the provider via an Intent extra. The provider, of course, should not implicitly trust this + * data as it is possible for the client to tamper with it. As such, the client + */ + bytes retrieve_intent = 1 [deprecated=true]; + + /** + * Additional, non-standard properties that the provider can supply as part of the response. + */ map additional_props = 2; + + /** + * Indicates whether the provider believes it has credentials available matching the client's + * request. + */ + bool credentials_available = 3; + + /** + * Additional data that the client should send back verbatim, as an Intent extra, to the + * provider if the client chooses to invoke this provider for credential retrieval. The + * byte array should be treated as opaque and meaningless from the client's perspective. + */ + bytes preloaded_data = 4; } message CredentialRetrieveBbqResponseList {