1210302 - (CVE-2015-7193) CORS does a simple instead of preflighted request for POST with non-standard Content-Type header

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Shinto K Anto]

Attachment: Details.docx

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

Created and HTML page which will fire a cross domain POST request. opened the file in firefox and clicked on the button. It sends a cross domain request without any security check. (More details are present in the attached doc)


Actual results:

Firefox sends the request directly to the domain without security check. Sends the cross origin POST request from the file hosted locally to a totally different domain. 



Expected results:

Expected result is, When a cross domain POST happens browser should ideally send the OPTION request first to the domain to check weather it accept the POST requests from this particular domain. IN the attachment I have also given a screenshot of testing the same thing in a chrome browser.

Comment 1

[:Gijs (he/him)]

Attachment: Reporter's testcase

Comment 2

[:Gijs (he/him)]

Your testcase doesn't actually demonstrate SOP bypass, because you're not accessing any data from the third-party domain. I can do a form-based POST to wherever I like, see e.g. http://stackoverflow.com/questions/11423682/cross-domain-form-posting and its answers.

The difference with Chrome is that they preflight your request, and we don't - but we're still denying you access to the data because the response doesn't have the required CORS headers (see simple vs. preflighted requests in https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS ).

I'm not sure there's a bug here - you're setting this header:

        xhr.setRequestHeader("Content-Type", "application/json, text/plain, */*, charset=utf-8");

which is bogus. I'm having trouble determining from the spec what we should be doing here, but it seems like "send the header as-is and don't care" is unlikely to be correct (and if it is, I would say we should change the spec).

Anne, can you help and/or redirect the request for help to someone who knows more about this? :-)

(keeping this hidden for now while awaiting proper sec evaluation, but I do not believe this is currently demonstrating an actual security bug in Firefox - though this is assuming that using a simple instead of a preflighted request here is not fatal to security, which is an unproven assumption on my part.)

Comment 3

[Anne (:annevk)]

Gijs, not doing a preflight here does actually violate SOP. SOP cares about both read and write protection.

Ehsan, is this similar fallout to bug 1207556?

Comment 4

[:Gijs (he/him)]

(In reply to Anne (:annevk) from comment #3)
> Gijs, not doing a preflight here does actually violate SOP. SOP cares about
> both read and write protection.

Doesn't that depend on the content-type mimetype and/or how that is parsed? Without that header set (or with it set to one of the 'OK' mimetypes like multipart-form, text/plain or urlencoded), this should be a simple request according to the spec, right?

Comment 5

[Shinto K Anto]

Let me give a small background here. 

My developer was telling he is validating "Accept" and "Content-Type" header to prevent CSRF attacks. He was right be cause he was testing it in Chrome. in chrome. It wont be possible to set it because of the security in browser. When I checked the same this in Firefox the prevention was not working. browser sends the request to domain even if there is a costume header value.

Comment 6

[Anne (:annevk)]

Gijs, yes (to both).

Comment 7

[(no longer active)]

Let me investigate to see what's going on here.

Comment 8

[(no longer active)]

This is completely unrelated to bug 1207556.

According to the spec, we should parse the Content-Type header according to <https://tools.ietf.org/html/rfc7231#section-3.1.1.1>, but the code we use to do that is this function: <http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsURLHelper.cpp#960>.  As you can see, in that function, we're trying to parse more than one media-type, which violates the spec.  Therefore, in the first iteration of the loop, we parse "application/json", in the second iteration we parse "text/plain" which gets overwritten into aContentType, and in the third iteration we parse "*/*" which gets ignored according to <http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsURLHelper.cpp#892>, and in the fourth iteration we parse "charset=utf-8" which gets ignored because it doesn't have a slash in it: <http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsURLHelper.cpp#901>.  Therefore we return text/plain from that function (the value overwritten on the second iteration) and we think we have a safe Content-Type header, and we lose.

The moral of the story is that the code written to parse the Content-Type header for *responses* (which need to deal with the junk that web servers serve us) should not be the same as the code that parses the same header for a *request*.

Before I write a patch, Patrick, do you mind verifying my sanity above please?  Thanks!

Comment 9

[(no longer active)]

(Not that this is very old code and affects all branches.)

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #8)
> 
> Content-Type header, and we lose.
> 
> The moral of the story is that the code written to parse the Content-Type
> header for *responses* (which need to deal with the junk that web servers
> serve us) should not be the same as the code that parses the same header for
> a *request*.

right.. better yet, perhaps we should parse the request content-type as well (that's still potentially full of junk, right - content js isn't more constrained than the server) and then normalize the header to match the parsing.

Comment 11

[Anne (:annevk)]

I'm not sure we want to normalize request headers that can be set by developers. It would be against the specification and would limit flexibility unnecessarily. And I say unnecessarily because if this had a preflight all would be fine.

Comment 12

[(no longer active)]

Yeah, my point was that we should be very strict when parsing the request Content-Type header as that is controlled by the author and we make security decisions based on it, but loose as we currently are when parsing the response Content-Type header.

Comment 13

[Patrick McManus [:mcmanus]]

fwiw (and that's not much) if you look at the comments in the parser, you'll see that its not really trying to be robust to nonsensical junk in particular (like in the STR here).. the motivation is the server is sending the same type both with and without a charset so as to be compatible with clients that can't parse charsets.

Comment 14

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d00a50c1a47f

Comment 15

[(no longer active)]

Attachment: Part 1: Rename nsINetUtil.parseContentType() to parseResponseContentType()

Comment 16

[(no longer active)]

Attachment: Part 2: Rename NS_ParseContentType to NS_ParseResponseContentType

Comment 17

[(no longer active)]

Attachment: Part 3: Add a NS_ParseRequestContentType API

Comment 18

[(no longer active)]

Attachment: Part 4: Add automated tests

The test shows the places where this patch fixes our existing behavior.  The manifest part is benign, the next are all SOP violations.

Comment 19

[Patrick McManus [:mcmanus]]

its interesting that 2616 and 7231 both limit content-type to one media type, which is at odds with the legacy comment in nsurlhelper which uses the generic header value syntax

969     // Augmented BNF (from RFC 2616 section 3.7):
970     //
971     //   header-value = media-type *( LWS "," LWS media-type )

2616 does this in 14.17 - perhaps it was just misread in the original implementation or maybe this was needed for interop. hard to tell.

anyhow - other than forcing the needed preflight when the parse fails with the new rules, do you know of other side effects?

Comment 20

[(no longer active)]

Attachment: Part 3: Add a NS_ParseRequestContentType API

Adding a unit test for parseRequestContentType helped reveal a few bugs in the parsing code.  This patch fixes them.

Comment 21

[(no longer active)]

Attachment: Part 5: Add a unit test for parseRequestContentType

Comment 22

[(no longer active)]

(In reply to Patrick McManus [:mcmanus] from comment #19)
> its interesting that 2616 and 7231 both limit content-type to one media
> type, which is at odds with the legacy comment in nsurlhelper which uses the
> generic header value syntax
> 
> 969     // Augmented BNF (from RFC 2616 section 3.7):
> 970     //
> 971     //   header-value = media-type *( LWS "," LWS media-type )
> 
> 2616 does this in 14.17 - perhaps it was just misread in the original
> implementation or maybe this was needed for interop. hard to tell.

Yeah, that's what I meant in comment 8.  I was assuming that we parse the Content-Type header that way because of compat issues with broken web servers.

> anyhow - other than forcing the needed preflight when the parse fails with
> the new rules, do you know of other side effects?

No.  All of the places where we're dealing with a response Content-Type header still go through the existing parser.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8668660 [details] [diff] [review]
Part 4: Add automated tests

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

I didn't realize that we have copied the CORS tests into test_fetch_cors.js :( Would have been nice to generalize existing tests rather than copy them...

Obviously that's a topic for a different bug though.

Comment 24

[Patrick McManus [:mcmanus]]

Comment on attachment 8668692 [details] [diff] [review]
Part 3: Add a NS_ParseRequestContentType API

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

::: netwerk/base/nsINetUtil.idl
@@ +14,5 @@
>  [scriptable, uuid(ff0b3233-7ec5-4bf4-830f-6b2edaa53661)]
>  interface nsINetUtil : nsISupports
>  {
>    /**
> +   * Parse a content-type request header and return the content type

can we add to the documentation that this is really about Strict vs Lax rather than request/response given that the spec doesn't draw a difference..

Comment 25

[(no longer active)]

(In reply to Patrick McManus [:mcmanus] from comment #24)
> Comment on attachment 8668692 [details] [diff] [review]
> Part 3: Add a NS_ParseRequestContentType API
> 
> Review of attachment 8668692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsINetUtil.idl
> @@ +14,5 @@
> >  [scriptable, uuid(ff0b3233-7ec5-4bf4-830f-6b2edaa53661)]
> >  interface nsINetUtil : nsISupports
> >  {
> >    /**
> > +   * Parse a content-type request header and return the content type
> 
> can we add to the documentation that this is really about Strict vs Lax
> rather than request/response given that the spec doesn't draw a difference..

Sure, but since that gives away what these patches do (I have tried to make them look like refactoring!) I will write and submit a new patch to fix the documentation and will land it together with the tests when we ship this.

Comment 26

[(no longer active)]

So I found a few other mistakes in the parsing code that I have fixed and will submit shortly.  But this parsing code is really complicated and I'm sure I have not been able to find all of the bugs in it.  And even worse, once we fix this bug on ESR, people are going to notice and look at the code with more scrutiny, and find other bugs in it.  I'd do that in this bug, but that would be way too risky for backporting, I think.

I filed bug 1210973 for that.  Patrick, do you mind finding someone on the Necko team to take it please?  Thanks!

Comment 27

[(no longer active)]

Attachment: Part 3: Add a NS_ParseRequestContentType API

Patrick, do you mind reviewing the changes to nsURLHelper.cpp again please?  The rest hasn't changed.

Comment 28

[(no longer active)]

Attachment: Part 5: Add a unit test for parseRequestContentType

Some of these tests also changed.  Sorry for the extra round trip!

Comment 29

[(no longer active)]

Attachment: Part 6: Update the documentation of the methods in the IDL file

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8669150 [details] [diff] [review]
Part 3: Add a NS_ParseRequestContentType API

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

Someone else should review the nsURLHelper.cpp changes. I don't feel qualified to. r=me on the rest.

::: netwerk/base/nsURLHelper.cpp
@@ +904,5 @@
> +    if (type != typeEnd &&
> +        memchr(type, '/', typeEnd - type) != nullptr &&
> +        (!aStrict ||
> +          net_FindCharNotInSet(start + consumed, end, HTTP_LWS) == end) &&
> +        (aStrict || strncmp(type, "*/*", typeEnd - type) != 0)) {

Took me a while to figure out what the construct above actually does.

I personally think the following is more clear:

... &&
(aStrict ? (net_FindCharNotInSet(start + consumed, end, HTTP_LWS) == end) :
           (strncmp(type, "*/*", typeEnd - type) != 0))) {

Comment 31

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8020d9c60224

Comment 32

[Patrick McManus [:mcmanus]]

Comment on attachment 8669150 [details] [diff] [review]
Part 3: Add a NS_ParseRequestContentType API

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

::: netwerk/base/nsURLHelper.cpp
@@ +904,5 @@
> +    if (type != typeEnd &&
> +        memchr(type, '/', typeEnd - type) != nullptr &&
> +        (!aStrict ||
> +          net_FindCharNotInSet(start + consumed, end, HTTP_LWS) == end) &&
> +        (aStrict || strncmp(type, "*/*", typeEnd - type) != 0)) {

I also prefer jonas's ?: formulation.. thanks for sorting this out!

Comment 33

[(no longer active)]

Attachment: Part 3: Add a NS_ParseRequestContentType API

Addressed the review comment.

Comment 34

[(no longer active)]

Comment on attachment 8668656 [details] [diff] [review]
Part 1: Rename nsINetUtil.parseContentType() to parseResponseContentType()

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I have tried to frame the patches as refactoring, so hopefully it won't be 100% straightforward.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.  (I am requesting approval only for the first 3 patches in this series.)

Which older supported branches are affected by this flaw? All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  They should not be much different.

How likely is this patch to cause regressions; how much testing does it need? It changes the web developer facing behavior of XHR and fetch to make them more secure, so I think regressions will mostly happen in content that is trying to trick us into bypassing CORS preflights.  That being said, it would be useful to have some testing of this on trunk and less stable branches before backporting all the way back to ESR.

Comment 35

[Al Billings [:abillings - ex-MoCo]]

This needs a security rating before it can go in. I'm not sure I understand the implications here. Can you suggest a rating?

Comment 36

[(no longer active)]

This is an SOP violation.  https://wiki.mozilla.org/Security_Severity_Ratings is pretty unhelpful for classifying SOP violations.  The previous SOP violations that I have fixed were rated as sec-high so I'm marking it the same.  Please adjust the rating to the one we're supposed to use for SOP violations.

Comment 37

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8668656 [details] [diff] [review]
Part 1: Rename nsINetUtil.parseContentType() to parseResponseContentType()

Sec-approval+. We'll want this on branches too.

Comment 38

[(no longer active)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/be5c23120d65
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e0ad8b8818
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d47d56b107d4

Comment 39

[(no longer active)]

Comment on attachment 8668656 [details] [diff] [review]
Part 1: Rename nsINetUtil.parseContentType() to parseResponseContentType()

Approval Request Comment
[Feature/regressing bug #]: We've had this bug for a long time.
[User impact if declined]: SOP violation.
[Describe test coverage new/current, TreeHerder]: Has tests which I ran locally and on the try server.
[Risks and why]: This is a risky change.  It would be nice to let it bake on trunk for a while.
[String/UUID change made/needed]: This changes nsINetUtil's uuid.

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be5c23120d65
https://hg.mozilla.org/mozilla-central/rev/e6e0ad8b8818
https://hg.mozilla.org/mozilla-central/rev/d47d56b107d4
https://hg.mozilla.org/mozilla-central/rev/e2feb3a13f83

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Oops, https://hg.mozilla.org/mozilla-central/rev/e2feb3a13f83 was for a different bug.

Comment 42

[Kent James (:rkent)]

The method rename in nsINetUtil does not really seem strictly necessary to accomplish the goals of this bug, but is what is causing the need to roll the interface uuid. Could you avoid that, at least for esr38?

Comment 43

[(no longer active)]

(In reply to Kent James (:rkent) from comment #42)
> The method rename in nsINetUtil does not really seem strictly necessary to
> accomplish the goals of this bug

What makes you think that?!

> but is what is causing the need to roll
> the interface uuid. Could you avoid that, at least for esr38?

Why?

Comment 44

[Kent James (:rkent)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> (In reply to Kent James (:rkent) from comment #42)
> > The method rename in nsINetUtil does not really seem strictly necessary to
> > accomplish the goals of this bug
> 
> What makes you think that?!

Well you could have left the existing name and used it for the stricter check, for example. Or use it for its existing looser use, and only switch to the stricter check in Mozilla code where it matters. Changing the name when there was no need to change the calling sequence is not necessary. There are over 10 addons that use this, for example, that this change will break in esr38. I also thought that there was still strictly a need to preserve some binary compatibility due to b2g issues, and this is a severe breakage of binary compatibility for and esr release. (In the interest of full disclosure it also makes my life particularly difficult).

Has there been a formal policy change that there is no longer any concern with changing uuid or the actual calling sequence to idls in esr builds? At the very least this is going to need an addon compatibility note. That seems pretty severe if this is not really necessary to solve the underlying security problem.

> 
> > but is what is causing the need to roll
> > the interface uuid. Could you avoid that, at least for esr38?
> 
> Why?

Well, so that you 1) don't break addons and 2) don't violate the rolling stable uuid policy (assuming there still is one) for major releases.

Comment 45

[(no longer active)]

(In reply to Kent James (:rkent) from comment #44)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> > (In reply to Kent James (:rkent) from comment #42)
> > > The method rename in nsINetUtil does not really seem strictly necessary to
> > > accomplish the goals of this bug
> > 
> > What makes you think that?!
> 
> Well you could have left the existing name and used it for the stricter
> check, for example. Or use it for its existing looser use, and only switch
> to the stricter check in Mozilla code where it matters. Changing the name
> when there was no need to change the calling sequence is not necessary.

Have you read the third paragraph of comment 8?  The name is absolutely important, because of the reason explained there.  (Also see part 6 which hasn't been landed yet.)

> There are over 10 addons that use this, for example, that this change will
> break in esr38.

10 add-ons with binary components that use nsINetUtil?  If yes, it would be helpful if you provide the list of those add-ons.

> I also thought that there was still strictly a need to
> preserve some binary compatibility due to b2g issues, and this is a severe
> breakage of binary compatibility for and esr release.

I am not aware of such restrictions for nsINetUtil.

> (In the interest of
> full disclosure it also makes my life particularly difficult).

May I ask why?

> Has there been a formal policy change that there is no longer any concern
> with changing uuid or the actual calling sequence to idls in esr builds? At
> the very least this is going to need an addon compatibility note. That seems
> pretty severe if this is not really necessary to solve the underlying
> security problem.

Yes, we have dropped support for extensions with binary components.  (I think that has happened after 38...)

> > > but is what is causing the need to roll
> > > the interface uuid. Could you avoid that, at least for esr38?
> > 
> > Why?
> 
> Well, so that you 1) don't break addons and 2) don't violate the rolling
> stable uuid policy (assuming there still is one) for major releases.

The decision as to whether or not take this patch for esr is owned by the release drivers, and I have been very explicit about the uuid change in comment 39, so they have all of the information they need to make a decision.  If they ask me, I can provide a different version of the patch that doesn't change the uuid of nsINetUtil but I'm not inclined to do that before they have asked me to.

Comment 46

[Kent James (:rkent)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #45)
> (In reply to Kent James (:rkent) from comment #44)
> 
> > There are over 10 addons that use this, for example, that this change will
> > break in esr38.
> 
> 10 add-ons with binary components that use nsINetUtil?  If yes, it would be
> helpful if you provide the list of those add-ons.

These are not binary addons. For them, it is not the uuid roll that causes the problem, it is the name change. A typical example (and I know nothing about this or most of the other affected addons):

/410914/resources/r2d2b2g/data/linux64/b2g/modules/AppsUtils.jsm
    line 343 -- let contentType = NetUtil.parseContentType(aContentType, charset, hadCharset);

> 
> > (In the interest of
> > full disclosure it also makes my life particularly difficult).
> 
> May I ask why?

Well I still maintain a binary addon, for Thunderbird. I'm not asking you to be concerned with my needs, I only disclosed so you understand that I have a motivation to oppose this.

> 
> > Has there been a formal policy change that there is no longer any concern
> > with changing uuid or the actual calling sequence to idls in esr builds? At
> > the very least this is going to need an addon compatibility note. That seems
> > pretty severe if this is not really necessary to solve the underlying
> > security problem.
> 
> Yes, we have dropped support for extensions with binary components.  (I
> think that has happened after 38...)

Does "dropping support for binary addons" also mean that there is no concern about rolling uuid values anymore in major releases, like the gecko38 branch?

>
> The decision as to whether or not take this patch for esr is owned by the
> release drivers, and I have been very explicit about the uuid change in
> comment 39, so they have all of the information they need to make a
> decision.  If they ask me, I can provide a different version of the patch
> that doesn't change the uuid of nsINetUtil but I'm not inclined to do that
> before they have asked me to.

Right and I am hoping for my own sake that the uuid and name change in esr38 is not accepted, but I realize and disclose I have my own narrow perspective.

Comment 47

[Patrick McManus [:mcmanus]]

ehsan, what do you think of adding back parseContentType to the idl, marking it deprecated, and just having the doc and implementation call parseResponseContentType for backwards compat? (we would keep the new functions and the uuid change, but things would be ok from a naming perspsective.)

Comment 48

[(no longer active)]

(In reply to Kent James (:rkent) from comment #46)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #45)
> > (In reply to Kent James (:rkent) from comment #44)
> > 
> > > There are over 10 addons that use this, for example, that this change will
> > > break in esr38.
> > 
> > 10 add-ons with binary components that use nsINetUtil?  If yes, it would be
> > helpful if you provide the list of those add-ons.
> 
> These are not binary addons. For them, it is not the uuid roll that causes
> the problem, it is the name change. A typical example (and I know nothing
> about this or most of the other affected addons):
> 
> /410914/resources/r2d2b2g/data/linux64/b2g/modules/AppsUtils.jsm
>     line 343 -- let contentType = NetUtil.parseContentType(aContentType,
> charset, hadCharset);

That extension is dead.

> > 
> > > (In the interest of
> > > full disclosure it also makes my life particularly difficult).
> > 
> > May I ask why?
> 
> Well I still maintain a binary addon, for Thunderbird. I'm not asking you to
> be concerned with my needs, I only disclosed so you understand that I have a
> motivation to oppose this.

Thanks for the info.  Like I said, the decision doesn't belong to me.

> > > Has there been a formal policy change that there is no longer any concern
> > > with changing uuid or the actual calling sequence to idls in esr builds? At
> > > the very least this is going to need an addon compatibility note. That seems
> > > pretty severe if this is not really necessary to solve the underlying
> > > security problem.
> > 
> > Yes, we have dropped support for extensions with binary components.  (I
> > think that has happened after 38...)
> 
> Does "dropping support for binary addons" also mean that there is no concern
> about rolling uuid values anymore in major releases, like the gecko38 branch?

As I said, I think that happened after 38, so it may not apply there, but uuid changes only affect binary add-ons and as such there is no point to be concerned about them going forward.  (This is *highly* off topic for this bug, BTW.)

Comment 49

[(no longer active)]

(In reply to Patrick McManus [:mcmanus] from comment #47)
> ehsan, what do you think of adding back parseContentType to the idl, marking
> it deprecated, and just having the doc and implementation call
> parseResponseContentType for backwards compat? (we would keep the new
> functions and the uuid change, but things would be ok from a naming
> perspsective.)

We can do that for ESR if the release manager asks me to, but I am opposed to doing that on trunk.  The distinction is very subtle and the naming does help in making it more clear which function should be used for what.  I think the security of our users should override the convenience of add-on developers here.  (See <https://mxr.mozilla.org/addons/search?string=parseContentType&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons>, more than half of those are dead b2g add-ons.)  This *really* doesn't matter that much, but adding the addon-compat keyword nonetheless.

Comment 50

[Kent James (:rkent)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #48)
> (In reply to Kent James (:rkent) from comment #46)
> > 
> > Does "dropping support for binary addons" also mean that there is no concern
> > about rolling uuid values anymore in major releases, like the gecko38 branch?
> 
> As I said, I think that happened after 38, so it may not apply there, but
> uuid changes only affect binary add-ons and as such there is no point to be
> concerned about them going forward.  (This is *highly* off topic for this
> bug, BTW.)

It is on topic because: you want to roll a uuid in esr38 in THIS BUG. You justify it (in comment 45 in THIS BUG) by stating that binary extensions are no longer permitted post-38 (which seems to argue against the uuid change in esr38 BTW). I ask if there is a connection between the two, you claim that is off topic. For the changes in THIS BUG, I still have the question: did the change in support in Firefox for binary extensions remove the expectation that uuid would not be changed in major versions without really strong reasons (which level this change does not meet, as there are good alternatives)? I take it from you answers that there is an assumption that there is no plan to maintain uuid stability going forward for Firefox/Gecko major releases. Has that point been made explicit somewhere?

I would still claim:

1) For Firefox, support for binary extensions was dropped post-esr38, so rolling uuid should still be highly discouraged in esr38.

2) Thunderbird has more users on esr38 than Firefox since this is our major release branch, and Thunderbird has not dropped support for binary extensions. Now we can land our own version of this patch if necessary in our release branch, but I've gotten flak in the past when I have proposed to modify security patches for Thunderbird, so I would highly prefer if the agreed esr38 patch would meet Thunderbird's requirements as well as Firefox's. But I will do my own version if necessary (which will involve moving the new methods to a separate interface, and QIing to that interface in the places where it is used, plus maintaining backwards compatibility with the name as proposed also in comment 47.

3) Rolling a uuid is typically a sign that a breaking change is occurring, and we should at least start asking questions about whether that is going to cause compatibility problems. I pointed out that there are known js extensions using the changed methods (one being Enigmail, which is Thunderbird's second most popular addon) which is clear evidence that there are compatibility issues associated with this.

While "the security of our users should override the convenience of add-on developers" is certainly true, that is not really what this is about. This is about us not being willing to put in a little extra effort to make a security change work well with addons. There are secure solutions available, that require a little extra work, that do not break addon.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Jorge, as Ehsan said that this patch will impact nsINetUtil's uuid. Do you think we can take this in beta?
Thanks

Comment 52

[(no longer active)]

(In reply to Kent James (:rkent) from comment #50)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #48)
> > (In reply to Kent James (:rkent) from comment #46)
> > > 
> > > Does "dropping support for binary addons" also mean that there is no concern
> > > about rolling uuid values anymore in major releases, like the gecko38 branch?
> > 
> > As I said, I think that happened after 38, so it may not apply there, but
> > uuid changes only affect binary add-ons and as such there is no point to be
> > concerned about them going forward.  (This is *highly* off topic for this
> > bug, BTW.)
> 
> It is on topic because: you want to roll a uuid in esr38 in THIS BUG.

Can you please stop accusing me and go back and read the last paragraph of comment 45 again?

> I take it from you answers that there is an assumption that
> there is no plan to maintain uuid stability going forward for Firefox/Gecko
> major releases. Has that point been made explicit somewhere?

I don't know.  What I do know is that uuid stability only matters for compatibility with binary extensions, and we're dropping support for those types of extensions so whether or not we change our uuids, nobody will notice.

> I would still claim:
> [snipped]

Thanks.  Our release management team is well aware of all of this and are well euipped to make the right decision.  I defer to them.

> While "the security of our users should override the convenience of add-on
> developers" is certainly true, that is not really what this is about. This
> is about us not being willing to put in a little extra effort to make a
> security change work well with addons. There are secure solutions available,
> that require a little extra work, that do not break addon.

You're quoting me out of context, so I really don't want to get into an argument with you over this.  Please go and read comment 49 again, and this time, pay attention to the part that you chose to not quote.

Comment 53

[(no longer active)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #51)
> Jorge, as Ehsan said that this patch will impact nsINetUtil's uuid. Do you
> think we can take this in beta?
> Thanks

Note that 42 doesn't support binary extensions any more, so what matters for beta more is the change in the name of the function, not the change in the uuid itself.

Comment 54

[Jorge Villalobos [:jorgev] (he/him)]

The MXR shows a few add-ons that use that function. We recently published the compatibility blog post for 42, but we can update it. Also we haven't done the bulk validation yet, so I can add a test for this and warn developers about it. In short, I'm okay with this change.

Since this is a security bug, I could use a short sentence explanation of this change so developers know what this is about.

Comment 55

[(no longer active)]

(In reply to Jorge Villalobos [:jorgev] from comment #54)
> Since this is a security bug, I could use a short sentence explanation of
> this change so developers know what this is about.

What level of detail should we get into?  Explaining the reason why we renamed the function and added a new one gives away the nature of the bug... :(

Comment 56

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #55)
> What level of detail should we get into?  Explaining the reason why we
> renamed the function and added a new one gives away the nature of the bug...
> :(

Has the behavior of the function changed in any way that add-on devs would notice? If not, we can just say that the function was renamed as part of a security fix.

Comment 57

[:Gijs (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #56)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #55)
> > What level of detail should we get into?  Explaining the reason why we
> > renamed the function and added a new one gives away the nature of the bug...
> > :(
> 
> Has the behavior of the function changed in any way that add-on devs would
> notice? If not, we can just say that the function was renamed as part of a
> security fix.

Comment 58

[(no longer active)]

(In reply to Jorge Villalobos [:jorgev] from comment #56)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #55)
> > What level of detail should we get into?  Explaining the reason why we
> > renamed the function and added a new one gives away the nature of the bug...
> > :(
> 
> Has the behavior of the function changed in any way that add-on devs would
> notice? If not, we can just say that the function was renamed as part of a
> security fix.

The behavior of the new parseResponseContentType() function is the same as the old parseContentType() function, so that sounds good to me.

Comment 59

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ehsan, I was reviewing the patch you nominated for uplift to esr38.4.0 and I don't think I can approve the IDL (uuid) change. This would be a breaking change for esr. Even though the extent of add-ons that would break is low (reading through the previous comments), this kind of change is not usually uplifted to ESRs.

Given that, we have two choices: A) Create a different patch for esr that does not include IDL changes or B) wontfix for esr38. Thoughts?

Comment 60

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8668656 [details] [diff] [review]
Part 1: Rename nsINetUtil.parseContentType() to parseResponseContentType()

If Jorge is OK, I am OK for 42. Should be in 42 beta 7.

Comment 61

[(no longer active)]

(In reply to Ritu Kothari (:ritu) from comment #59)
> Ehsan, I was reviewing the patch you nominated for uplift to esr38.4.0 and I
> don't think I can approve the IDL (uuid) change. This would be a breaking
> change for esr. Even though the extent of add-ons that would break is low
> (reading through the previous comments), this kind of change is not usually
> uplifted to ESRs.

What is specifically considered acceptable for ESR?  Do I have to create an nsINetUtil_ESR_NN interface like we did in the old days?

Comment 62

[Jorge Villalobos [:jorgev] (he/him)]

From the add-on compat perspective, what we can't do is remove any APIs that are currently in use. Adding new APIs while preserving the existing ones is okay.

Comment 63

[(no longer active)]

OK, thanks!  I'll prepare an ESR38 patch.

Comment 64

[(no longer active)]

Attachment: esr38 specific patch

This is sufficiently different that it would be nice if it got reviewed separately.

Please note that this is the only patch for this bug to land on ESR38, I gave up on renaming things there since we aren't developing on that branch.

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4f08c76ba2c

Comment 66

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f828f5ec27c

Comment 67

[Carsten Book [:Tomcat]]

sorry had to back this out from aurora for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1326440&repo=mozilla-aurora

Comment 68

[Carsten Book [:Tomcat]]

also had to back this out from beta for failing on own tests like https://treeherder.mozilla.org/logviewer.html#?job_id=576007&repo=mozilla-beta

Comment 69

[(no longer active)]

(In reply to Carsten Book [:Tomcat] from comment #67)
> sorry had to back this out from aurora for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=1326440&repo=mozilla-
> aurora
(In reply to Carsten Book [:Tomcat] from comment #68)
> also had to back this out from beta for failing on own tests like
> https://treeherder.mozilla.org/logviewer.html#?job_id=576007&repo=mozilla-
> beta

Well, yeah, of course.  You only landed part 1.  But the approval request was for part 1-3.  Sorry this is confusing.

I'll land on Aurora and Beta myself.

Comment 70

[(no longer active)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/684ad79fcbad
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/adae4392080b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/513f936fee6c

Comment 71

[(no longer active)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/82cc6b6e6eaa
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/71966c03aef5
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/c1cd30406136

Comment 72

[(no longer active)]

Comment on attachment 8673966 [details] [diff] [review]
esr38 specific patch

See comment 39, sans the uuid change.

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/e86f9e931083

Comment 74

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue on Fx41.

Verified fixed on Fx42, 43 and 44, as well as latest ESR 38.4.0.

Comment 75

[(no longer active)]

Now that the fix for this has been shipped, can I land parts 4-6?

Comment 76

[Daniel Veditz [:dveditz]]

Yes, please land parts 4-6 now.

Comment 77

[(no longer active)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f599f773b134
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa7495fc298
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8fdc09bd1e

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb8fdc09bd1e
https://hg.mozilla.org/mozilla-central/rev/bfa7495fc298
https://hg.mozilla.org/mozilla-central/rev/f599f773b134