Issue 2566733003: Annotate Trusteer Rapport < 3.6 as incompatible with Chrome.

Will Harris

Description was changed from ========== Annotate Trusteer Rapport < 3.6 as incompatible with Chrome. BUG=671194 ...

4 years ago (2016-12-09 21:15:48 UTC) #1

Will Harris

Description was changed from ========== Annotate Trusteer Rapport < 3.6 as incompatible with Chrome. BUG=671194 ...

4 years ago (2016-12-10 00:07:21 UTC) #2

Will Harris

wfh@chromium.org changed reviewers: + chrisha@chromium.org, jschuh@chromium.org

4 years ago (2016-12-10 00:07:21 UTC) #3

Will Harris

chrisha -> PTAL jschuh -> for approval of the general approach.

4 years ago (2016-12-10 00:07:44 UTC) #4

Will Harris

Description was changed from ========== Annotate Trusteer Rapport < 3.6 as incompatible with Chrome. BUG=671194 ...

4 years ago (2016-12-10 00:08:00 UTC) #5

jschuh

On 2016/12/10 00:07:44, Will Harris wrote: > chrisha -> PTAL > > jschuh -> for ...

4 years ago (2016-12-10 00:19:22 UTC) #6

Will Harris

it seems finnur is the owner of enumerate_modules_model* - can you take a look? alternatively, ...

4 years ago (2016-12-10 00:32:15 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2566733003/1

4 years ago (2016-12-10 00:59:30 UTC) #12

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 1, "attempt_start_ts": 1481331520269040, "parent_rev": "7538013716114a255105019b5ec55c5e1e31579c", "commit_rev": "3059ffe1e627250c991ba0938352bec89d6f4211"}

4 years ago (2016-12-10 02:35:16 UTC) #13

commit-bot: I haz the power

Description was changed from ========== Annotate Trusteer Rapport < 3.6 as incompatible with Chrome. BUG=671194 ...

4 years ago (2016-12-10 02:35:44 UTC) #14

chrisha

Late to the party LGTM. jschuh: the actions are inherited from the old conflicts code, ...

4 years ago (2016-12-10 06:09:41 UTC) #16

Finnur

This file has changed significantly since I worked on it; I think chrisa@ is probably ...

4 years ago (2016-12-12 11:46:06 UTC) #17

Message was sent while issue was closed.

This file has changed significantly since I worked on it; I think chrisa@ is
probably a better candidate to approve this change (and already has).

But I must say I was a bit surprised to see the blacklist removed in favor of a
whitelist. I'm not against it or disappointed, it just didn't appear on my radar
(and I agree that maintaining the list was a bit onerous). But, now we've come
full circle and we're adding a blacklist again, which I find a bit amusing. :) 

Taking a step back, though -- I don't see a whitelist implementation, because
(up until this change) I don't see any modules being called out as bad. Was
everything considered to be on the whitelist up until then? If we're going for a
whitelist approach, why isn't Trusteer Rapport >= v3.6 on the whitelist, instead
of < v3.6 being on the blacklist?

> I will lgtm the general approach, without commenting on why we would support a
> RecommendedAction other than UNINSTALL. /s

I can provide a bit of background for the different recommended actions...
https://cs.chromium.org/chromium/src/chrome/browser/win/enumerate_modules_mod...

The old system was designed to be able to annotate a module and/or point to a
Help Center article to explain the problem and possible remedies -- without
having to wait for the Help Center content to appear (or change the client
afterwards to point to the new HC article). We could just blacklist and provide
a link that the HC would redirect to a generic blacklist explanation support
article until they had time to catch up and write the right response.

Why would an HC article be necessary? Well, in some cases, uninstall is
complicated/messy. In some cases, the upgrade path/how to avoid injecting the
DLL isn't obvious, and in some cases we envisioned we might need a more nuanced
recommendation, which could only be explained in more details than the
about:conflicts page has space for. We'd normalized windows paths to env
%path%'s and hashed them and the names to construct links to point to the HC.
Which had the side effect of making updates to the list a bit onerous
(constructing hashes). But the system could find suspicious DLLs which had
Microsoft/Lenovo/Logitech DLL names, but were loaded in the user's directory,
for example, and call them out specifically -- without calling out the official
DLL. We also had a UI bubble, notifying the user when there was a
particularly-bad-guaranteed-crash waiting to happen, which I believe is what
NOTIFY_USER was used for and that was used to proactively warn XP users at one
point a few years back when crashes went through the roof due to a conflicting
DLL.

Anyway, that's the background, if it helps.

commit-bot: I haz the power

Description was changed from ========== Annotate Trusteer Rapport < 3.6 as incompatible with Chrome. BUG=671194 ...

4 years ago (2016-12-12 15:05:35 UTC) #18

commit-bot: I haz the power

Patchset 1 (id:??) landed as https://crrev.com/07a5425d95dc96b3d0a3bd8acad943cf0f4cd2fb Cr-Commit-Position: refs/heads/master@{#437729}

4 years ago (2016-12-12 15:05:36 UTC) #19

chrisha

On 2016/12/12 11:46:06, Finnur wrote: > This file has changed significantly since I worked on ...

4 years ago (2016-12-13 16:37:53 UTC) #20

Message was sent while issue was closed.

On 2016/12/12 11:46:06, Finnur wrote:
> This file has changed significantly since I worked on it; I think chrisa@ is
> probably a better candidate to approve this change (and already has).
> 
> But I must say I was a bit surprised to see the blacklist removed in favor of
a
> whitelist. I'm not against it or disappointed, it just didn't appear on my
radar
> (and I agree that maintaining the list was a bit onerous). But, now we've come
> full circle and we're adding a blacklist again, which I find a bit amusing. :)

> 
> Taking a step back, though -- I don't see a whitelist implementation, because
> (up until this change) I don't see any modules being called out as bad. Was
> everything considered to be on the whitelist up until then? If we're going for
a
> whitelist approach, why isn't Trusteer Rapport >= v3.6 on the whitelist,
instead
> of < v3.6 being on the blacklist?
> 
> > I will lgtm the general approach, without commenting on why we would support
a
> > RecommendedAction other than UNINSTALL. /s
> 
> I can provide a bit of background for the different recommended actions...
>
https://cs.chromium.org/chromium/src/chrome/browser/win/enumerate_modules_mod...
> 
> The old system was designed to be able to annotate a module and/or point to a
> Help Center article to explain the problem and possible remedies -- without
> having to wait for the Help Center content to appear (or change the client
> afterwards to point to the new HC article). We could just blacklist and
provide
> a link that the HC would redirect to a generic blacklist explanation support
> article until they had time to catch up and write the right response.
> 
> Why would an HC article be necessary? Well, in some cases, uninstall is
> complicated/messy. In some cases, the upgrade path/how to avoid injecting the
> DLL isn't obvious, and in some cases we envisioned we might need a more
nuanced
> recommendation, which could only be explained in more details than the
> about:conflicts page has space for. We'd normalized windows paths to env
> %path%'s and hashed them and the names to construct links to point to the HC.
> Which had the side effect of making updates to the list a bit onerous
> (constructing hashes). But the system could find suspicious DLLs which had
> Microsoft/Lenovo/Logitech DLL names, but were loaded in the user's directory,
> for example, and call them out specifically -- without calling out the
official
> DLL. We also had a UI bubble, notifying the user when there was a
> particularly-bad-guaranteed-crash waiting to happen, which I believe is what
> NOTIFY_USER was used for and that was used to proactively warn XP users at one
> point a few years back when crashes went through the roof due to a conflicting
> DLL.
> 
> Anyway, that's the background, if it helps.

The blacklist was removed because it wasn't being used, and we didn't anticipate
wanting to use it again in the near future :/

We're still planning on moving to a whitelist, however, and actually blocking
third party libraries from being loaded (as much as is it is reasonably possible
to do so).

We'll be using chrome://conflicts messaging in the transition period before we
actually start blocking modules from loading. We plan on reusing the
notification and link mechanism for this purpose. (None of that code was
removed, simply the blacklist implementation itself.)

Will Harris

4 years ago (2016-12-13 20:35:21 UTC) #21

Message was sent while issue was closed.

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2577533002/ by wfh@chromium.org.

The reason for reverting is: These issues have been resolved in the latest
update from Trusteer..

Issue 2566733003: Annotate Trusteer Rapport < 3.6 as incompatible with Chrome. (Closed)

Description

Patch Set 1 #

Messages