1421018 - Block TBNotifier.exe from accessing accessible services

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Jim Mathies [:jimm]]

David and I agree, this is basically malware and is unwanted. We can block access to this client.

Comment 1

[Jim Mathies [:jimm]]

Things we need to do here:

1) Since the blocklist code for exe checking isn't implemented yet, we'll need to add it. This shouldn't be a lot of work, we're just talking about walking over a list of strings and doing some safe string comparing.

There's a place holder for the check [1] in the code currently. We need a new static exe list in addition to the existing dll list [2], and we need to walk over that list and check it against the a11y instantiator we detect, which we have as a nsIFile [3].

I would also suggest adding a new environment variable for disabling the exe check, similar to the dll module check. We might want to rename the existing env variable [4] to something more appropriate. I think it might be useful to have switches for both block lists features.

[1] https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/accessible/windows/msaa/LazyInstantiator.cpp#295
[2] https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/accessible/windows/msaa/LazyInstantiator.cpp#231
[3] https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/accessible/windows/msaa/LazyInstantiator.cpp#287
[4] https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/accessible/windows/msaa/LazyInstantiator.cpp#248

Comment 2

[Eitan Isaacson [:eeejay]]

(In reply to Jim Mathies [:jimm] from comment #1)
> I would also suggest adding a new environment variable for disabling the exe
> check, similar to the dll module check. We might want to rename the existing
> env variable [4] to something more appropriate. I think it might be useful
> to have switches for both block lists features.
> 

Why wouldn't you want both block lists to use the same env variable? Is there a case when you want to block remote but not local or vice versa?

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Create accessible client blocklist and add TBNNotifier.exe. r?jimm

I used the same environment variable name for both. I can change that if you think having them be distinct is useful. Couldn't install ask.com notifier, but I tested on inspect.exe and it works.

Comment 4

[Eitan Isaacson [:eeejay]]

I guess a needinfo too is excessive.. sorry for the spam.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8933825 [details] [diff] [review]
Create accessible client blocklist and add TBNNotifier.exe. r?jimm

Review of attachment 8933825 [details] [diff] [review]:
-----------------------------------------------------------------

FYI I'm fine with just one env flag for the whole thing.

::: accessible/windows/msaa/LazyInstantiator.cpp
@@ +302,5 @@
> +  nsresult rv;
> +  if (!PR_GetEnv("MOZ_DISABLE_ACCESSIBLE_BLOCKLIST")) {
> +    // Debugging option is not present, so check blocklist.
> +    nsAutoString leafName;
> +    rv = clientExe->GetLeafName(leafName);

is this retrieving the exe name or a directory name?

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Jim Mathies [:jimm] from comment #5)
> Comment on attachment 8933825 [details] [diff] [review]
> Create accessible client blocklist and add TBNNotifier.exe. r?jimm
> 
> Review of attachment 8933825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FYI I'm fine with just one env flag for the whole thing.
> 
> ::: accessible/windows/msaa/LazyInstantiator.cpp
> @@ +302,5 @@
> > +  nsresult rv;
> > +  if (!PR_GetEnv("MOZ_DISABLE_ACCESSIBLE_BLOCKLIST")) {
> > +    // Debugging option is not present, so check blocklist.
> > +    nsAutoString leafName;
> > +    rv = clientExe->GetLeafName(leafName);
> 
> is this retrieving the exe name or a directory name?

The exe name. That's what we want. Right?

Comment 7

[Jim Mathies [:jimm]]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > Comment on attachment 8933825 [details] [diff] [review]
> > Create accessible client blocklist and add TBNNotifier.exe. r?jimm
> > 
> > Review of attachment 8933825 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > FYI I'm fine with just one env flag for the whole thing.
> > 
> > ::: accessible/windows/msaa/LazyInstantiator.cpp
> > @@ +302,5 @@
> > > +  nsresult rv;
> > > +  if (!PR_GetEnv("MOZ_DISABLE_ACCESSIBLE_BLOCKLIST")) {
> > > +    // Debugging option is not present, so check blocklist.
> > > +    nsAutoString leafName;
> > > +    rv = clientExe->GetLeafName(leafName);
> > 
> > is this retrieving the exe name or a directory name?
> 
> The exe name. That's what we want. Right?

yep!

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8933825 [details] [diff] [review]
Create accessible client blocklist and add TBNNotifier.exe. r?jimm

Review of attachment 8933825 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/windows/msaa/LazyInstantiator.cpp
@@ +239,5 @@
> +/**
> + * This is the blocklist for known "bad" remote clients that instantiate a11y.
> + */
> +static const char* gBlockedRemoteClients[] = {
> +  "TBNNotifier.exe" // Ask.com Toolbar, bug 1421018

All of our existing blocklists use lower case, you should follow that here as well.

Comment 9

[Eitan Isaacson [:eeejay]]

Attachment: Create accessible client blocklist and add TBNNotifier.exe.

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to Jim Mathies [:jimm] from comment #8)
> All of our existing blocklists use lower case, you should follow that here
> as well.

Done!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda85f961db4
Create accessible client blocklist and add TBNNotifier.exe. r=jimm

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/cda85f961db4