1120985 - Allow nsMultiMixedConv to compute its boundary if content-type=application/package

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

Currently, nsMultiMixedConv must be given a boundary in the content-type header, or it will not work [1]
Also, if the content doesn't start with a boundary, it will automatically add it to the stream (presumably to avoid an Apache bug)

However, the streamable format for application/package doesn't require it be given a boundary, and it enforces that the package starts with the boundary (otherwise, parsing the package would be impossible). [2]

In order to not break anything on the web, I think we should restrain this new behaviour only for application/package, and not for multipart/*.

[1] https://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsMultiMixedConv.cpp#697
[2] https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format

Comment 1

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

Attachment: Allow nsMultiMixedConv to compute its boundary if content-type=application/package

Also makes nsMultiMixedConv/nsPartChannel save and return individual headers for each part of the resource file.

Comment 2

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

Attachment: have GetURI return the packaged resource's URI

I'm not sure if this change is actually required. Probably not. Just leaving it here for now.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8549217 [details] [diff] [review]
Allow nsMultiMixedConv to compute its boundary if content-type=application/package

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

::: netwerk/base/public/moz.build
@@ +86,5 @@
>      'nsIRedirectResultListener.idl',
>      'nsIRequest.idl',
>      'nsIRequestObserver.idl',
>      'nsIRequestObserverProxy.idl',
> +    'nsIResponseHeadProvider.idl',

and the file?

::: netwerk/protocol/http/nsHttpResponseHead.h
@@ +11,5 @@
>  #include "nsString.h"
>  
> +namespace IPC {
> +    template <typename> struct ParamTraits;
> +}

isn't there some header for this?
why exactly is it here (and there)?

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +396,5 @@
> +nsPartChannel::VisitResponseHeaders(nsIHttpHeaderVisitor *visitor)
> +{
> +  if (!mResponseHead)
> +    return NS_ERROR_NOT_AVAILABLE;
> +  return mResponseHead->Headers().VisitHeaders(visitor);

indent

@@ +547,4 @@
>          PushOverLine(cursor, bufLen);
>  
> +        bool needMoreChars = bufLen < mTokenLen + 2;
> +        char * lfChar = PL_strnchr(buffer, '\r', bufLen);

bool lfChar = nsDependentCString(buffer, bufLen).Find('\r') != kNotFound;

@@ +554,5 @@
>              // is called.
>              mFirstOnData = true;
> +        } else if (mPackagedApp) {
> +            // We need to check the line starts with --
> +            if (bufLen >= 2 && (buffer[0] != '-' || buffer[1] != '-')) {

StringBeginsWith(nsDependentCString(buffer, bufLen), NS_LITERAL_CSTRING("--"))

@@ +560,5 @@
> +            }
> +
> +            // If the boundary was set in the header,
> +            // we need to check it matches with the one in the file.
> +            if (mTokenLen && PL_strncmp(buffer + 2, mToken.get(), mTokenLen)) {

StringBeginsWith(nsDependentCString(buffer + 2, bufLen - 2), mToken);

@@ +565,5 @@
> +                return NS_ERROR_FAILURE;
> +            }
> +
> +            if (!mTokenLen) {
> +                mToken = nsCString(buffer + 2, lfChar - buffer - 2);

Are you sure this is copied?  I assume it is, but please double check.

@@ +729,5 @@
> +    // https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format
> +    // Although it is compatible with multipart/* this format does not require
> +    // the boundary to be included in the header, as it can be ascertained from
> +    // the content of the file.
> +    if (delimiter.Find("application/package") != kNotFound) {

when some malicious provider injects this to the boundary for an otherwise multipart/* content, don't we get to some unexpected state that could potentially be somehow exploited?  There are lots of strstr dangerous functions, so one could find out how to load something we actually don't want to load.


Maybe check the mimetype after you don't find the "boundary" keyword?

@@ +731,5 @@
> +    // the boundary to be included in the header, as it can be ascertained from
> +    // the content of the file.
> +    if (delimiter.Find("application/package") != kNotFound) {
> +        mPackagedApp = true;
> +        mToken = "";

mToken.Truncate()

@@ +757,5 @@
>  
>      mToken = boundaryString;
>      mTokenLen = boundaryString.Length();
> +
> +   if (mTokenLen == 0 && !mPackagedApp)

I think when !mPackagedApp will always pass..

@@ +772,5 @@
> +
> +    if (mToken.IsEmpty()) {
> +        aStatus = NS_ERROR_FAILURE;
> +        rv = NS_ERROR_FAILURE;
> +    }

so, this was a bug in nsMultiMixedConv?
anyway, add comments why you no longer return

@@ +900,5 @@
>  
>      nsCOMPtr<nsILoadGroup> loadGroup;
>      (void)mPartChannel->GetLoadGroup(getter_AddRefs(loadGroup));
>  
> +    mPartChannel->SetResponseHead(mResponseHead.forget());

why?  this needs comments.
also, keep off the "load group assignment" block

then, I'd much more prefer to not carry the response head via a member, but rather via a local var.  seems doable.

@@ +995,5 @@
>      uint32_t lineFeedIncrement = 1;
>      
> +    if (mPackagedApp && !mResponseHead) {
> +        mResponseHead = new nsHttpResponseHead();
> +    }

as well here.

@@ +1022,5 @@
>          char tmpChar = *newLine;
>          *newLine = '\0'; // cursor is now null terminated
> +
> +        if (mResponseHead) {
> +            nsAutoCString tmpHeader(cursor);

why you need to go through nsAutoCString anyway?

::: netwerk/test/unit/test_content_type_application_package.js
@@ +19,5 @@
> +                         Ci.nsILoadInfo.SEC_NORMAL,
> +                         Ci.nsIContentPolicy.TYPE_OTHER);
> +}
> +
> +var multipartBody = "--gc0pJq0M:08jU534c0p\r\nContent-Location: /index.html\r\nContent-Type: text/html\r\n\r\n<html>\r\n  <head>\r\n    <script src=\"/scripts/app.js\"></script>\r\n    ...\r\n  </head>\r\n  ...\r\n</html>\r\n\r\n--gc0pJq0M:08jU534c0p\r\nContent-Location: /scripts/app.js\r\nContent-Type: text/javascript\r\n\r\nmodule Math from '/scripts/helpers/math.js';\r\n...\r\n\r\n--gc0pJq0M:08jU534c0p\r\nContent-Location: /scripts/helpers/math.js\r\nContent-Type: text/javascript\r\n\r\nexport function sum(nums) { ... }\r\n...\r\n\r\n--gc0pJq0M:08jU534c0p--";

could you format this a bit? like:

"--asdfaf\r\n" +
"Content-Location: ...\r\n" +
...

@@ +44,5 @@
> +  [
> +   { data: "<html>\r\n  <head>\r\n    <script src=\"/scripts/app.js\"></script>\r\n    ...\r\n  </head>\r\n  ...\r\n</html>\r\n", type: "text/html" },
> +   { data: "module Math from '/scripts/helpers/math.js';\r\n...\r\n", type: "text/javascript" },
> +   { data: "export function sum(nums) { ... }\r\n...\r\n", type: "text/javascript" }
> +  ];

maybe build the response string from this?

@@ +81,5 @@
> +
> +  onDataAvailable: function(request, context, stream, offset, count) {
> +    try {
> +      this._buffer = this._buffer.concat(read_stream(stream, count));
> +      dump("BUFFEEE: " + this._buffer + "\n\n");

remove the dump please

@@ +106,5 @@
> +  },
> +
> +  visitHeader(header, value) {
> +    headerNum++;
> +    dump("HEADER: "+header+":"+value+"\n");

don't you want to check the headers a bit more?
remove the dump please.

::: netwerk/test/unit/xpcshell.ini
@@ +307,5 @@
>  # The local cert service used by this test is not currently shipped on Android
>  skip-if = os == "android"
>  [test_1073747.js]
> +[test_content_type_application_package.js]
> +

no new line

Comment 4

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

(In reply to Honza Bambas (:mayhemer) from comment #3)
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.h
> > +namespace IPC {
> > +    template <typename> struct ParamTraits;
> > +}
> 
> isn't there some header for this?
> why exactly is it here (and there)?

There is, but this needs to be fwd-declared to avoid including/exporting the other headers as well. I added comments to say so.
> 
> bool lfChar = nsDependentCString(buffer, bufLen).Find('\r') != kNotFound;
> 

buffer isn't null-terminated, so I can't directly use a nsDependentCString.
I can create a new nsCString based on the buffer, or allocate an extra byte at the end of the buffer and set it to \0.

> @@ +729,5 @@
> > +    // https://w3ctag.github.io/packaging-on-the-web/#streamable-package-format
> > +    // Although it is compatible with multipart/* this format does not require
> > +    // the boundary to be included in the header, as it can be ascertained from
> > +    // the content of the file.
> > +    if (delimiter.Find("application/package") != kNotFound) {
> 
> when some malicious provider injects this to the boundary for an otherwise
> multipart/* content, don't we get to some unexpected state that could
> potentially be somehow exploited?  There are lots of strstr dangerous
> functions, so one could find out how to load something we actually don't
> want to load.
> 

Having application/package in the content-type just says it should try to compute the boundary from the first line in the content. This would fail if the boundary were missing. But I agree that we should be careful about the security aspect of it.

> 
> Maybe check the mimetype after you don't find the "boundary" keyword?
> 

> > +   if (mTokenLen == 0 && !mPackagedApp)
> 
> I think when !mPackagedApp will always pass..

The spec isn't very clear about this: http://www.w3.org/TR/web-packaging/#streamable-package-format
It doesn't specifically say that it mustn't include the boundary in the header. If we keep this optional, it would make the application/package format compatible with the multipart/* format.

> 
> @@ +772,5 @@
> > +
> > +    if (mToken.IsEmpty()) {
> > +        aStatus = NS_ERROR_FAILURE;
> > +        rv = NS_ERROR_FAILURE;
> > +    }
> 
> so, this was a bug in nsMultiMixedConv?
> anyway, add comments why you no longer return

Before, we would have detected a missing token in AsyncOpen. Since we're parsing the response, we can fail later, and we need to pass this error to the listener. I added comments clarifying this.

> 
> @@ +1022,5 @@
> >          char tmpChar = *newLine;
> >          *newLine = '\0'; // cursor is now null terminated
> > +
> > +        if (mResponseHead) {
> > +            nsAutoCString tmpHeader(cursor);
> 
> why you need to go through nsAutoCString anyway?

ParseHeaderLine is destructive.

Comment 5

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

Attachment: Allow nsMultiMixedConv to compute its boundary if content-type=application/package

Also makes nsMultiMixedConv/nsPartChannel save and return individual headers for each part of the resource file.

Comment 6

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

Attachment: interdiff.patch

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8558919 [details] [diff] [review]
Allow nsMultiMixedConv to compute its boundary if content-type=application/package

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

::: netwerk/base/nsIResponseHeadProvider.idl
@@ +9,5 @@
> +
> +%{C++
> +namespace mozilla { namespace net {
> +  class nsHttpResponseHead;
> +} }

nit:

namespace A {
namespace B {

}
}

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +571,5 @@
> +
> +            // Save the token.
> +            if (!mTokenLen) {
> +                mToken = nsCString(Substring(firstBuffer, 2).BeginReading(),
> +                                             posCR - 2);

posCR should be where Substring is (column)

@@ +597,5 @@
>      char *token = nullptr;
>  
> +    // This may get initialized by ParseHeaders and the resulting
> +    // HttpResponseHead will be passed to nsPartChannel by SendStart
> +    nsAutoPtr<nsHttpResponseHead> responseHead;

nit: declare before use.  don't worry to declare twice (in two blocks)

@@ +902,5 @@
>      // Set up the new part channel...
>      mPartChannel = newChannel;
>  
> +    // We pass the headers to the nsPartChannel
> +    mPartChannel->SetResponseHead(aHead.forget());

I don't understand the forget() here.  SetResponseHead() assigns to nsRefPtr, so you actually leak here?

@@ +1002,5 @@
>  }
>  
>  nsresult
> +nsMultiMixedConv::ParseHeaders(nsIChannel *aChannel, char *&aPtr,
> +                               uint32_t &aLen, nsAutoPtr<nsHttpResponseHead> &aHead,

nit: you can have nsHttpResponseHead **aHead and caller can do getter_AddRefs(head), it's the usual way unless there is some obstacle.

best have a local nsRefPtr and then forget it to the ** result.

Comment 8

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

Attachment: Allow nsMultiMixedConv to compute its boundary if content-type=application/package

Also makes nsMultiMixedConv/nsPartChannel save and return individual headers for each part of the resource file.

Comment 9

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

(In reply to Honza Bambas (:mayhemer) from comment #7)
> @@ +597,5 @@
> >      char *token = nullptr;
> >  
> > +    // This may get initialized by ParseHeaders and the resulting
> > +    // HttpResponseHead will be passed to nsPartChannel by SendStart
> > +    nsAutoPtr<nsHttpResponseHead> responseHead;
> 
> nit: declare before use.  don't worry to declare twice (in two blocks)
> 

Actually, it has to be the same. Moreover, I had to backtrack on the "declaring the responseHead on the stack" issue, because with large enough headers, OnDataAvailable may be called twice, meaning mResponseHead has to be a member variable (I added a test for this)

> @@ +902,5 @@
> >      // Set up the new part channel...
> >      mPartChannel = newChannel;
> >  
> > +    // We pass the headers to the nsPartChannel
> > +    mPartChannel->SetResponseHead(aHead.forget());
> 
> I don't understand the forget() here.  SetResponseHead() assigns to
> nsRefPtr, so you actually leak here?
> 

nsHttpResponseHead is not refCounted, so here we would simply set mResponseHead to null, while passing the pointer to the partChannel. I checked that there are no actual leaks here.

> @@ +1002,5 @@
> >  }
> >  
> >  nsresult
> > +nsMultiMixedConv::ParseHeaders(nsIChannel *aChannel, char *&aPtr,
> > +                               uint32_t &aLen, nsAutoPtr<nsHttpResponseHead> &aHead,
> 
> nit: you can have nsHttpResponseHead **aHead and caller can do
> getter_AddRefs(head), it's the usual way unless there is some obstacle.
> 
> best have a local nsRefPtr and then forget it to the ** result.

Can't do this since it's not refCounted, and now we have to keep it as a member variable.

Comment 10

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

Attachment: nsPartChannel does not return the correct value for isLastPart

Comment 11

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

This last patch is for a minor bug I found in nsMultiMixedConv.
It is however a feature that I really need for the packaged app service :)

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8560793 [details] [diff] [review]
Allow nsMultiMixedConv to compute its boundary if content-type=application/package

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

Thanks!

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +772,5 @@
>      mToken = boundaryString;
>      mTokenLen = boundaryString.Length();
> +
> +   if (mTokenLen == 0 && !mPackagedApp)
> +       return NS_ERROR_FAILURE;

{ }

@@ +1012,5 @@
>      bool done = false;
>      uint32_t lineFeedIncrement = 1;
> +
> +    // We only create an nsHttpResponseHead for packaged app channels
> +    // It may already be initialized, from a previous call of ParseHeaders

maybe (to make it absolutely clear for dummies) add:

// since the headers for a single part may come in more then one chunk

::: netwerk/test/unit/test_multipart_streamconv_application_package.js
@@ +1,1 @@
> +Cu.import("resource://testing-common/httpd.js");

add a description (best a list of steps) what this test is doing please.

@@ +88,5 @@
> +
> +function initHugeHeaders() {
> +  // We need to insert a lot of data into the headers to make sure all of it
> +  // is being properly processed. The cutoff is around 400, so we insert 500 to
> +  // be sure. 2 separate resource are enough, with different values for each one.

you could also send the content back in small chunks (have an async response handler that writes piece by piece).

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8561191 [details] [diff] [review]
nsPartChannel does not return the correct value for isLastPart

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

r=honzab with comments fixed.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +621,5 @@
>              (*(token + mTokenLen + 1) == '-')) {
>              // This was the last delimiter so we can stop processing
>              rv = SendData(cursor, LengthToToken(cursor, token));
>              if (NS_FAILED(rv)) return rv;
> +            mPartChannel->SetIsLastPart();

null check before use

Comment 14

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

Attachment: nsPartChannel does not return the correct value for isLastPart

Comment 15

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

Attachment: Allow nsMultiMixedConv to compute its boundary if content-type=application/package

Also makes nsMultiMixedConv/nsPartChannel save and return individual headers for each part of the resource file.

Comment 16

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

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3743376a0aa9

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edadb5717a9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4168d9fc7207

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edadb5717a9c
https://hg.mozilla.org/mozilla-central/rev/4168d9fc7207