1125231 - some avast update requests have bad locale section in them

Status: RESOLVED INCOMPLETE

(Plugins Graveyard :: Avast AV, defect)

Description

[[e-mail] (:bhearsum)]

While analysing aus3 traffic after the switch to Balrog, Nick came across a bunch of requests like:
/update/3/Firefox/35.0/20150108202552/WINNT_x86-msvc/x86 de/release/Windows_NT%206.1.7601%20%28x64%29/default/default/update.xml?force=1&avast=1

Note the "x86 de", which should be just plain "de". Users in this situation are not getting any update offered right now. Seems like another case where avast's update hijacking is doing the wrong thing.

Over a 6h period yesterday there were 325,00 requests like this.

Comment 1

[[e-mail] (:bhearsum)]

It's worth noting that even though we're getting over a million requests per day like this, not all of them represent a stuck user. Lots of them are coming from 35.0, so the users are still able to update somehow.

Comment 2

[[e-mail] (:bhearsum)]

Interestingly, we get _some_ avast requests to Balrog, but none of them have the busted "x86 ab-CD":
[[e-mail]1 aus4.mozilla.org]$ grep avast access_2015-01-23-14 | wc -l
5290
[[e-mail]1 aus4.mozilla.org]$ grep avast access_2015-01-23-14 | grep "/x86 " | wc -l
0

It makes me wonder if the version of Avast that prepends "x86 " to the channel also queries AUS different. Perhaps it's coming through aus2.mozilla.org? Or somehow requesting via IP address instead of name?

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> Interestingly, we get _some_ avast requests to Balrog, but none of them have
> the busted "x86 ab-CD":
> [[e-mail]1 aus4.mozilla.org]$ grep avast
> access_2015-01-23-14 | wc -l
> 5290
> [[e-mail]1 aus4.mozilla.org]$ grep avast
> access_2015-01-23-14 | grep "/x86 " | wc -l
> 0
> 
> It makes me wonder if the version of Avast that prepends "x86 " to the
> channel also queries AUS different. Perhaps it's coming through
> aus2.mozilla.org? Or somehow requesting via IP address instead of name?

Oops, this was meant for bug 1125219. Ignore it here!

Comment 4

[Robert Strong (they/them - no direct email)]

I sent an email regarding this bug to our contact at Avast.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: avast mateys

I don't particularly like needing to work around issues like this, but dealing with it is better for our users than not, so we should probably take this.

Comment 6

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8562921 [details] [diff] [review]
avast mateys

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

I'm not mad about enabling this behaving from Avast, but I'd rather have users updated than not.

::: auslib/test/web/test_client.py
@@ +249,5 @@
> +        # to the locale. We need to make sure we handle this case correctly
> +        # so that these people can keep up to date.
> +        ret = self.client.get('/update/4/b/1.0/1/p/x86 l/a/a/a/a/1/update.xml')
> +        self.assertEqual(ret.status_code, 200)
> +        self.assertEqual(ret.mimetype, 'text/xml')

An alternative would be to make a second request, without the 'x86 ', and 
  self.assertEqual(ret.data, ret2.data)

Slightly preferable, in the sense it verifies the result we want without getting bogged down in the content of the response.

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/1b5928abe461e6547d799e67810a621e4a9d58c7
bug 1125231: handle avast update requests that mangle locale. r=nthomas

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 8562921 [details] [diff] [review]
avast mateys

(In reply to Nick Thomas [:nthomas] from comment #6)
> Comment on attachment 8562921 [details] [diff] [review]
> avast mateys
> 
> Review of attachment 8562921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not mad about enabling this behaving from Avast, but I'd rather have
> users updated than not.
> 
> ::: auslib/test/web/test_client.py
> @@ +249,5 @@
> > +        # to the locale. We need to make sure we handle this case correctly
> > +        # so that these people can keep up to date.
> > +        ret = self.client.get('/update/4/b/1.0/1/p/x86 l/a/a/a/a/1/update.xml')
> > +        self.assertEqual(ret.status_code, 200)
> > +        self.assertEqual(ret.mimetype, 'text/xml')
> 
> An alternative would be to make a second request, without the 'x86 ', and 
>   self.assertEqual(ret.data, ret2.data)
> 
> Slightly preferable, in the sense it verifies the result we want without
> getting bogged down in the content of the response.

Hmm, good idea. I made that change and pushed https://github.com/mozilla/balrog/commit/1b5928abe461e6547d799e67810a621e4a9d58c7

Comment 9

[[e-mail] (:bhearsum)]

Attachment: fix xh locale

I caught this in dev - turns out that lstrip takes a list of chars, not a string!

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8563640 [details] [diff] [review]
fix xh locale

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

Good catch!

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 8563640 [details] [diff] [review]
fix xh locale

Landed this, thanks for the quick review!

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/7f6bacd1b52f1f5dd87126339991edd711a42e35
bug 1125231: fix bug in avast workaround that caused xh locale to be rewritten as xh. r=nthomas

Comment 13

[[e-mail] (:bhearsum)]

The update server is handling this now:
https://aus4.mozilla.org/update/3/Firefox/35.0/20150108202552/WINNT_x86-msvc/x86%20de/release/Windows_NT%206.1.7601%20%28x64%29/default/default/update.xml?force=1&avast=1
https://aus4.mozilla.org/update/3/Firefox/35.0/20150108202552/WINNT_x86-msvc/de/release/Windows_NT%206.1.7601%20%28x64%29/default/default/update.xml?force=1&avast=1

^ Both of those serve the same update now.

Leaving open for communication with Avast.

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

There's a small residual problem for requests like ?force=1?avast=1, ie bogus query string. Roughly 1 per/min.

Comment 15

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #14)
> There's a small residual problem for requests like ?force=1?avast=1, ie
> bogus query string. Roughly 1 per/min.

The traceback is in our code, so it's probably pretty easy to deal with...I filed bug 1141513 to keep things clean.

Comment 16

[Robert Strong (they/them - no direct email)]

Is this still an issue?

Comment 17

[[e-mail] (:bhearsum)]

(In reply to Robert Strong - out until 5/6 [:rstrong] (use needinfo to contact me) from comment #16)
> Is this still an issue?

We're serving responses to these bad requests now, it's not clear whether or not Avast has fixed the issue on their side, though. I still see over 100,000 requests of this type over a 24h period, so I suspect not. I dunno if we care to keep this bug open for that though.

Comment 18

[Benjamin Smedberg]

Closing old bugs in the Plugins component. We aren't going to track issues in 3rd-party plugins in the Mozilla bug tracker. In addition, support for NPAPI plugins will be removed at the end of this year; for more details see the post at https://blog.mozilla.org/futurereleases/2015/10/08/npapi-plugins-in-firefox/

If there is a serious bug in Firefox, it needs to be filed in the "Core" product, "Plug-Ins" component.