Packaged Apps do not apply security headers

RESOLVED FIXED in Firefox 43, Firefox OS master

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: freddyb, Assigned: hchang, NeedInfo)

Tracking

({sec-moderate})

unspecified
FxOS-S5 (21Aug)
sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox43 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main43-])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

3 years ago
+++ 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?
(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.
(Assignee)

Updated

3 years ago
Assignee: nobody → hchang
(Assignee)

Comment 3

3 years ago
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!
Attachment #8634720 - Flags: review?(valentin.gosu)
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.
Attachment #8634720 - Flags: review?(valentin.gosu)
Attachment #8634720 - Flags: review?(honzab.moz)
Attachment #8634720 - Flags: feedback+
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
Attachment #8634720 - Flags: review?(honzab.moz) → review-
Priority: -- → P1
(Assignee)

Updated

3 years ago
Attachment #8634720 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
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!
Flags: needinfo?(honzab.moz)
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..();
}
Attachment #8639163 - Flags: review+
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 9

3 years ago
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.
Yep, don't copy the Content-Language as well.
(Assignee)

Comment 11

3 years ago
Created attachment 8639724 [details] [diff] [review]
Bug1181137.patch (carry r+ and modify test case)
Attachment #8639724 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8639163 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8640267 [details] [diff] [review]
Bug1181137.patch (carry r+, modified test case, and fix lint error)
(Assignee)

Updated

3 years ago
Attachment #8640267 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8639724 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8640274 [details] [diff] [review]
Bug1181137.patch (carry r+, modified test case, and fix lint error)
Attachment #8640267 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
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!
Flags: needinfo?(honzab.moz)
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?
Flags: needinfo?(jonas)
blocking-b2g: --- → 2.5+
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
Attachment #8640274 - Flags: feedback+
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 17

3 years ago
Created attachment 8644742 [details] [diff] [review]
Bug1181137 (carry r+, modified test case, fixed lint error, and added set-cookies to the black list)
(Assignee)

Updated

3 years ago
Attachment #8644742 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8640274 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Target Milestone: --- → FxOS-S5 (21Aug)
https://hg.mozilla.org/mozilla-central/rev/462c4c964f04
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
status-b2g-master: --- → fixed
Minusing for advisory based on https://bugzilla.mozilla.org/show_bug.cgi?id=1180637#c14
Whiteboard: [adv-main43-]
(Reporter)

Comment 22

3 years ago
(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.
You need to log in before you can comment on or make changes to this bug.