The sync client is sending the cookie value for .mozilla.org/com on all sync requests

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
8 years ago
26 days ago

People

(Reporter: clyon, Assigned: ally)

Tracking

unspecified
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Looking at the sync logs from a few different sync clients, it seems that we shouldn't be sending the cookie value in all requests to the sync servers. 

I did talk with mconnor about this and suggested this: 

https://developer.mozilla.org/en/Creating_Sandboxed_HTTP_Connections

Updated

7 years ago
Group: client-services-security → mozilla-services-security
(Assignee)

Comment 1

7 years ago
Connor, is this still an issue and how should we be prioritizing it?
Almost certainly still an issue.  It's not a killer thing, but we really should fix it sooner rather than later.

I suspect our network wrapper makes this relatively easy to fix.  cc-ing gps for opinion.
Component: General → Firefox Sync: Backend
QA Contact: general → sync-backend
Who set the cookies in question? It seems like the sync server shouldn't set them in the first place if you don't want to send them?
They're domain cookies from visiting mozilla.com or other sites that set domain cookies.
(Assignee)

Updated

6 years ago
Assignee: nobody → ally
(Assignee)

Comment 5

6 years ago
Created attachment 659905 [details]
v 0 + test
Attachment #659905 - Flags: review?(gps)
Attachment #659905 - Flags: feedback?(cbiesinger)
Comment on attachment 659905 [details]
v 0 + test

+  var server = httpd_setup({"/original": original});

original should probably get a different name now, maybe just "handler"

+  var uri = CommonUtils.makeURI("http://original.com/");
+  cookieSer.setCookieString(uri, null, "test=test; path=/; domain=original.com;", null);

This needs to use localhost, not original.com. I'm not sure how domain cookies work for localhost, but it should be safe to make this a host cookie and just remove the domain=original.com; part.

@@ -143,6 +143,8 @@ AsyncResource.prototype = {

You don't have a test for AsyncResource. Looks like you should add one to http://mxr.mozilla.org/mozilla-central/source/services/sync/tests/unit/test_resource_async.js ?
Attachment #659905 - Flags: feedback?(cbiesinger) → feedback-
(I'm unclear on what RESTRequest vs AsyncResource is used for, but I trust gps will understand that part of the patch)
(Assignee)

Comment 8

6 years ago
Comment on attachment 659905 [details]
v 0 + test

no point in pestering gps for review until feedback is addressed
Attachment #659905 - Flags: review?(gps)
(Assignee)

Comment 9

6 years ago
Created attachment 660242 [details] [diff] [review]
v1 + tests

biesi's feedback applied, resource test added.
Attachment #659905 - Attachment is obsolete: true
Attachment #660242 - Flags: feedback?(cbiesinger)
Comment on attachment 660242 [details] [diff] [review]
v1 + tests

+++ b/js/src/jsinterp.cpp
+    //DebugOnly<uint32_t> blockDepth = regs.fp()->blockChain().stackDepth();

I guess you don't want this change in here :)

looks good otherwise! f+
Attachment #660242 - Flags: feedback?(cbiesinger) → feedback+
(Assignee)

Updated

6 years ago
Attachment #660242 - Flags: review?(gps)
(Assignee)

Comment 11

6 years ago
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #10)
> Comment on attachment 660242 [details] [diff] [review]
> v1 + tests
> 
> +++ b/js/src/jsinterp.cpp
> +    //DebugOnly<uint32_t> blockDepth = regs.fp()->blockChain().stackDepth();
> 
> I guess you don't want this change in here :)

...nope. Its a hack to get m-c with clang to compile properly. :)
Comment on attachment 660242 [details] [diff] [review]
v1 + tests

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

Nit: use let everywhere. Always. The only place var is allowed is inside content. And, even then it is preferred to have the compartment use a modern version of JS that supports let.

f+ because I want to see a full/as-is patch before r+.

::: js/src/jsinterp.cpp
@@ +3650,4 @@
>  BEGIN_CASE(JSOP_LEAVEFORLETIN)
>  BEGIN_CASE(JSOP_LEAVEBLOCKEXPR)
>  {
> +    //DebugOnly<uint32_t> blockDepth = regs.fp()->blockChain().stackDepth();

We need to get you using the right compiler...

::: services/common/rest.js
@@ +117,4 @@
>    response: null,
>  
>    /**
> +   * nsIRequest load flags. Don't do any caching by default. Don't  send user

Nit: whitespace

@@ +122,2 @@
>     */
> +  loadFlags: Ci.nsIRequest.LOAD_BYPASS_CACHE | Ci.nsIRequest.INHIBIT_CACHING | Ci.nsIRequest.LOAD_ANONYMOUS,

RESTRequest is shared by multiple modules. Adjusting the default load flags could have undesired consequences.

It is arguably silly that we set these flags by default. But, given that RESTRequest is intended for and mostly used by automated services (which typically don't use cookies and are authenticated therefore don't need a cache), I think I'll let this slide.

::: services/common/tests/unit/test_restrequest.js
@@ +777,5 @@
> +  }
> +  var server = httpd_setup({"/test": handler});
> +
> +  var cookieSer = Components.classes["@mozilla.org/cookieService;1"]
> +    .getService(Components.interfaces.nsICookieService);

Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService)

If you need to wrap:

Cc["@mozilla.org/..."]
  .getService("XXX"); // . is aligned with [
Attachment #660242 - Flags: review?(gps) → feedback+
(Assignee)

Comment 13

6 years ago
Created attachment 660584 [details] [diff] [review]
v2

While I followed directions, I'm not sure if the --no-prefix really formatted the patch in a manner that is acceptable for hg. I think the internet might be misleading me there.
Attachment #660584 - Flags: review?(gps)
Comment on attachment 660584 [details] [diff] [review]
v2

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

Please land in inbound.

::: services/common/tests/unit/test_restrequest.js
@@ +778,5 @@
> +  let server = httpd_setup({"/test": handler});
> +
> +  let cookieSer = Components.classes["@mozilla.org/cookieService;1"]
> +                                    .getService(Components.interfaces
> +                                    .nsICookieService);

If you used Cc and Ci instead of Components.classes and Components.interfaces, respectively, you wouldn't need this crazy formatting.
Attachment #660584 - Flags: review?(gps) → review+

Updated

6 years ago
Attachment #660242 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/541a789f3912
Group: mozilla-services-security → mozilla-corporation-confidential
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Ally: bsmith mentioned that there might be some interaction (read: it doesn't work) between nsIRequest.LOAD_ANONYMOUS and authenticating proxies. Worth investigating?
btw why is this a mozilla corporation bug as opposed to a security bug?
(In reply to Richard Newman [:rnewman] from comment #17)
> Ally: bsmith mentioned that there might be some interaction (read: it
> doesn't work) between nsIRequest.LOAD_ANONYMOUS and authenticating proxies.
> Worth investigating?

Bug 701019. Looks like it was fixed.
What's the plan for opening this bug to the public? Any reason not to do it now?
I have no problem with opening this bug to the public.
Group: mozilla-corporation-confidential

Updated

6 years ago
Depends on: 836602
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.