Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address security concern dapps #13398

Merged
merged 3 commits into from
May 20, 2022
Merged

Address security concern dapps #13398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6badd21
Implemented eTLD+1 as bold on dialogs
qamarngr May 19, 2022
a2b8a17
fixup
qamarngr May 19, 2022
8eb3e76
fixups reviews
qamarngr May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions .../chromium/chrome/browser/crypto_wallet/fragments/dapps/AddSwitchChainNetworkFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import static org.chromium.chrome.browser.crypto_wallet.activities.BraveWalletDAppsActivity.ActivityType.ADD_ETHEREUM_CHAIN;
import static org.chromium.chrome.browser.crypto_wallet.activities.BraveWalletDAppsActivity.ActivityType.SWITCH_ETHEREUM_CHAIN;

import android.app.Activity;
import android.graphics.Bitmap;
import android.os.Bundle;
import android.text.SpannableString;
import android.text.Spanned;
Expand All @@ -12,6 +14,7 @@
import android.view.View;
import android.view.ViewGroup;
import android.widget.Button;
import android.widget.ImageView;
import android.widget.TextView;

import androidx.annotation.NonNull;
Expand All @@ -26,6 +29,7 @@
import org.chromium.brave_wallet.mojom.NetworkInfo;
import org.chromium.brave_wallet.mojom.SwitchChainRequest;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.app.BraveActivity;
import org.chromium.chrome.browser.crypto_wallet.activities.BraveWalletBaseActivity;
import org.chromium.chrome.browser.crypto_wallet.activities.BraveWalletDAppsActivity;
import org.chromium.chrome.browser.crypto_wallet.activities.BraveWalletDAppsActivity.ActivityType;
Expand All @@ -34,7 +38,11 @@
import org.chromium.chrome.browser.crypto_wallet.fragments.TwoLineItemFragment;
import org.chromium.chrome.browser.crypto_wallet.util.NavigationItem;
import org.chromium.chrome.browser.crypto_wallet.util.Utils;
import org.chromium.chrome.browser.ui.favicon.FaviconHelper;
import org.chromium.chrome.browser.ui.favicon.FaviconHelper.DefaultFaviconHelper;
import org.chromium.chrome.browser.ui.favicon.FaviconHelper.FaviconImageCallback;
import org.chromium.chrome.browser.util.TabUtils;
import org.chromium.url.GURL;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -50,6 +58,9 @@ public class AddSwitchChainNetworkFragment extends BaseDAppsFragment {
private boolean hasMultipleAddSwitchChainRequest;
private List<TwoLineItemDataSource> networks;
private List<TwoLineItemDataSource> details;
private ImageView mFavicon;
private FaviconHelper mFaviconHelper;
private DefaultFaviconHelper mDefaultFaviconHelper;

public AddSwitchChainNetworkFragment(BraveWalletDAppsActivity.ActivityType panelType) {
mPanelType = panelType;
Expand Down Expand Up @@ -129,9 +140,39 @@ public void onClick(@NonNull View widget) {
processNextSwitchChainRequest();
}
});
mFavicon = view.findViewById(R.id.fragment_add_token_iv_domain_icon);
TextView siteTv = view.findViewById(R.id.fragment_add_token_tv_site);
GURL siteUrl = Utils.getCurentTabUrl();
if (siteUrl != null) {
getBraveWalletService().geteTldPlusOneFromOrigin(Utils.getCurrentMojomOrigin(),
origin -> { siteTv.setText(Utils.geteTLD(origin.eTldPlusOne)); });
showFavIcon(siteUrl.getOrigin());
}
return view;
}

private void showFavIcon(GURL url) {
mFaviconHelper = new FaviconHelper();
BraveActivity activity = BraveActivity.getBraveActivity();
if (activity != null) {
FaviconImageCallback imageCallback =
(bitmap, iconUrl) -> setBitmapOnImageView(url, bitmap);
// 0 is a max bitmap size for download
mFaviconHelper.getLocalFaviconImageForURL(
activity.getCurrentProfile(), url, 0, imageCallback);
}
}

private void setBitmapOnImageView(GURL pageUrl, Bitmap iconBitmap) {
if (iconBitmap == null) {
if (mDefaultFaviconHelper == null) mDefaultFaviconHelper = new DefaultFaviconHelper();
iconBitmap = mDefaultFaviconHelper.getDefaultFaviconBitmap(
getActivity().getResources(), pageUrl, true);
}
mFavicon.setImageBitmap(iconBitmap);
mFavicon.setVisibility(View.VISIBLE);
}

@Override
public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) {
super.onViewCreated(view, savedInstanceState);
Expand Down Expand Up @@ -240,4 +281,11 @@ public interface AddSwitchRequestProcessListener {

void onSwitchRequestProcessed(boolean hasMoreRequests);
}

@Override
public void onDestroy() {
super.onDestroy();
if (mFaviconHelper != null) mFaviconHelper.destroy();
if (mDefaultFaviconHelper != null) mDefaultFaviconHelper.clearCache();
}
}
3 changes: 2 additions & 1 deletion ...ava/org/chromium/chrome/browser/crypto_wallet/fragments/dapps/ConnectAccountFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ public void onClick(View v) {
mRecyclerView = view.findViewById(R.id.accounts_list);
mFavicon = view.findViewById(R.id.favicon);

mWebSite.setText(getCurrentHostHttpAddress().getSpec());
getBraveWalletService().geteTldPlusOneFromOrigin(Utils.getCurrentMojomOrigin(),
origin -> { mWebSite.setText(Utils.geteTLD(origin.eTldPlusOne)); });
initComponents();

return view;
Expand Down
8 changes: 7 additions & 1 deletion .../java/org/chromium/chrome/browser/crypto_wallet/fragments/dapps/DAppsMessageFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@
package org.chromium.chrome.browser.crypto_wallet.fragments.dapps;

import android.os.Bundle;
import android.text.SpannableStringBuilder;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.TextView;

import org.chromium.brave_wallet.mojom.SignMessageRequest;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.crypto_wallet.util.Utils;

public class DAppsMessageFragment extends BaseDAppsFragment {
private SignMessageRequest mCurrentSignMessageRequest;
Expand All @@ -27,7 +29,11 @@ public View onCreateView(
LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
View view = inflater.inflate(R.layout.fragment_dapps_message, container, false);
TextView signMessageText = view.findViewById(R.id.sign_message_text);
signMessageText.setText(mCurrentSignMessageRequest.message);
SpannableStringBuilder spBuilder = new SpannableStringBuilder();
spBuilder.append(Utils.geteTLD(mCurrentSignMessageRequest.originInfo.eTldPlusOne));
spBuilder.append(" ");
spBuilder.append(getString(R.string.brave_wallet_provide_encryption_key_description));
signMessageText.setText(spBuilder);

return view;
}
Expand Down
33 changes: 24 additions & 9 deletions ...chromium/chrome/browser/crypto_wallet/permission/BraveEthereumPermissionPromptDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.chromium.chrome.browser.crypto_wallet.permission;

import android.app.Activity;
import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.Rect;
Expand All @@ -20,10 +21,13 @@
import org.chromium.base.annotations.NativeMethods;
import org.chromium.brave_wallet.mojom.AccountInfo;
import org.chromium.brave_wallet.mojom.BraveWalletConstants;
import org.chromium.brave_wallet.mojom.BraveWalletService;
import org.chromium.brave_wallet.mojom.KeyringService;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.app.BraveActivity;
import org.chromium.chrome.browser.crypto_wallet.BraveWalletServiceFactory;
import org.chromium.chrome.browser.crypto_wallet.KeyringServiceFactory;
import org.chromium.chrome.browser.crypto_wallet.util.Utils;
import org.chromium.content_public.browser.ImageDownloadCallback;
import org.chromium.content_public.browser.WebContents;
import org.chromium.mojo.bindings.ConnectionErrorHandler;
Expand Down Expand Up @@ -57,6 +61,7 @@ public class BraveEthereumPermissionPromptDialog
private int mRequestId; // Used for favicon downloader
private KeyringService mKeyringService;
private boolean mMojoServicesClosed;
private BraveWalletService mBraveWalletService;

@CalledByNative
private static BraveEthereumPermissionPromptDialog create(long nativeDialogController,
Expand Down Expand Up @@ -84,11 +89,11 @@ void show() {
mFavIconImage = customView.findViewById(R.id.favicon);
setFavIcon();
mRecyclerView = customView.findViewById(R.id.accounts_list);
GURL visibleUrl = mWebContents.getVisibleUrl();
if (visibleUrl != null) {
TextView domain = customView.findViewById(R.id.domain);
domain.setText(visibleUrl.getOrigin().getSpec());
}

InitBraveWalletService();
TextView domain = customView.findViewById(R.id.domain);
mBraveWalletService.geteTldPlusOneFromOrigin(Utils.getCurrentMojomOrigin(),
origin -> { domain.setText(Utils.geteTLD(origin.eTldPlusOne)); });

mPropertyModel =
new PropertyModel.Builder(ModalDialogProperties.ALL_KEYS)
Expand All @@ -111,6 +116,13 @@ void show() {
}
}

private void InitBraveWalletService() {
if (mBraveWalletService != null) {
return;
}
mBraveWalletService = BraveWalletServiceFactory.getInstance().getBraveWalletService(this);
}

private void initAccounts() {
assert mKeyringService != null;
mKeyringService.getKeyringInfo(BraveWalletConstants.DEFAULT_KEYRING_ID, keyringInfo -> {
Expand Down Expand Up @@ -202,11 +214,14 @@ private void dismissDialog() {

public void DisconnectMojoServices() {
mMojoServicesClosed = true;
if (mKeyringService == null) {
return;
if (mKeyringService != null) {
mKeyringService.close();
mKeyringService = null;
}
if (mBraveWalletService != null) {
mBraveWalletService.close();
mBraveWalletService = null;
}
mKeyringService.close();
mKeyringService = null;
}

@Override
Expand Down
26 changes: 26 additions & 0 deletions android/java/org/chromium/chrome/browser/crypto_wallet/util/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
import android.net.Uri;
import android.os.Build;
import android.os.Handler;
import android.text.Html;
import android.text.SpannableString;
import android.text.Spanned;
import android.text.style.ClickableSpan;
import android.util.Pair;
import android.view.View;
Expand Down Expand Up @@ -1498,6 +1500,30 @@ public static boolean isBiometricAvailable(Context context) {
}
}

public static Spanned geteTLD(String etldPlusOne) {
GURL url = getCurentTabUrl();
StringBuilder builder = new StringBuilder();
builder.append(url.getScheme()).append("://").append(url.getHost());
int index = builder.indexOf(etldPlusOne);
if (index > 0 && index < builder.length()) {
builder.insert(index, "<b>");
builder.insert(builder.length(), "</b>");
}
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
return Html.fromHtml(builder.toString(), Html.FROM_HTML_MODE_LEGACY);
} else {
return Html.fromHtml(builder.toString());
}
}

public static GURL getCurentTabUrl() {
ChromeTabbedActivity activity = BraveActivity.getChromeTabbedActivity();
if (activity != null) {
return activity.getActivityTab().getUrl().getOrigin();
}
return GURL.emptyGURL();
}

@NonNull
public static Profile getProfile(boolean isIncognito) {
ChromeActivity chromeActivity = BraveActivity.getBraveActivity();
Expand Down
1 change: 1 addition & 0 deletions android/java/res/layout/fragment_dapps_message.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
android:layout_height="wrap_content"
android:minHeight="100dp"
android:inputType="none"
android:paddingVertical="10dp"
android:textIsSelectable="true"
android:focusable="false"
android:longClickable="false"
Expand Down
5 changes: 4 additions & 1 deletion browser/ui/android/strings/android_brave_strings.grd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2993,7 +2993,10 @@ If you don't accept this request, VPN will not reconnect and your internet conne
Disconnect
</message>
<message name="IDS_FRAGMENT_SIGN_MESSAGE_DESCRIPTION" desc="Sign message dialog description.">
Your signature is being requested
A DApp is requesting your public encryption key
</message>
<message name="IDS_BRAVE_WALLET_PROVIDE_ENCRYPTION_KEY_DESCRIPTION" desc="Provide encryption key panel details">
is requesting your wallets public encryption key. If you consent to providing this key, the site will be able to compose encrypted messages to you.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wallet's?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes @qamarngr could you pls fix and uplift?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same in IDS_BRAVE_WALLET_PROVIDE_ENCRYPTION_KEY_TITLE in components/resources/wallet_strings.grdp

Copy link
Member

@bbondy bbondy Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also looks like this string isn't using a proper variable for the origin in question. Languages can't be translated 1:1 via concatenation like that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbondy will fix it this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkarolin What should be fixed in IDS_BRAVE_WALLET_PROVIDE_ENCRYPTION_KEY_TITLE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://ci.brave.com/job/brave-core-create-uplift-prs/1917/

</message>
<message name="IDS_BRAVE_WALLET_ALLOW_ADD_NETWORK_NAME" desc="Add network panel name label">
Network name
Expand Down
6 changes: 6 additions & 0 deletions components/brave_wallet/browser/brave_wallet_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,12 @@ void BraveWalletService::GetActiveOrigin(GetActiveOriginCallback callback) {
std::move(callback).Run(MakeOriginInfo(url::Origin()));
}

void BraveWalletService::GeteTLDPlusOneFromOrigin(
const url::Origin& origin,
GetActiveOriginCallback callback) {
std::move(callback).Run(MakeOriginInfo(origin));
}

void BraveWalletService::GetPendingSignMessageRequests(
GetPendingSignMessageRequestsCallback callback) {
std::vector<mojom::SignMessageRequestPtr> requests;
Expand Down
2 changes: 2 additions & 0 deletions components/brave_wallet/browser/brave_wallet_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ class BraveWalletService : public KeyedService,
const std::string& account,
ResetPermissionCallback callback) override;
void GetActiveOrigin(GetActiveOriginCallback callback) override;
void GeteTLDPlusOneFromOrigin(const url::Origin& origin,
GetActiveOriginCallback callback) override;
void GetPendingSignMessageRequests(
GetPendingSignMessageRequestsCallback callback) override;
void NotifySignMessageRequestProcessed(bool approved, int id) override;
Expand Down
4 changes: 4 additions & 0 deletions components/brave_wallet/common/brave_wallet.mojom
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,10 @@ interface BraveWalletService {
// Obtains the active origin info.
GetActiveOrigin() => (OriginInfo origin_info);

// Obtains the active origin info with eTLD+1
GeteTLDPlusOneFromOrigin(url.mojom.Origin origin)
=> (OriginInfo origin_info);

// These are used for UI notifying native when the user approve/reject SignMessage[Hardware]Request
NotifySignMessageRequestProcessed(bool approved, int32 id);
// Obtains the pending sign message requests
Expand Down