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)
Firefox for Android Graveyard
Settings and Preferences
Tracking
(firefox38 wontfix, firefox38.0.5 wontfix, firefox39 fixed, firefox40 fixed, fennec39+)
RESOLVED
FIXED
Firefox 40
People
(Reporter: bhearsum, Assigned: esawin)
References
Details
Attachments
(2 files, 1 obsolete file)
19.48 KB,
text/plain
|
Details | |
975 bytes,
patch
|
esawin
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
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.
Comment 3•9 years ago
|
||
(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?
comment #4
Flags: needinfo?(bhearsum)
Reporter | ||
Comment 6•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
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)
Reporter | ||
Comment 8•9 years ago
|
||
(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)
Reporter | ||
Comment 9•9 years ago
|
||
(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).
Assignee | ||
Comment 10•9 years ago
|
||
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
Comment 11•9 years ago
|
||
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
Updated•9 years ago
|
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
Assignee | ||
Comment 12•9 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c874e696120b
Attachment #8599875 -
Flags: review?(snorp)
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+
Assignee | ||
Comment 15•9 years ago
|
||
Replaced Equals with EqualsLiteral.
Attachment #8599875 -
Attachment is obsolete: true
Attachment #8599933 -
Flags: review+
Updated•9 years ago
|
tracking-fennec: ? → 40+
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3b3b8835cb2e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Assignee | ||
Updated•9 years ago
|
tracking-fennec: 40+ → ?
Assignee | ||
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
Comment 21•9 years ago
|
||
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+
Updated•9 years ago
|
tracking-fennec: ? → 39+
Comment 23•9 years ago
|
||
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;`
Comment 24•9 years ago
|
||
(In reply to Danil Ivanov from comment #23) > After this changes custom protocols urls stop working this Filed Bug 1170020. Thanks for the report!
Updated•5 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•