+++ 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.

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 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 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

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 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..(); }

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.

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!

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 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

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

(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.