Closed Bug 1158131 Opened 9 years ago Closed 9 years ago

Retrieving update URL pref from Gecko causes pref URL to be dereferenced as a complex pref, resulting in an HTTP request

Categories

(Firefox for Android Graveyard :: Settings and Preferences, defect)

defect
Not set
normal

Tracking

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39 fixed, firefox40 fixed, fennec39+)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
fennec 39+ ---

People

(Reporter: bhearsum, Assigned: esawin)

References

Details

Attachments

(2 files, 1 obsolete file)

While looking at something unrelated, I found entries like this in the Apache logs of our update server:
 [24/Apr/2015:12:29:08 +0000] "GET /update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%MOZ_VERSION%/update.xml HTTP/1.1" 400 226 "-" "Mozilla/5.0 (Android; Mobile; rv:37.0) Gecko/37.0 Firefox/37.0"

If it was just a few requests I'd be tempted to say that someone copy and pasted the app.update.url pref to into the address bar to see what it did. But I'm counting over 40,000 requests like this on a single web head (out of 6) in a 24 hour period, which makes me pretty darn sure we've got browser in the wild sending this.

I don't see it for versions lower than 37.0, so it's probably a pretty recent thing. I also see it for versions up to 40.0 - so it's not just happening in builds distributed through Google Play.
I think Eugen is looking at this. Regression from Bug 792992?
Blocks: 792992
tracking-fennec: --- → ?
Component: Other → Core
Product: Release Engineering → Android Background Services
QA Contact: mshal
Interesting. I really don't see how this is possible.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #2)
> Interesting. I really don't see how this is possible.

Based on code like https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/updater/UpdateServiceHelper.java#123 it's hard to see where such a URL could come from.

Is it possible we're somehow seeing a JavaScript update ping as well as a Java update ping?
(In reply to Nick Alexander :nalexander from comment #3)
> Is it possible we're somehow seeing a JavaScript update ping as well as a
> Java update ping?

Unless all of those people flipped app.update.enabled, that doesn't look possible to me either.

Ben, can we figure out when these first started happening? By date, that is?
Unfortunately, I can only find a week's worth of logs :(. It was happening on April 16th, but I can't tell you when it first started. WebOps might have older logs, though I doubt it...
Flags: needinfo?(bhearsum)
bhearsum: can you determine if individual clients are sending /both/ well-formed and mal-formed requests, or does it appear that there's a bifurcation?  esawin suggested that he was seeing multiple requests, some bad, from an individual client.
Flags: needinfo?(bhearsum)
Attached file update groups
(In reply to Nick Alexander :nalexander from comment #7)
> bhearsum: can you determine if individual clients are sending /both/
> well-formed and mal-formed requests, or does it appear that there's a
> bifurcation?  esawin suggested that he was seeing multiple requests, some
> bad, from an individual client.

It's hard to say with certainty...the best I can do is try to correlate requests from the same IPs. As you can see, there's Desktop requests in same of the groups too, presumably because people are behind NAT or have tethered their laptop to their phone. The first and second groups are both Gecko 37, and don't show much.

The third group is Gecko 39 and 40 and has _tons_ of pings. Again, it's hard to be certain of anything but this group _appears_ not to be pinging twice from the same browser. I see that the client with the user agent "Mozilla/5.0 (Android; Tablet; rv:40.0) Gecko/40.0 Firefox/40.0" is pinging once with unsubstituted values, and once for the GMP product - but there aren't any pings from it with the "Fennec" product. All of those ones in these group look like a different device.

I'm heading out on PTO very shortly, but if you want to do additional analysis someone might be able to hook you up with direct access to the logs.
Flags: needinfo?(bhearsum)
(In reply to Ben Hearsum [:bhearsum] from comment #8)
> Created attachment 8597441 [details]
> update groups
> 
> (In reply to Nick Alexander :nalexander from comment #7)
> > bhearsum: can you determine if individual clients are sending /both/
> > well-formed and mal-formed requests, or does it appear that there's a
> > bifurcation?  esawin suggested that he was seeing multiple requests, some
> > bad, from an individual client.
> 
> It's hard to say with certainty...the best I can do is try to correlate
> requests from the same IPs. As you can see, there's Desktop requests in same
> of the groups too, presumably because people are behind NAT or have tethered
> their laptop to their phone. The first and second groups are both Gecko 37,
> and don't show much.
> 
> The third group is Gecko 39 and 40 and has _tons_ of pings. Again, it's hard
> to be certain of anything but this group _appears_ not to be pinging twice
> from the same browser. I see that the client with the user agent
> "Mozilla/5.0 (Android; Tablet; rv:40.0) Gecko/40.0 Firefox/40.0" is pinging
> once with unsubstituted values, and once for the GMP product - but there
> aren't any pings from it with the "Fennec" product. All of those ones in
> these group look like a different device.
> 
> I'm heading out on PTO very shortly, but if you want to do additional
> analysis someone might be able to hook you up with direct access to the logs.

Shyam - I'm not sure if this is possible or not, I'm cc'ing you as a heads up just in case though. The logs in question are access logs on the aus4 web heads (or really, any aus4 logs with the request information should be fine. eg, if there's zeus logs or something).
This issue is reproducible on all versions since the introduction of the new pref in bug 792992.
It is a general issue with how we handle pref values containing external URLs.
More specifically the code path [1] through [4] is causing the issue by opening a connection to the pref value URL.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1380
[2] https://dxr.mozilla.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp#241
[3] https://dxr.mozilla.org/mozilla-central/source/intl/strres/nsStringBundle.cpp#209
[4] https://dxr.mozilla.org/mozilla-central/source/intl/strres/nsStringBundle.cpp#73
Two proposed fixes:

* Add two explicit paths for retrieving prefs known to be simple or localized strings.
* In the 'smart' case, check for sane schemes that apply for localized strings (in this case it should be resource:, I think).
Assignee: nobody → esawin
Status: NEW → ASSIGNED
Component: Core → Settings and Preferences
Product: Android Background Services → Firefox for Android
Summary: tons of update requests from Android with unsubstituted paths → Retrieving update URL pref from Gecko causes pref URL to be dereferenced as a complex pref, resulting in an HTTP request
Version: unspecified → Trunk
Comment on attachment 8599875 [details] [diff] [review]
0001-Bug-1158131-Add-local-resource-whitelisting-for-stri.patch

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

::: intl/strres/nsStringBundle.cpp
@@ +70,5 @@
>    if (NS_FAILED(rv)) return rv;
>  
> +  // whitelist check for local schemes
> +  nsCString scheme;
> +  uri->GetScheme(scheme);

You can use uri->SchemeIs() instead of getting the scheme and comparing yourself.

@@ +71,5 @@
>  
> +  // whitelist check for local schemes
> +  nsCString scheme;
> +  uri->GetScheme(scheme);
> +  if (!scheme.Equals("chrome") && !scheme.Equals("jar") &&

I think it would be a lot nicer if we just had a static array with these, so you can more easily add another one.
Attachment #8599875 - Flags: review?(snorp) → review-
Comment on attachment 8599875 [details] [diff] [review]
0001-Bug-1158131-Add-local-resource-whitelisting-for-stri.patch

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

Eugen convinced me this way is fine.
Attachment #8599875 - Flags: review- → review+
Replaced Equals with EqualsLiteral.
Attachment #8599875 - Attachment is obsolete: true
Attachment #8599933 - Flags: review+
Blocks: 1160204
tracking-fennec: ? → 40+
https://hg.mozilla.org/mozilla-central/rev/3b3b8835cb2e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Eugen, could you request uplift for this?
Flags: needinfo?(esawin)
tracking-fennec: 40+ → ?
This is an old issue, but we just recently regressed on it with the introduction of the new pref in bug 792992, which is tracking 37.
Comment on attachment 8599933 [details] [diff] [review]
0001-Bug-1158131-Add-local-resource-whitelisting-for-stri.patch

Approval Request Comment
[Feature/regressing bug #]: bug 792992 introduced a new pref with an external (HTTPS) value which triggered the regression, but the actual issue is older 
[User impact if declined]: reading prefs can trigger network connections, possibly increasing loading times and having other side effects
[Describe test coverage new/current, TreeHerder]: there is some coverage through implicit pref/string bundle loading in feature tests and it has been on Nightly for a few days 
[Risks and why]: moderate, because it's a core functionality that is used by most modules
[String/UUID change made/needed]: none
Flags: needinfo?(esawin)
Attachment #8599933 - Flags: approval-mozilla-aurora?
Comment on attachment 8599933 [details] [diff] [review]
0001-Bug-1158131-Add-local-resource-whitelisting-for-stri.patch

Approved for uplift to 39 (beta) since this sounds like a recent regression and a possible performance issue.
Attachment #8599933 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
tracking-fennec: ? → 39+
After this changes custom protocols urls stop working this `Services.strings.createBundle`. Like 'xb' in the YandexBar.
`Services.strings.createBundle('xb://...')` throws NS_ERROR_ABORT in the Firefox 39b.
We did workaround in the YandexBar now, but confused by speed of these changes.

Maybe good also to check protocol flags? Something like
`Services.io.getProtocolFlags(scheme) & Ci.nsIProtocolHandler.URI_IS_UI_RESOURCE;`
or
`Services.io.getProtocolFlags(scheme) & Ci.nsIProtocolHandler.URI_IS_LOCAL_FILE;`
Depends on: 1170020
(In reply to Danil Ivanov from comment #23)
> After this changes custom protocols urls stop working this

Filed Bug 1170020. Thanks for the report!
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.