1181137 - Packaged Apps do not apply security headers

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Frederik Braun [:freddy]]

+++ This bug was initially created as a clone of Bug #1180637 +++

If enumerating all security headers is now on me, I'm entirely sure that I will miss something.

But we should look at X-Frame-Options and Access-Control-Allow-Origin to say the least.

Manually adding headers to the cache sounds a bit weird to me. Can't we copy all of the headers except for a few that we know we do *not* want them?

Comment 1

[Valentin Gosu [:valentin] (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #0)
> +++ This bug was initially created as a clone of Bug #1180637 +++
> 
> If enumerating all security headers is now on me, I'm entirely sure that I
> will miss something.
> 
> But we should look at X-Frame-Options and Access-Control-Allow-Origin to say
> the least.
> 
> Manually adding headers to the cache sounds a bit weird to me. Can't we copy
> all of the headers except for a few that we know we do *not* want them?

I guess copying all/most of the package's headers is also possible, but we need to figure out which ones we want to ignore.

Comment 2

[Henry Chang [:hchang]]

Attachment: Bug1181137.patch

Comment 3

[Henry Chang [:hchang]]

Comment on attachment 8634720 [details] [diff] [review]
Bug1181137.patch

Hi Valentin,

I've attached a patch to copy all the headers from the base channel to 
subresource's header except:

1) The header already in the subresource like 'Content-Location'.
2) The header is in the pre-defined black list like 'Content-Length'.

I tested with http://people.mozilla.org/~fdesre/packages/loqui.pak!//index.html,
which has the following base header

Date: Thu, 16 Jul 2015 13:36:34 GMT
Server: Apache
X-Backend-Server: people1.dmz.scl3.mozilla.com
Last-Modified: Wed, 24 Jun 2015 22:00:23 GMT
ETag: "440d91"
Accept-Ranges: bytes
Content-Length: 4459921
Content-Type: application/package

and all of them would be copied to each subresource except 'Content-Length'
since it's appeared in the black list.

Could you please help review this patch? Thansk!

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8634720 [details] [diff] [review]
Bug1181137.patch

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

It looks fine to me, but I'd rather Honza took a look at it too. He's been the main reviewer for this feature.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8634720 [details] [diff] [review]
Bug1181137.patch

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

Overall good.

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +88,5 @@
>  }
>  
> +namespace
> +{
> +  class HeaderCopier final : public nsIHttpHeaderVisitor

namespace { // anon

class ...
{
  ...
}

} // anon

@@ +95,5 @@
> +    NS_DECL_ISUPPORTS
> +    NS_DECL_NSIHTTPHEADERVISITOR
> +
> +    HeaderCopier(nsHttpResponseHead* aHeader)
> +      : mHeader(aHeader)

don't use the word "header", this is "head"

@@ +121,5 @@
> +
> +  bool
> +  HeaderCopier::ShouldCopy(const nsACString &aHeader) const
> +  {
> +    // Don't ever copy headers that already in the destination.

that are already, in other words "don't overwrite"?

@@ +130,5 @@
> +    // A black list for header we shouldn't copy.
> +    const char* IGNORED_HEADER[] = {
> +      "Content-Location",
> +      "Content-Type",
> +      "Content-Length",

First, use nsHttp atoms.  See https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpAtomList.h

Second, we probably need to ignore much more headers than just these three.

Some of coming to my mind:
ETag
Last-Modified - but here it depends...  Valentin?
Cache-control - but this one needs to look into the spec how should be handled
Authentication
Connection
Content-Disposition
Content-MD5
Content-Range
Proxy-Authenticate
Proxy-Connection
TE
Trailer
Transfer-Encoding
Vary - but this one also needs to be consulted with the spec or the spec should be updated how to copy/handle it
WWW-Authenticate

@@ +148,3 @@
>  /* static */ nsresult
>  PackagedAppService::CacheEntryWriter::CopyHeadersFromChannel(nsIChannel *aChannel,
>                                                    nsHttpResponseHead *aHead)

when you are here, please fix the indention

Comment 6

[Henry Chang [:hchang]]

Attachment: Bug1181137.patch

Comment 7

[Henry Chang [:hchang]]

Hi Honza,

Are you able to review the patch again? I cannot flag your for reviewing.
If you can't, who else should I ask for better?

Thanks!

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8639163 [details] [diff] [review]
Bug1181137.patch

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +92,5 @@
>  
> +namespace // anon
> +{
> +  class HeaderCopier final : public nsIHttpHeaderVisitor
> +  {

we don't indent inside namespaces, as I wrote in the comment, please copy the style exactly:


namespace { // anon

class ...
{
  ...
}

} // anon

@@ +131,5 @@
> +    if (mHead->PeekHeader(header)) {
> +      return false;
> +    }
> +
> +    // A black list for header we shouldn't copy.

of headers

@@ +132,5 @@
> +      return false;
> +    }
> +
> +    // A black list for header we shouldn't copy.
> +    const nsHttpAtom COPY_BLACKLIST[] = {

make this static const and call it kHeadersCopyBlacklist

@@ +138,5 @@
> +      nsHttp::Cache_Control,
> +      nsHttp::Connection,
> +      nsHttp::Content_Disposition,
> +      nsHttp::Content_Encoding,
> +      nsHttp::Content_Language,

hmm.. maybe I'd copy this one, what the spec says?

@@ +180,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsRefPtr<HeaderCopier> headerCopier = new HeaderCopier(aHead);
> +  if (!NS_SUCCEEDED(httpChan->VisitResponseHeaders(headerCopier))) {

nit: if you don't want any console warnings here (probably no need to) then just do:

  return httpChan->Visit..();
}

Comment 9

[Henry Chang [:hchang]]

Thanks Honza! Sorry for ignoring the indentation again.

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #8)
> Comment on attachment 8639163 [details] [diff] [review]
> Bug1181137.patch
> 
> Review of attachment 8639163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +138,5 @@
> > +      nsHttp::Cache_Control,
> > +      nsHttp::Connection,
> > +      nsHttp::Content_Disposition,
> > +      nsHttp::Content_Encoding,
> > +      nsHttp::Content_Language,
> 
> hmm.. maybe I'd copy this one, what the spec says?
> 

The spec says as [1] and it's hard to tell if it would mislead the processing
of the resource. Since the HTTP header is usually controlled by the server, 
what if we stay conservative and only copy the headers that we MUST copy?

Thanks!

[1]

The Content-Language entity-header field describes the natural language(s) of the intended audience for the enclosed entity. Note that this might not be equivalent to all the languages used within the entity-body.

       Content-Language  = "Content-Language" ":" 1#language-tag
Language tags are defined in section 3.10. The primary purpose of Content-Language is to allow a user to identify and differentiate entities according to the user's own preferred language. Thus, if the body content is intended only for a Danish-literate audience, the appropriate field is

       Content-Language: da
If no Content-Language is specified, the default is that the content is intended for all language audiences. This might mean that the sender does not consider it to be specific to any natural language, or that the sender does not know for which language it is intended.

Multiple languages MAY be listed for content that is intended for multiple audiences. For example, a rendition of the "Treaty of Waitangi," presented simultaneously in the original Maori and English versions, would call for

       Content-Language: mi, en
However, just because multiple languages are present within an entity does not mean that it is intended for multiple linguistic audiences. An example would be a beginner's language primer, such as "A First Lesson in Latin," which is clearly intended to be used by an English-literate audience. In this case, the Content-Language would properly only include "en".

Content-Language MAY be applied to any media type -- it is not limited to textual documents.

Comment 10

[Honza Bambas (:mayhemer)]

Yep, don't copy the Content-Language as well.

Comment 11

[Henry Chang [:hchang]]

Attachment: Bug1181137.patch (carry r+ and modify test case)

Comment 12

[Henry Chang [:hchang]]

Attachment: Bug1181137.patch (carry r+, modified test case, and fix lint error)

Comment 13

[Henry Chang [:hchang]]

Attachment: Bug1181137.patch (carry r+, modified test case, and fix lint error)

Comment 14

[Henry Chang [:hchang]]

Hi Honza,

Copying header would break the existing test case so I just added a quick
fix to the test case. The header verification will only strstr the target
header instead of exact matching. Looking into the nsHttpResponseHead 
implementation, the copied headers are guaranteed appeared in the end so
the position of expected headers is always at the very beginning. Could you
please have a quick look at the modified test case? Thanks!

Comment 15

[Paul Theriault [:pauljt] (no longer reading bugmail)]

What about set-cookie headers? I'm pretty sure we don't want to copy these since it doesnt make a lot of sense, and would be a security risk in the signed package case.


Actually for signed packages they get their own origin, so we probably just don't want to copy ANY headers, and rely solely on package headers in that case?

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8640274 [details] [diff] [review]
Bug1181137.patch (carry r+, modified test case, and fix lint error)

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

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +91,5 @@
>  }
>  
> +namespace { // anon
> +  class HeaderCopier final : public nsIHttpHeaderVisitor
> +  {

how many times do I have to repeat this?

namespace { // anon

class
{
};

} // anon

::: netwerk/test/unit/test_packaged_app_service.js
@@ +131,5 @@
>    onMetaDataElement: function(key, value) {
> +    if (key == 'response-head') {
> +      var kExpectedResponseHead = "HTTP/1.1 200 \r\nContent-Location: /index.html\r\nContent-Type: text/html\r\n";
> +      ok(0 === value.indexOf(kExpectedResponseHead), 'The cached response header not matched');
> +    }

I think it's ok

Comment 17

[Henry Chang [:hchang]]

Attachment: Bug1181137 (carry r+, modified test case, fixed lint error, and added set-cookies to the black list)

Comment 18

[Henry Chang [:hchang]]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/462c4c964f04

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/462c4c964f04

Comment 21

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

Minusing for advisory based on https://bugzilla.mozilla.org/show_bug.cgi?id=1180637#c14

Comment 22

[Frederik Braun [:freddy]]

(In reply to Paul Theriault [:pauljt] from comment #15)
> What about set-cookie headers? I'm pretty sure we don't want to copy these
> since it doesnt make a lot of sense, and would be a security risk in the
> signed package case.
> 
> 
> Actually for signed packages they get their own origin, so we probably just
> don't want to copy ANY headers, and rely solely on package headers in that
> case?

Very late reply:
This is because of bug 1181137 comment 0, i.e., packaged apps ignoring site/origin-wide security headers and a potential attacker thus being able to escape existing security controls via packages.

We wanted to avoid enumerating all relevant security headers and potentially missing some of them. Instead, we went for a list of headers that we do not explicitly want.