1263793 - Verify remote newtab signatures using the content signature service

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Franziskus Kiefer [:franziskus]]

We should use the new content signature service for remote newtab content signatures.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review commit: https://reviewboard.mozilla.org/r/53954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53954/

Comment 2

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review51726

This looks OK. Keeler should take a look too, I think.

::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js:223
(Diff revision 1)
>    for (let i = 0; i < TESTS.length; i++) {
>      let testCase = TESTS[i];
>      let url = "", aNewTabPref = "";
>      let reload = false;
>      var aExpectedStrings = testCase.testStrings;
> +    dump(" >>> "+testCase.testStrings+"\n");

Do we need to leave these dumps here?

Comment 3

[Franziskus Kiefer [:franziskus]]

(In reply to Mark Goodwin [:mgoodwin] from comment #2)
> Comment on attachment 8754401 [details]
> Do we need to leave these dumps here?

no... I'll remove it when Keeler did a review. I suspect he wants me to change something anyway.

Comment 4

[Franziskus Kiefer [:franziskus]]

https://reviewboard.mozilla.org/r/53954/#review51726

> Do we need to leave these dumps here?

nope, will remove in the next version after Keeler's review

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review51940

This looks good, but I'd like to have a look at the interdiff after addressing these comments.

::: dom/security/ContentVerifier.h:16
(Diff revision 1)
>  #include "nsIObserver.h"
>  #include "nsIStreamListener.h"
>  #include "nsNSSShutDown.h"
>  #include "nsString.h"
>  #include "nsTArray.h"
>  #include "ScopedNSSTypes.h"

The NSS-related includes can be removed (and nsIObserver.h, looks like).

::: dom/security/ContentVerifier.cpp:19
(Diff revision 1)
>  #include "nsIRequest.h"
>  #include "nssb64.h"
>  #include "nsSecurityHeaderParser.h"
>  #include "nsServiceManagerUtils.h"
>  #include "nsStringStream.h"
>  #include "nsThreadUtils.h"

Looks like some of these includes can be removed.

::: dom/security/ContentVerifier.cpp:135
(Diff revision 1)
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
>  
>    // update the signature verifier
> -  return Update(mContent[mContent.Length()-1]);
> +  return mVerifier->Update(mContent[mContent.Length()-1]);

How about just mContent.LastElement()?

::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js:34
(Diff revision 1)
>  const URI_ERROR_HEADER = URI_HEADER_BASE + "error";
> -const URI_KEYERROR_HEADER = URI_HEADER_BASE + "errorInKeyid";
> +const URI_KEYERROR_HEADER = URI_HEADER_BASE + "errorInX5U";
>  const URI_SIGERROR_HEADER = URI_HEADER_BASE + "errorInSignature";
>  const URI_NO_HEADER = URI_HEADER_BASE + "noHeader";
>  
>  const URI_BAD_SIG = BASE + "sig=bad&key=good&file=good&header=good";

Unless I'm misunderstanding thow these tests work, it seems like in some cases (but not all) the "key" parameter has been replaced with "x5u". This should be consistent one way or another.

::: dom/security/test/contentverifier/file_contentserver.sjs:35
(Diff revision 1)
>  const sriSignature = path + "file_about_newtab_sri_signature";
>  
>  const badCspFile = path + "file_about_newtab_bad_csp.html";
>  const badCspSignature = path + "file_about_newtab_bad_csp_signature";
>  
> +const goodCertChain = path + "goodChain.pem";

Maybe "goodCertChainPath"?

::: dom/security/test/contentverifier/file_contentserver.sjs:125
(Diff revision 1)
>   * it further handles invalidateFile=yep and validateFile=yep to change the
>   * served file
>   */
>  function handleRequest(request, response) {
>    let params = new URLSearchParams(request.queryString);
>    let keyType = params.get("key");

Isn't this parameter not relevant any more?

::: dom/security/test/contentverifier/file_contentserver.sjs:184
(Diff revision 1)
>      return;
>    }
>  
> +  // we have to return the certificate chain on request for the x5u parameter
> +  if (x5uParam) {
> +    if (x5uParam == "default") {

nit: |if (x5uParam && x5uParam == "default") {|

::: dom/security/test/contentverifier/file_contentserver.sjs:210
(Diff revision 1)
>     */
>    let csHeader = "";
> -  let keyId = goodKeyId;
> +  let x5uString = goodX5UString;
>    let signature = goodSignature;
>    let file = goodFile;
>    if (keyType == "bad") {

Should this be x5uParam?

::: dom/security/test/contentverifier/goodChain.pem:1
(Diff revision 1)
> +-----BEGIN CERTIFICATE-----

How were these generated/where did they come from? (As in, if these came from certspec files, the specifications need to be included. If they didn't, they probably should.)

::: security/manager/ssl/ContentSignatureVerifier.h:49
(Diff revision 1)
>    ~ContentSignatureVerifier();
>  
>    nsresult UpdateInternal(const nsACString& aData,
>                            MutexAutoLock& /*proofOfLock*/,
>                            const nsNSSShutDownPreventionLock& /*proofOfLock*/);
> +  nsresult DownloadCertChain(nsACString& aCertChain,

aCertChain should be annotated as an out parameter.

::: security/manager/ssl/ContentSignatureVerifier.h:68
(Diff revision 1)
>  
>    // verifier context for incremental verifications
>    mozilla::UniqueVFYContext mCx;
>    // signature to verify
>    nsCString mSignature;
> +  // x5u value pointing to pem cert chain

Maybe this is a good place to document that "x5u" means "X.509 URL"

::: security/manager/ssl/ContentSignatureVerifier.h:69
(Diff revision 1)
>    // verifier context for incremental verifications
>    mozilla::UniqueVFYContext mCx;
>    // signature to verify
>    nsCString mSignature;
> +  // x5u value pointing to pem cert chain
> +  nsCString mCertChain;

This name could be a bit more descriptive. Maybe just tack on "URL" at the end?

::: security/manager/ssl/ContentSignatureVerifier.cpp:255
(Diff revision 1)
>    // add data if we got any
>    return UpdateInternal(aData, lock, locker);
>  }
>  
>  nsresult
> +ContentSignatureVerifier::DownloadCertChain(nsACString& aCertChain,

aCertChain should be annotated as an out parameter here too.

::: security/manager/ssl/ContentSignatureVerifier.cpp:269
(Diff revision 1)
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIChannel> channel;
> +  rv = NS_NewChannel(getter_AddRefs(channel), certChainURI,

This should be reviewed by someone who's familiar with the gotchas of making synchronous requests like this. Presumably we're certain this isn't running on the main thread? (As well as the socket thread, for that matter.) Perhaps we should add some assertions to that fact.

::: security/manager/ssl/ContentSignatureVerifier.cpp:306
(Diff revision 1)
> +                                                    const nsACString& aCSHeader,
> +                                                    const nsACString& aName)
> +{
> +  MutexAutoLock lock(mMutex);
> +
> +  if (mCx) {

Since this function doesn't check for NSS shutdown, this doesn't entirely prevent the bad behavior it's trying to prevent. Maybe just have an mInitialized bool?

::: security/manager/ssl/ContentSignatureVerifier.cpp:405
(Diff revision 1)
>  ContentSignatureVerifier::ParseContentSignatureHeader(
>    const nsACString& aContentSignatureHeader)
>  {
>    // We only support p384 ecdsa according to spec
>    NS_NAMED_LITERAL_CSTRING(signature_var, "p384ecdsa");
> +  NS_NAMED_LITERAL_CSTRING(certChain_var, "x5u");

Maybe "certChainURL_var" would be more clear.

::: security/manager/ssl/nsIContentSignatureVerifier.idl:64
(Diff revision 1)
>  
>    /**
> +   * Creates a context to verify a content signature against data that is added
> +   * later with update calls.
> +   * This does not require the caller to download the certificate chain. It's
> +   * done internally.

We should probably document that this means the header must contain an x5u field.

::: security/manager/ssl/nsIContentSignatureVerifier.idl:72
(Diff revision 1)
> +   * @param aContentSignatureHeader The signature of the data, url-safe base64
> +   *                                encoded.
> +   * @param aName                   The (host)name for which the end entity must
> +                                    be valid.
> +   */
> +  void createContextWithoutChain(in ACString aData, in ACString aSignature,

It looks like this is only ever called with an empty string for aData. This suggests that this parameter is unnecessary. Is this function ever expected to be called with a non-empty aData?

Also, "aSignature" doesn't match the documentation name of "aContentSignatureHeader", which seems a more appropriate name.

Comment 6

[Franziskus Kiefer [:franziskus]]

https://reviewboard.mozilla.org/r/53954/#review51940

> Unless I'm misunderstanding thow these tests work, it seems like in some cases (but not all) the "key" parameter has been replaced with "x5u". This should be consistent one way or another.

right, this should all be x5u, changed it

> Isn't this parameter not relevant any more?

this is the x5u you complained about in the other comment. renamed it to x5u

> How were these generated/where did they come from? (As in, if these came from certspec files, the specifications need to be included. If they didn't, they probably should.)

this is the cert chain from security/manager/ssl/tests/unit/test_content_signing/ (content_signing_remote_newtab_ee.pem, content_signing_int.pem, content_signing_root.pem).
I added a comment to this respect into file_contentserver.sjs where it is used.

> This name could be a bit more descriptive. Maybe just tack on "URL" at the end?

this actually confused me as well when reading it again... added URL already.

> This should be reviewed by someone who's familiar with the gotchas of making synchronous requests like this. Presumably we're certain this isn't running on the main thread? (As well as the socket thread, for that matter.) Perhaps we should add some assertions to that fact.

According to [1] this must be on the main thread. I added an assert to this respect but we should get someone from the necko folks look at it.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIChannel

> Since this function doesn't check for NSS shutdown, this doesn't entirely prevent the bad behavior it's trying to prevent. Maybe just have an mInitialized bool?

added mInitialised

> It looks like this is only ever called with an empty string for aData. This suggests that this parameter is unnecessary. Is this function ever expected to be called with a non-empty aData?
> 
> Also, "aSignature" doesn't match the documentation name of "aContentSignatureHeader", which seems a more appropriate name.

changed the name (also in the other functions).
We don't use it yet with any data, but other users might want to use it. It would be the same as calling createContext and then update. So I think this is nicer. But if you think no one will ever use it we can drop it (from all createContexts then to be consistent).

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/1-2/

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/2-3/

Comment 9

[Franziskus Kiefer [:franziskus]]

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

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: rb53954.patch

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8757280 [details] [diff] [review]
rb53954.patch

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

::: security/manager/ssl/ContentSignatureVerifier.cpp
@@ +277,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIInputStream> stream;
> +  rv = channel->Open2(getter_AddRefs(stream));

don't ever open channels with Open().  please use AsyncOpen() and an async consumer, even if that means to make this whole class API be async.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

(Reviewed in splinter.)

Comment 13

[Franziskus Kiefer [:franziskus]]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> Comment on attachment 8757280 [details] [diff] [review]
> rb53954.patch
> 
> Review of attachment 8757280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/ContentSignatureVerifier.cpp
> @@ +277,5 @@
> > +    return rv;
> > +  }
> > +
> > +  nsCOMPtr<nsIInputStream> stream;
> > +  rv = channel->Open2(getter_AddRefs(stream));
> 
> don't ever open channels with Open().  please use AsyncOpen() and an async
> consumer, even if that means to make this whole class API be async.

Well, this API can't be async as it's blocking everything (creation of the context and as such the entire page load). So I don't really see a point in making this async and then block on calling it. But if that's the preferred way, I can do it...

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #11)
> > Comment on attachment 8757280 [details] [diff] [review]
> > rb53954.patch
> > 
> > Review of attachment 8757280 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: security/manager/ssl/ContentSignatureVerifier.cpp
> > @@ +277,5 @@
> > > +    return rv;
> > > +  }
> > > +
> > > +  nsCOMPtr<nsIInputStream> stream;
> > > +  rv = channel->Open2(getter_AddRefs(stream));
> > 
> > don't ever open channels with Open().  please use AsyncOpen() and an async
> > consumer, even if that means to make this whole class API be async.
> 
> Well, this API can't be async as it's blocking everything (creation of the
> context and as such the entire page load). So I don't really see a point in
> making this async and then block on calling it. But if that's the preferred
> way, I can do it...

Calling Open() on a channel blocks the UI.  When the server or local net will respond slowly from various reasons, users will be very frustrated, don't you think?

This probably needs an overall design change then.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review52656

::: security/manager/ssl/ContentSignatureVerifier.h:50
(Diff revisions 1 - 3)
>    ~ContentSignatureVerifier();
>  
>    nsresult UpdateInternal(const nsACString& aData,
>                            MutexAutoLock& /*proofOfLock*/,
>                            const nsNSSShutDownPreventionLock& /*proofOfLock*/);
> -  nsresult DownloadCertChain(nsACString& aCertChain,
> +  nsresult DownloadCertChain(nsACString& aCertChain, /* out */

nit: it might be more clear to have the comma after the comment, but no big deal

::: security/manager/ssl/ContentSignatureVerifier.cpp:256
(Diff revisions 1 - 3)
>    // add data if we got any
>    return UpdateInternal(aData, lock, locker);
>  }
>  
>  nsresult
> -ContentSignatureVerifier::DownloadCertChain(nsACString& aCertChain,
> +ContentSignatureVerifier::DownloadCertChain(nsACString& aCertChain, /* out */

same idea

::: security/manager/ssl/ContentSignatureVerifier.cpp:286
(Diff revisions 1 - 3)
>    rv = channel->Open2(getter_AddRefs(stream));
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
>  
>    rv = NS_ConsumeStream(stream, UINT32_MAX, aCertChain);

Blocking the main thread to fetch this isn't a viable approach. Maybe CreateContextWithoutChain should take a callback that it calls when the certificate chain has been asynchronously fetched (and/or the fetch fails, which we should probably have a test for).

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://reviewboard.mozilla.org/r/53954/#review51940

> changed the name (also in the other functions).
> We don't use it yet with any data, but other users might want to use it. It would be the same as calling createContext and then update. So I think this is nicer. But if you think no one will ever use it we can drop it (from all createContexts then to be consistent).

I think that unless you have a specific future use in mind, if nothing in this patch uses it don't bother adding it. If something ever does need it, the interface can be changed.

Comment 17

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/3-4/

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: rb53954.patch

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8758281 [details] [diff] [review]
rb53954.patch

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

Approach looks good to me, now let's tackle the details.

::: dom/security/ContentVerifier.h
@@ +19,5 @@
>   * releases the request to the next listener if the verification is successful.
>   * If the verification fails or anything else goes wrong, a
>   * NS_ERROR_INVALID_SIGNATURE is thrown.
>   */
> +class ContentVerifier : public nsIStreamListener, public nsICSReceiverCallback

preserve the original formatting (each derived on its own line)

@@ +30,5 @@
>  
>    explicit ContentVerifier(nsIStreamListener* aMediatedListener,
>                             nsISupports* aMediatedContext)
>      : mNextListener(aMediatedListener)
> +    , mVerifier(nullptr) {}

no need to init mVerifier, it's comptr

@@ +47,5 @@
> +  nsCOMPtr<nsIContentSignatureVerifier> mVerifier;
> +  // holding a pointer to the request and context to resume/cancel it
> +  // not owning
> +  nsIRequest* mRequest;
> +  nsISupports* mContext;

why not strong references?

::: dom/security/ContentVerifier.cpp
@@ +137,4 @@
>  }
>  
> +NS_IMETHODIMP
> +ContentVerifier::NotifyCS(bool done)

no abbrevs in names please

@@ +145,5 @@
>    }
>  
> +  // In this case something went wrong with the cert. Let's stop this load.
> +  CSV_LOG(("failed to get a valid cert chain\n"));
> +  mRequest->Cancel(NS_ERROR_INVALID_SIGNATURE);

it might be better to cancel before resume

::: security/manager/ssl/ContentSignatureVerifier.h
@@ +75,5 @@
>    // signature to verify
>    nsCString mSignature;
> +  // x5u (X.509 URL) value pointing to pem cert chain
> +  nsCString mCertChainURL;
> +  // the downloaded chert chain to verify against

chert?

@@ +82,5 @@
>    mozilla::UniqueSECKEYPublicKey mKey;
> +  // name of the verifying context
> +  nsCString mName;
> +  // callback to notify when finished (not owning)
> +  nsICSReceiverCallback *mCallback;

must be held strongly! use nsCOMPtr

::: security/manager/ssl/ContentSignatureVerifier.cpp
@@ +274,5 @@
> +  nsCOMPtr<nsIChannel> channel;
> +  rv = NS_NewChannel(getter_AddRefs(channel), certChainURI,
> +                     nsContentUtils::GetSystemPrincipal(),
> +                     nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
> +                     nsIContentPolicy::TYPE_OTHER);

how should redirects be handled?  mainly redirects to different origins?

@@ +282,5 @@
> +
> +  nsCOMPtr<nsIInterfaceRequestor> listenerRequestor =
> +    do_QueryInterface(reinterpret_cast<nsISupports*>(this));
> +  NS_ENSURE_STATE(listenerRequestor);
> +  rv = channel->SetNotificationCallbacks(listenerRequestor);

I think you can just pass |this| directly

OTOH, what is the reason you are passing this class as notification callbacks to the channel?  please add a comment what interface is expected by the channel

@@ +293,5 @@
> +  if (priorityChannel) {
> +    priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_HIGHEST);
> +  }
> +
> +  rv = channel->AsyncOpen2(this);

don't we need a way to cancel this channel potentially?  is there a way to cancel this whole async signature verification process?

And I think you should keep a reference to the channel, just to not let it die.

or should it be added to the same loadgroup as the channel that initiates this signature verification?  something tells me it might be a good idea to do, unless you plan on some complex coalescing logic (however, HTTP cache should work here well)

also, it might be good (for a followup bug) to forward any cache related load flags from the channel that initiates the signature verification when we e.g. do a hard reload or so, which would reload the cert chain from a stubbornly caching origin server

@@ +326,5 @@
> +  mCallback = aCallback;
> +  mName.Assign(aName);
> +
> +  // We must download the cert chain now.
> +  // This is async and blocks creatContextInternal calls.

creat

@@ +346,5 @@
> +  if (mInitialised) {
> +    return NS_ERROR_ALREADY_INITIALIZED;
> +  }
> +  mInitialised = true;
> +  mDownloadedCertChain = true;

please add a comment why you are setting this flag here to true

@@ +387,5 @@
> +
> +  // If we didn't create the context yet, bail!
> +  if (!mDownloadedCertChain) {
> +    return NS_ERROR_FAILURE;
> +  }

This seems like something that should never happen, candidate more for MOZ_CRASH/ASSERT?  or is the consumer prepared to retry later?

@@ +448,5 @@
>        mSignature = directive->mValue;
>      }
> +    if (directive->mName.Length() == certChainURL_var.Length() &&
> +        directive->mName.EqualsIgnoreCase(certChainURL_var.get(),
> +                                          certChainURL_var.Length())) {

why exactly the check on length first?

@@ +480,5 @@
> +NS_IMETHODIMP
> +ContentSignatureVerifier::OnStartRequest(nsIRequest* aRequest,
> +                                         nsISupports* aContext)
> +{
> +  MutexAutoLock lock(mMutex);

why? :)

@@ +488,5 @@
> +NS_IMETHODIMP
> +ContentSignatureVerifier::OnStopRequest(nsIRequest* aRequest,
> +                                        nsISupports* aContext, nsresult aStatus)
> +{
> +  MutexAutoLock lock(mMutex);

why exactly are you so much locking this?  you expect some other thread canceling this class (not that i would see a way how to do it) or what?  for perf reasons it might be better to think whether the lock is needed over all of the nsIStreamListener methods

the only protected member here seems to be mCallback

@@ +498,5 @@
> +
> +  // We got the cert chain now. Let's create the context.
> +  if (NS_FAILED(CreateContextInternal(NS_LITERAL_CSTRING(""), certChain, mName,
> +                                      lock))) {
> +    mCallback->NotifyCS(false);

you also have to handle the case when aStatus is a failure
and also the case when you get 4XX or 5XX responses from the server

@@ +503,5 @@
> +    return NS_OK;
> +  }
> +
> +  mDownloadedCertChain = true;
> +  mCallback->NotifyCS(true);

please swap the callback to a local comptr first, then call on the local var (in all cases), that avoids any however improbable reentrancy mishmash and also safely releases the callback reference (that you are now missing)

@@ +517,5 @@
> +  MutexAutoLock lock(mMutex);
> +  nsAutoCString buffer;
> +
> +  nsresult rv = NS_ConsumeStream(aInputStream, aCount, buffer);
> +  if (rv != NS_BASE_STREAM_WOULD_BLOCK && NS_FAILED(rv)) {

you will never get NS_BASE_STREAM_WOULD_BLOCK here.  if this fails, then definitely for a fatal reason.

::: security/manager/ssl/nsIContentSignatureVerifier.idl
@@ +104,5 @@
> + * { 0x1eb90707, 0xdf59, 0x48b7, \
> + * { 0x9d, 0x42, 0xd8, 0xbf, 0x63, 0x0a, 0xe7, 0x44 } }
> + */
> +[scriptable, uuid(1eb90707-df59-48b7-9d42-d8bf630ae744)]
> +interface nsICSReceiverCallback : nsISupports

no "CS" abbreviations in the name please

@@ +106,5 @@
> + */
> +[scriptable, uuid(1eb90707-df59-48b7-9d42-d8bf630ae744)]
> +interface nsICSReceiverCallback : nsISupports
> +{
> +  void notifyCS(in boolean verified);

here as well

@@ +107,5 @@
> +[scriptable, uuid(1eb90707-df59-48b7-9d42-d8bf630ae744)]
> +interface nsICSReceiverCallback : nsISupports
> +{
> +  void notifyCS(in boolean verified);
> +};

comments what this is, who is using it, hints on who is/are implementer/s, what is the lifetime, the more the better...  and I'm really getting tired of repeating myself about telling people to add comments on their new idls...

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

f+ in splinter.

Comment 21

[Franziskus Kiefer [:franziskus]]

Thanks for your feedback. A new version will be up shortly with hopefully most points addressed. Some comments below

> why exactly are you so much locking this?  you expect some other thread canceling this class (not that i would see a way how to do it) or what?  for perf reasons it might be better to think whether the lock is needed over all of the nsIStreamListener methods the only protected member here seems to be mCallback

We're calling CreateContextInternal in here, which requires a lock.

> >        mSignature = directive->mValue;
> >      }
> > +    if (directive->mName.Length() == certChainURL_var.Length() &&
> > +        directive->mName.EqualsIgnoreCase(certChainURL_var.get(),
> > +                                          certChainURL_var.Length())) {
>  why exactly the check on length first?

If the length doesn't match, the strings can't match. So a tiny little bit more efficient (and we did this way before so I just c/p).

> don't we need a way to cancel this channel potentially?  is there a way to cancel this whole async signature verification process?

hm, not sure if we can cancel this one. Can we cancel a channel while it is suspended? Any caller that's loading something itself has to be suspended while fetching the cert chain.

>or should it be added to the same loadgroup as the channel that initiates this signature verification?  something tells me it might be a good idea to do, unless you plan on some complex coalescing logic (however, HTTP cache should work here well)

In this case it might be a good idea. But since this is a generic interface I'm not sure we should do it. The caller could be anything.

> also, it might be good (for a followup bug) to forward any cache related load flags from the channel that initiates the signature verification when we e.g. do a hard reload or so, which would reload the cert chain from a stubbornly caching origin server

yeah, I thought we're relying on the HTTP cache for now and see if we have to do something about caching later if it's too bad.

Comment 22

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/4-5/

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: rb53954.patch

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review53626

please next time check that you have addressed or somehow responded to all the review comments.

you didn't answer to the redirect handling issue question.  if you don't know the answer, we can start a wider discussion.

::: dom/security/ContentVerifier.cpp:84
(Diff revisions 4 - 5)
>    // If this fails, we return an invalid signature error to load a fallback page.
>    // If everthing is good, we return a new stream to the next listener and kick
>    // that one of.
>    bool verified = false;
> -  nsresult rv = mVerifier->End(&verified);
> -  if (NS_FAILED(rv) || !verified || NS_FAILED(aStatus)) {
> +  nsresult rv;
> +  if (NS_FAILED(aStatus) || NS_FAILED(mVerifier->End(&verified)) || !verified) {

and http response code checking?

don't you have to call End on the verifier to release the NSS resources even in the NS_FAILED(aStatus) case?  or is it released soon after and all is done in dtor?

::: security/manager/ssl/ContentSignatureVerifier.cpp:282
(Diff revisions 4 - 5)
>                       nsIContentPolicy::TYPE_OTHER);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
>  
> -  nsCOMPtr<nsIInterfaceRequestor> listenerRequestor =
> +  // this implements the nsIStreamListener interface to read the channel's content

and that is the reason to add is notification callbacks?  that doesn't make sense.  nsIStreamListener is passed to AsyncOpen, that is enough to get onstart/etc calls.

so, why exactly are you having this class as notification callbacks on the channel?

::: security/manager/ssl/ContentSignatureVerifier.cpp:388
(Diff revisions 4 - 5)
>    }
>    MutexAutoLock lock(mMutex);
>  
>    // If we didn't create the context yet, bail!
>    if (!mDownloadedCertChain) {
> +    MOZ_ASSERT(0);

MOZ_ASSERT(false, "some explanatory text")

please

::: security/manager/ssl/ContentSignatureVerifier.cpp:411
(Diff revisions 4 - 5)
>      return NS_ERROR_FAILURE;
>    }
>  
>    // If we didn't create the context yet, bail!
>    if (!mDownloadedCertChain) {
> +    MOZ_ASSERT(0);

here as well

::: security/manager/ssl/ContentSignatureVerifier.cpp:491
(Diff revisions 4 - 5)
>  
>  NS_IMETHODIMP
>  ContentSignatureVerifier::OnStopRequest(nsIRequest* aRequest,
>                                          nsISupports* aContext, nsresult aStatus)
>  {
>    MutexAutoLock lock(mMutex);

you didn't answer why you are locking so much here.

why?

::: security/manager/ssl/ContentSignatureVerifier.cpp:502
(Diff revisions 4 - 5)
>  
>    // We got the cert chain now. Let's create the context.
> -  if (NS_FAILED(CreateContextInternal(NS_LITERAL_CSTRING(""), certChain, mName,
> +  if (NS_FAILED(aStatus) ||
> +      NS_FAILED(CreateContextInternal(NS_LITERAL_CSTRING(""), certChain, mName,
>                                        lock))) {
> -    mCallback->NotifyCS(false);
> +    mCallback->Notify(false);

you didn't address the local comptr swap here.

::: security/manager/ssl/nsIContentSignatureVerifier.idl:107
(Diff revisions 4 - 5)
>   * Callback for nsIContentSignatureVerifier.
>   * { 0x1eb90707, 0xdf59, 0x48b7, \
> - * { 0x9d, 0x42, 0xd8, 0xbf, 0x63, 0x0a, 0xe7, 0x44 } }
> + *   { 0x9d, 0x42, 0xd8, 0xbf, 0x63, 0x0a, 0xe7, 0x44 } }
>   */
>  [scriptable, uuid(1eb90707-df59-48b7-9d42-d8bf630ae744)]
>  interface nsICSReceiverCallback : nsISupports

nsIContentSignatureReceiverCallback please.

::: security/manager/ssl/nsIContentSignatureVerifier.idl:113
(Diff revisions 4 - 5)
>  {
> -  void notifyCS(in boolean verified);
> +  /**
> +   * Notification callback that's called by nsIContentSignatureVerifier when
> +   * the cert chain is downloaded.
> +   * If download and initialisation were successful, verified is true,
> +   * other false. If verified is false, the verification must be aborted.

otherwise?

::: security/manager/ssl/nsIContentSignatureVerifier.idl:115
(Diff revisions 4 - 5)
> +   * Notification callback that's called by nsIContentSignatureVerifier when
> +   * the cert chain is downloaded.
> +   * If download and initialisation were successful, verified is true,
> +   * other false. If verified is false, the verification must be aborted.
> +   */
> +  void notify(in boolean verified);

please try to find a bit more descriptive name for this method, "Notify" is too general IMO

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8758771 [details] [diff] [review]
rb53954.patch

(done in rb this time)

Comment 26

[Franziskus Kiefer [:franziskus]]

(In reply to Honza Bambas (:mayhemer) from comment #24)
> Comment on attachment 8754401 [details]
> MozReview Request: Bug 1263793 - Using content signature verifier for
> verifying remote newtab, r=keeler,mayhemer
> 
> https://reviewboard.mozilla.org/r/53954/#review53626
> 
> please next time check that you have addressed or somehow responded to all
> the review comments.

oh, I missed that indeed, sorry! Using reviewboard would make things easier :)
I hop I didn't miss anything this time.

> how should redirects be handled?  mainly redirects to different origins?
What exactly would I have to do to handle redirects? (I was hoping that's transparent) Currently I wouldn't expect any here (as we control the other end), but we should definitely be able to handle them. Ideally this should follow redirects and we read the final content the same way we do right now.

> ::: dom/security/ContentVerifier.cpp:84
> (Diff revisions 4 - 5)
> >    // If this fails, we return an invalid signature error to load a fallback page.
> >    // If everthing is good, we return a new stream to the next listener and kick
> >    // that one of.
> >    bool verified = false;
> > -  nsresult rv = mVerifier->End(&verified);
> > -  if (NS_FAILED(rv) || !verified || NS_FAILED(aStatus)) {
> > +  nsresult rv;
> > +  if (NS_FAILED(aStatus) || NS_FAILED(mVerifier->End(&verified)) || !verified) {
> 
> and http response code checking?

I don't think that's necessary (actually not even the status check imho). If we're missing a single bit from the content, the verifications fails anyway. So if the verification succeeds we should be fine, right?

> don't you have to call End on the verifier to release the NSS resources even
> in the NS_FAILED(aStatus) case?  or is it released soon after and all is
> done in dtor?

Yep, the ContentSignatureVerifier destructor cleans up the NSS resources. If the load's done, the ContentVerifier gets destroyed and with that the ContentSignatureVerifier, which releases all NSS resources at that point. I don't think it's necessary to invoke that manually.

> ::: security/manager/ssl/ContentSignatureVerifier.cpp:282
> (Diff revisions 4 - 5)
> >                       nsIContentPolicy::TYPE_OTHER);
> >    if (NS_FAILED(rv)) {
> >      return rv;
> >    }
> >  
> > -  nsCOMPtr<nsIInterfaceRequestor> listenerRequestor =
> > +  // this implements the nsIStreamListener interface to read the channel's content
> 
> and that is the reason to add is notification callbacks?  that doesn't make
> sense.  nsIStreamListener is passed to AsyncOpen, that is enough to get
> onstart/etc calls.
> 
> so, why exactly are you having this class as notification callbacks on the
> channel?

oh, I didn't know that. Removed it, we're fine with passing it to AsyncOpen then.
 
> ::: security/manager/ssl/ContentSignatureVerifier.cpp:491
> (Diff revisions 4 - 5)
> >  
> >  NS_IMETHODIMP
> >  ContentSignatureVerifier::OnStopRequest(nsIRequest* aRequest,
> >                                          nsISupports* aContext, nsresult aStatus)
> >  {
> >    MutexAutoLock lock(mMutex);
> 
> you didn't answer why you are locking so much here.

I wrote
> We're calling CreateContextInternal in here, which requires a lock.
So we have to lock here unless we're changing CreateContextInternal. But I don't think we should do that (as discussed in bug 1252882). The lock ensures that no one uses this as a service and accesses any of its functionality concurrently.  (CreateContextInternal writes mCx and mKey.)

Comment 27

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/5-6/

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #26)
> (In reply to Honza Bambas (:mayhemer) from comment #24)
> > Comment on attachment 8754401 [details]
> > MozReview Request: Bug 1263793 - Using content signature verifier for
> > verifying remote newtab, r=keeler,mayhemer
> > 
> > https://reviewboard.mozilla.org/r/53954/#review53626
> > 
> > please next time check that you have addressed or somehow responded to all
> > the review comments.
> 
> oh, I missed that indeed, sorry! Using reviewboard would make things easier
> :)

rb makes some other things much more complicated ;)  and there was a clear bugzilla comment from the splinter review.

> I hop I didn't miss anything this time.
> 
> > how should redirects be handled?  mainly redirects to different origins?
> What exactly would I have to do to handle redirects? (I was hoping that's
> transparent) Currently I wouldn't expect any here (as we control the other
> end), but we should definitely be able to handle them. Ideally this should
> follow redirects and we read the final content the same way we do right now.

my concern is more about to _not_ follow them as we/the origin server could be under an attack and we being forwarded to a malicious server potentially.  

normally redirects will of course follow.  depends on the security settings tho (I'm no deep expert) what exactly happens e.g. in case of a cross-origin redirect.  something I feel we may want to disallow.  this needs a security review IMO.

> 
> > ::: dom/security/ContentVerifier.cpp:84
> > (Diff revisions 4 - 5)
> > >    // If this fails, we return an invalid signature error to load a fallback page.
> > >    // If everthing is good, we return a new stream to the next listener and kick
> > >    // that one of.
> > >    bool verified = false;
> > > -  nsresult rv = mVerifier->End(&verified);
> > > -  if (NS_FAILED(rv) || !verified || NS_FAILED(aStatus)) {
> > > +  nsresult rv;
> > > +  if (NS_FAILED(aStatus) || NS_FAILED(mVerifier->End(&verified)) || !verified) {
> > 
> > and http response code checking?
> 
> I don't think that's necessary (actually not even the status check imho). If
> we're missing a single bit from the content, the verifications fails anyway.
> So if the verification succeeds we should be fine, right?

hmm.. still I think you should only proceed when you get a 200 response, because you never know what content you get in a 50X or 40X page.  it could also be a way to perform an attack: mangle with error pages on the server and return a malicious (attackers) certchain so that malicious content may end up treated as a trusted  signed content.

> 
> > don't you have to call End on the verifier to release the NSS resources even
> > in the NS_FAILED(aStatus) case?  or is it released soon after and all is
> > done in dtor?
> 
> Yep, the ContentSignatureVerifier destructor cleans up the NSS resources. If
> the load's done, the ContentVerifier gets destroyed and with that the
> ContentSignatureVerifier, which releases all NSS resources at that point. I
> don't think it's necessary to invoke that manually.

OK.  please add a comment that the verifier is released and destroyed soon after this point or call End() on it regardless the result.

> 
> > ::: security/manager/ssl/ContentSignatureVerifier.cpp:282
> > (Diff revisions 4 - 5)
> > >                       nsIContentPolicy::TYPE_OTHER);
> > >    if (NS_FAILED(rv)) {
> > >      return rv;
> > >    }
> > >  
> > > -  nsCOMPtr<nsIInterfaceRequestor> listenerRequestor =
> > > +  // this implements the nsIStreamListener interface to read the channel's content
> > 
> > and that is the reason to add is notification callbacks?  that doesn't make
> > sense.  nsIStreamListener is passed to AsyncOpen, that is enough to get
> > onstart/etc calls.
> > 
> > so, why exactly are you having this class as notification callbacks on the
> > channel?
> 
> oh, I didn't know that. Removed it, we're fine with passing it to AsyncOpen
> then.

OK.

>  
> > ::: security/manager/ssl/ContentSignatureVerifier.cpp:491
> > (Diff revisions 4 - 5)
> > >  
> > >  NS_IMETHODIMP
> > >  ContentSignatureVerifier::OnStopRequest(nsIRequest* aRequest,
> > >                                          nsISupports* aContext, nsresult aStatus)
> > >  {
> > >    MutexAutoLock lock(mMutex);
> > 
> > you didn't answer why you are locking so much here.
> 
> I wrote
> > We're calling CreateContextInternal in here, which requires a lock.
> So we have to lock here unless we're changing CreateContextInternal. But I
> don't think we should do that (as discussed in bug 1252882). The lock
> ensures that no one uses this as a service and accesses any of its
> functionality concurrently.  (CreateContextInternal writes mCx and mKey.)

Aha!  I think I understand now.  you treat stream listener method as any other public method of the verifier.  ok, got it.

(OTOH, to make sure that this is not used as a service - no concurrency - is to make CreateContext* public methods return a new object (with a new interface and not registered as a component!) that does the job then (creates the context, updates it, spits the result).  something I was suggesting a long ago when this class started to be modified for this signed content enterprise.  Not something I would require for r+ here, tho, don't worry ;)  I already gave up on that point anyway.)

Comment 29

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review53944

::: security/manager/ssl/ContentSignatureVerifier.cpp:382
(Diff revisions 5 - 6)
>    }
>    MutexAutoLock lock(mMutex);
>  
>    // If we didn't create the context yet, bail!
>    if (!mDownloadedCertChain) {
> -    MOZ_ASSERT(0);
> +    MOZ_ASSERT(0, "Someone called ContentSignatureVerifier::Update before "

MOZ_ASSERT(false, ...)

::: security/manager/ssl/ContentSignatureVerifier.cpp:500
(Diff revisions 5 - 6)
>  
>    // We got the cert chain now. Let's create the context.
>    if (NS_FAILED(aStatus) ||
>        NS_FAILED(CreateContextInternal(NS_LITERAL_CSTRING(""), certChain, mName,
>                                        lock))) {
> -    mCallback->Notify(false);
> +    callback->ContextCreated(false);

nit: a bit nicer would be to:

if (NS_FAILED(aStatus)) {
  callback->ContextCreated(false);
  return NS_OK;
}

rv = CreateContextInternal(NS_LITERAL_CSTRING(""), certChain, mName, lock);
if (NS_FAILED(rv)) {
  callback->ContextCreated(false);
  return NS_OK;
}

etc.

please add the check for 200 status here.

::: security/manager/ssl/nsIContentSignatureVerifier.idl:115
(Diff revisions 5 - 6)
>     * Notification callback that's called by nsIContentSignatureVerifier when
>     * the cert chain is downloaded.
> -   * If download and initialisation were successful, verified is true,
> -   * other false. If verified is false, the verification must be aborted.
> +   * If download and initialisation were successful, successful is true,
> +   * otherwise false. If successful is false, the verification must be aborted.
>     */
> -  void notify(in boolean verified);
> +  void contextCreated(in boolean successful);

like this!  makes much more sense.  thanks.

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

(f+ comment 29)

Comment 31

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(Just as a coordination heads-up: I'm aware of this review request, but I'll wait to add any comments until Honza has given an r+ so we don't cross any signals.)

Comment 32

[Franziskus Kiefer [:franziskus]]

> my concern is more about to _not_ follow them as we/the origin server could be under an attack and we being forwarded to a malicious server potentially.  
> normally redirects will of course follow.  depends on the security settings tho (I'm no deep expert) what exactly happens e.g. in case of a cross-origin redirect.  something I feel we may want to disallow.  this needs a security review IMO.

Ok. I think it should be fine to follow redirects for cert downloads. But let's see what David says to this.

> hmm.. still I think you should only proceed when you get a 200 response, because you never know what content you get in a 50X or 40X page.  it could also be a way to perform an attack: mangle with error pages on the server and return a malicious (attackers) certchain so that malicious content may end up treated as a trusted  signed content.

If the attacker can return a different cert chain, he can send any contain he likes (with a 200 response). So I don't think this is necessary. But it doesn't hurt. I added a check.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #31)
> (Just as a coordination heads-up: I'm aware of this review request, but I'll
> wait to add any comments until Honza has given an r+ so we don't cross any
> signals.)

I hope we're there soon. But you could also say something to the redirects.

Comment 33

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/6-7/

Comment 34

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #32)
> > my concern is more about to _not_ follow them as we/the origin server could be under an attack and we being forwarded to a malicious server potentially.  
> > normally redirects will of course follow.  depends on the security settings tho (I'm no deep expert) what exactly happens e.g. in case of a cross-origin redirect.  something I feel we may want to disallow.  this needs a security review IMO.
> 
> Ok. I think it should be fine to follow redirects for cert downloads. But
> let's see what David says to this.

I don't think it's a security concern. If we trust the server enough to send us a cert chain, we probably trust it enough to send a redirect. If it's been compromised and an attacker gets us to download either a bogus chain directly from the server or through a redirect, it doesn't matter anyway because there's only one root we trust, and a bogus chain won't verify to that root (unless we've really messed up and signed something we shouldn't have, in which case we've already lost). It would probably be a good idea to be sure that everything is over https, though, if we aren't already.

Also, another thing I was going to mention: can we make the request for the cert chain in parallel with the request for the content? (and only verify the signature after we have all the data we need). I think that would be more efficient than blocking downloading the content on receiving the chain.

Comment 35

[Franziskus Kiefer [:franziskus]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #34)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #32)
> It would probably be a good idea to
> be sure that everything is over https, though, if we aren't already.

right, I'll do that...

> Also, another thing I was going to mention: can we make the request for the
> cert chain in parallel with the request for the content? (and only verify
> the signature after we have all the data we need). I think that would be
> more efficient than blocking downloading the content on receiving the chain.

I agree, that would be nicer. I'm blocking here because I was asked to not buffer the page in the original design and we can't create a verifying context without a key. Given those restrictions we have to block the load until we fetched the cert chain. If you're both ok with buffering the page content (outside of the verifying context) while downloading the cert chain we can get concurrency. (Everything else would require a new NSS interface, which probably won't happen anytime soon.)

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #35)
> If you're both ok with buffering the page
> content (outside of the verifying context) while downloading the cert chain
> we can get concurrency. (Everything else would require a new NSS interface,
> which probably won't happen anytime soon.)

Good point David.  Buffering means memory consumption, but gains first-load performance, which is something we really should do.

Franziskus, if you can, please limit the pre-buffering to only some amount of data (under a preference, preferably).  Let's start with just 500kb or so.  When you reach this amount of buffered data, just suspend the content loading channel.  Resume on when you receive the cert chain and create the context (in your new callback).  If you tho believe the content to check the signature will in most (99.9%) cases be smaller than those suggested 500k limit, then just go with dumb buffering and no limit.

Comment 37

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I'll defer to Honza on the trade-offs between buffering and suspending the content.

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review55310

::: security/manager/ssl/ContentSignatureVerifier.cpp:491
(Diff revisions 6 - 7)
>  {
>    MutexAutoLock lock(mMutex);
>    nsAutoCString certChain;
>    nsCOMPtr<nsIContentSignatureReceiverCallback> callback;
>    callback.swap(mCallback);
> +  uint32_t code;

s/code/httpResponseCode/

::: security/manager/ssl/ContentSignatureVerifier.cpp:492
(Diff revisions 6 - 7)
>    MutexAutoLock lock(mMutex);
>    nsAutoCString certChain;
>    nsCOMPtr<nsIContentSignatureReceiverCallback> callback;
>    callback.swap(mCallback);
> +  uint32_t code;
> +  nsresult rv;

nit: except nsresult rv (which should be left as the first declaration in the whole method) please move each local var to the place of its first use.

the callback swap is good to be left at the top, tho.  it will be released sooner (always good)

::: security/manager/ssl/ContentSignatureVerifier.cpp:506
(Diff revisions 6 - 7)
>  
>    for (uint32_t i = 0; i < mCertChain.Length(); ++i) {
>      certChain.Append(mCertChain[i]);
>    }
>  
> +  if (NS_FAILED(aStatus)) {

maybe do this check before you join the certchain?

Comment 39

[Franziskus Kiefer [:franziskus]]

Thanks for the review Honza!
I'm OOO til London, so not sure when I'll get to this. But looks like it's only the concurrency (the newtab page is pretty small so I think we should be fine with the naive approach) and https enforcement in addition to your comments.

Comment 40

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/7-8/

Comment 41

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/8-9/

Comment 42

[Franziskus Kiefer [:franziskus]]

looks like reviewboard didn't seend out r? :/

Comment 43

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review57688

Ok - lgtm with comments addressed. Also, in the future feel free to ping me if I don't respond to a review in about a business day.

::: dom/security/ContentVerifier.cpp:64
(Diff revision 9)
>    *outWrittenCount = aCount;
>    return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> -ContentVerifier::OnStartRequest(nsIRequest* aRequest, nsISupports* aContext)
> +nsresult
> +ContentVerifier::FinishSignature()

It would be great if the functions in this file were in order of expected or common execution, but maybe that's a cleanup you can do later.

::: dom/security/ContentVerifier.cpp:69
(Diff revision 9)
> -                               nsresult aStatus)
>  {
>    // Verify the content:
>    // If this fails, we return an invalid signature error to load a fallback page.
>    // If everthing is good, we return a new stream to the next listener and kick
>    // that one of.

typo nit: "off"

::: dom/security/ContentVerifier.cpp:78
(Diff revision 9)
> -      rv = aStatus;
> -    } else {
> +  // ContentSignatureVerifier on destruction.
> +  if (NS_FAILED(mVerifier->End(&verified)) || !verified) {
> -      rv = NS_ERROR_INVALID_SIGNATURE;
> -    }
>      CSV_LOG(("failed to verify content\n"));
> -    mNextListener->OnStartRequest(aRequest, aContext);
> +    mNextListener->OnStopRequest(mRequest, mContext, rv);

rv is NS_OK here - is that correct? Should it be the result of calling mVerifier->End? (Or NS_ERROR_FAILURE or NS_ERROR_INVALID_SIGNATURE if End returns NS_OK but verified is false?)

::: dom/security/ContentVerifier.cpp:83
(Diff revision 9)
>    CSV_LOG(("Successfully verified content signature.\n"));
>  
> -  // start the next listener
> -  rv = mNextListener->OnStartRequest(aRequest, aContext);
> -  if (NS_SUCCEEDED(rv)) {
> -    // We emptied aInStr so we have to create a new one from buf to hand it
> +  // We emptied aInStr so we have to create a new one from buf to hand it

nit: update comment (what is aInStr?)

::: dom/security/ContentVerifier.cpp:121
(Diff revision 9)
> -  }
>  
> -  // Base 64 decode the signature
> -  ScopedSECItem rawSignatureItem(::SECITEM_AllocItem(nullptr, nullptr, 0));
> -  if (!rawSignatureItem ||
> -      !NSSBase64_DecodeBuffer(nullptr, rawSignatureItem,
> +  if (NS_FAILED(aStatus)) {
> +    nsresult rv = NS_OK;
> +    CSV_LOG(("Stream failed\n"));
> +    mNextListener->OnStopRequest(aRequest, aContext, rv);

Maybe this is just a confusing API I'm not familiar with, but why are we passing NS_OK to the next listener if we're in a failure branch?

::: dom/security/ContentVerifier.cpp:156
(Diff revision 9)
> -  // add the prefix to the verification buffer
> +  return NS_OK;
> -  return Update(kPREFIX);
>  }
>  
> -/**
> - * Add data to the context that should be verified.
> +NS_IMETHODIMP
> +ContentVerifier::ContextCreated(bool successful)

All of the operations on this class are expected to be on the main thread, right? I think we should assert that in each public function.

::: dom/security/ContentVerifier.cpp:172
(Diff revision 9)
> -    if (VFY_Update(mCx.get(),
> -                   (const unsigned char*)nsPromiseFlatCString(aData).get(),
> -                   aData.Length()) != SECSuccess) {
> -      return NS_ERROR_INVALID_SIGNATURE;
> +  // the buffered content.
> +  mContextCreated = true;
> +  for (size_t i = 0; i < mContent.Length(); ++i) {
> +    if (NS_FAILED(mVerifier->Update(mContent[i]))) {
> +      // Bail out if this fails. We can't return an error here, but if this
> +      // faild, NS_ERROR_INVALID_SIGNATURE is returned in FinishSignature.

typo nit: "failed"

::: security/manager/ssl/ContentSignatureVerifier.cpp:280
(Diff revision 9)
> +  nsresult rv = NS_NewURI(getter_AddRefs(certChainURI), mCertChainURL);
> +  if (NS_FAILED(rv) || !certChainURI) {
> +    return rv;
> +  }
> +
> +  /* If the address is not https, fail. */

nit: //-style comment

::: security/manager/ssl/ContentSignatureVerifier.cpp:394
(Diff revision 9)
>    }
>    MutexAutoLock lock(mMutex);
> +
> +  // If we didn't create the context yet, bail!
> +  if (!mDownloadedCertChain) {
> +    MOZ_ASSERT(false, "Someone called ContentSignatureVerifier::Update before "

nit: maybe MOZ_ASSERT_UNREACHABLE("...") would be best here

::: security/manager/ssl/ContentSignatureVerifier.cpp:418
(Diff revision 9)
>      return NS_ERROR_FAILURE;
>    }
>  
> +  // If we didn't create the context yet, bail!
> +  if (!mDownloadedCertChain) {
> +    MOZ_ASSERT(false, "Someone called ContentSignatureVerifier::End before "

nit: same

::: security/manager/ssl/ContentSignatureVerifier.cpp:432
(Diff revision 9)
>  
>  nsresult
>  ContentSignatureVerifier::ParseContentSignatureHeader(
>    const nsACString& aContentSignatureHeader)
>  {
> +  MutexAutoLock lock(mMutex);

It seems like ContentSignatureVerifier is main thread-only now (unless I'm misunderstanding how the necko callbacks work). If that's the case, we should remove mMutex and just assert that we're on the main thread in each public function. If that's not the case, we should actually acquire the lock in every public function (passing it through to private functions as a proof if necessary) and assert we're on whatever thread we're expecting to be on.

::: security/manager/ssl/ContentSignatureVerifier.cpp:530
(Diff revision 9)
> +  if (NS_FAILED(rv)) {
> +    callback->ContextCreated(false);
> +    return NS_OK;
> +  }
> +
> +  mDownloadedCertChain = true;

This doesn't seem to be protected by mMutex here (although see my comment about this possibly being main thread-only).

Comment 44

[Honza Bambas (:mayhemer)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #42)
> looks like reviewboard didn't seend out r? :/

And what is it you ask me?

Comment 45

[Honza Bambas (:mayhemer)]

Please:
- add comments to any newly added members (in content ContentVerifier, any other, I didn't go through the patch)
- explain what has changed since the last review (I think ver7 of the patch)

then resubmit and ask again for review.

Comment 46

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/9-10/

Comment 47

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

see commit message for description

Comment 48

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review58126

r-, issues still found.

next time, the overall patch description should explain the overall structure of the patch, the class relations, their life-paths, cross references (who references who, who waits for who).  it's easy to get confused what ContentVerifier and ContentSignatureVerifier actually do.  I'll definitely forget that till next review round..

every idiff description should also say what exactly has changed in that structure (best also why)

::: dom/security/ContentVerifier.h:56
(Diff revisions 7 - 10)
>    nsCOMPtr<nsIContentSignatureVerifier> mVerifier;
>    // holding a pointer to the request and context to resume/cancel it
>    nsCOMPtr<nsIRequest> mRequest;
>    nsCOMPtr<nsISupports> mContext;
> +  // semaphors to indicate that the verifying context was created, the entire
> +  // content was read resp.

your comment says nothing really useful.  it should say why the flags are here and what effect and purpose they have.

::: dom/security/ContentVerifier.cpp:57
(Diff revisions 7 - 10)
>                    const char* aRawSegment, uint32_t aToOffset, uint32_t aCount,
>                    uint32_t* outWrittenCount)
>  {
>    FallibleTArray<nsCString>* decodedData =
>      static_cast<FallibleTArray<nsCString>*>(aClosure);
>    nsAutoCString segment(aRawSegment, aCount);

didn't spot this first time:  please use nsDependentCString instead.  this does a copy of the segment 2 times...

::: dom/security/ContentVerifier.cpp:109
(Diff revisions 7 - 10)
> +}
> +
> +NS_IMETHODIMP
> +ContentVerifier::OnStartRequest(nsIRequest* aRequest, nsISupports* aContext)
> +{
> +  mRequest = aRequest;

what exactly is purpose of these assignments?

::: dom/security/ContentVerifier.cpp:119
(Diff revisions 7 - 10)
> +NS_IMETHODIMP
> +ContentVerifier::OnStopRequest(nsIRequest* aRequest, nsISupports* aContext,
> +                               nsresult aStatus)
> +{
> +  mRequest = aRequest;
> +  mContext = aContext;

and here as well?

::: dom/security/ContentVerifier.cpp:123
(Diff revisions 7 - 10)
> +  mRequest = aRequest;
> +  mContext = aContext;
> +
> +  if (NS_FAILED(aStatus)) {
> +    CSV_LOG(("Stream failed\n"));
> +    return mNextListener->OnStopRequest(aRequest, aContext, aStatus);

to make sure you don't call OnStopRequest more than once, throw mNextListener away.  please first swap mNextListener to a local nsCOMPtr, then use it to call on it ; everywhere you now call directly on mNextListener.  in general, get rid of any references you no longer need ASAP.

::: netwerk/protocol/http/nsHttpChannel.cpp:1088
(Diff revisions 7 - 10)
> +        rv = ProcessContentSignatureHeader(mResponseHead);
> +        if (NS_FAILED(rv)) {
> +            LOG(("Content-signature verification failed.\n"));
> +            return rv;
> +        }
> +    }

I think it's very important to note somewhere that ContentVerifier expects OnStartRequest has already been called on the target listener before ContentVerifier has being injected.

::: security/manager/ssl/ContentSignatureVerifier.cpp:186
(Diff revisions 7 - 10)
>                            CertPolicyId::anyPolicy,
>                            nullptr/*stapledOCSPResponse*/);
>    if (result != Success) {
> -    // the chain is bad
> +    // if there was a library error, return an appropriate error
> +    if (IsFatalError(result)) {
> +      return NS_ERROR_FAILURE;

nit: isn't there some kind of mapping function between Result and nsresult?

::: dom/security/ContentVerifier.h:52
(Diff revision 10)
> +  FallibleTArray<nsCString> mContent;
>    // content and next listener for nsIStreamListener
>    nsCOMPtr<nsIStreamListener> mNextListener;
> +  // the verifier
> +  nsCOMPtr<nsIContentSignatureVerifier> mVerifier;
> +  // holding a pointer to the request and context to resume/cancel it

maybe say what kind of request it is (that it's request for the content)

::: dom/security/ContentVerifier.h:53
(Diff revision 10)
>    // content and next listener for nsIStreamListener
>    nsCOMPtr<nsIStreamListener> mNextListener;
> +  // the verifier
> +  nsCOMPtr<nsIContentSignatureVerifier> mVerifier;
> +  // holding a pointer to the request and context to resume/cancel it
> +  nsCOMPtr<nsIRequest> mRequest;

this should be renamed to mContentRequest (and mContentRequestContext)

::: dom/security/ContentVerifier.cpp:82
(Diff revision 10)
> -      rv = NS_ERROR_INVALID_SIGNATURE;
> -    }
>      CSV_LOG(("failed to verify content\n"));
> -    mNextListener->OnStartRequest(aRequest, aContext);
> +    mNextListener->OnStopRequest(mRequest, mContext, NS_ERROR_INVALID_SIGNATURE);
> -    mNextListener->OnStopRequest(aRequest, aContext, rv);
>      return NS_ERROR_INVALID_SIGNATURE;

where goes this result?  should the method rather be void?

::: dom/security/ContentVerifier.cpp:163
(Diff revision 10)
>  {
> -  nsNSSShutDownPreventionLock locker;
> -  if (isAlreadyShutDown()) {
> -    return NS_ERROR_INVALID_SIGNATURE;
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!successful) {
> +    // In this case something went wrong with the cert. Let's stop this load.
> +    CSV_LOG(("failed to get a valid cert chain\n"));
> +    mRequest->Cancel(NS_ERROR_INVALID_SIGNATURE);

what if mRequest is still null (you didn't get OnStartRequest)?

I though you kept the content request channel from the very start?  your code is very confusing in this and I sense problems here.

::: dom/security/ContentVerifier.cpp:164
(Diff revision 10)
> -  nsNSSShutDownPreventionLock locker;
> -  if (isAlreadyShutDown()) {
> -    return NS_ERROR_INVALID_SIGNATURE;
> +  MOZ_ASSERT(NS_IsMainThread());
> +  if (!successful) {
> +    // In this case something went wrong with the cert. Let's stop this load.
> +    CSV_LOG(("failed to get a valid cert chain\n"));
> +    mRequest->Cancel(NS_ERROR_INVALID_SIGNATURE);
> +    mNextListener->OnStopRequest(mRequest, mContext, NS_ERROR_INVALID_SIGNATURE);

You can call mNextListener->OnStopRequest twice: when the content load fails and here when the cert chain download or ctx creation fails.

The call to mRequest->Cancel will actually trigger ContentVerifier::OnStopRequest with the error, so you will definitely call OnStopRequest on the next listener twice.  

That is illegal.  Dropping mNextListener after first use and non-null checks before calls will do the job.

::: security/manager/ssl/ContentSignatureVerifier.h:71
(Diff revision 10)
>    nsresult ParseContentSignatureHeader(const nsACString& aContentSignatureHeader);
>  
>    // verifier context for incremental verifications
>    mozilla::UniqueVFYContext mCx;
> +  bool mInitialised;
> +  bool mDownloadedCertChain;

I think this member should be renamed to mHasCertChain (because you may sinthesize it to true under certain condition w/o any download involved)

and please add a good comment what this member does.

::: security/manager/ssl/nsIContentSignatureVerifier.idl:81
(Diff revision 10)
> +   *                                encoded, and the x5u value.
> +   * @param aName                   The (host)name for which the end entity must
> +                                    be valid.
> +   */
> +  void createContextWithoutChain(in nsIContentSignatureReceiverCallback aCallback,
> +                                 in ACString aContentSignatureHeader,

nit: createContextWithout*Cert*Chain

Comment 49

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

rb is so stupid at syncing with bz..

Comment 50

[Franziskus Kiefer [:franziskus]]

(In reply to Honza Bambas (:mayhemer) from comment #48)
> Comment on attachment 8754401 [details]
> Bug 1263793 - Using content signature verifier for verifying remote newtab,
> 
> next time, the overall patch description should explain the overall
> structure of the patch, the class relations, their life-paths, cross
> references (who references who, who waits for who).  it's easy to get
> confused what ContentVerifier and ContentSignatureVerifier actually do. 
> I'll definitely forget that till next review round..

Ok, here a short walk-through again on what this does. Changes to the previous patch are mentioned below and in the commit message.

ContentVerifier is used by nsHttpChannel to decide whether a content signature is valid or not. This takes only place when the according verify signed content flag in the loadInfo is set (which is only the case for remote newtab at the moment). ContentVerifier blocks the entire load by buffering the content and invokes the ContentSignatureVerifier that holds the actual signature verification logic. Because the key to verify the content signature is provided only via a URL in a header of the content load, the ContentSignatureVerifier has to fetch the certificate chain from the provided URL. This happens when CreateContextWithoutCertChain is invoked by the ContentVerifier. The context used by the ContentSignatureVerifier to verify the signature can't be created before the cert chain is downloaded. Therefore the ContentVerifier only reads data in content OnDataAvailable invocations and calls update on the ContentSignatureVerifier only if the context got created (ContentVerifier::ContextCreated got called). If the content is fully loaded, ContentVerifier::OnStopRequest invokes FinishSignature to verify the signature if the context was created. If this is not the case, the ContextCreated callback does this. Once the signature is verified or any channel (content or cert chain downloading) failed, ContentVerifier invokes mNextListener. If the signature gets successfully verified, mNextListener is provided with the buffered content and the load proceeds as normal. If anything fails, the load is cancelled and a NS_ERROR_INVALID_SIGNATURE is thrown to trigger a fallback in nsDocshell.

> didn't spot this first time:  please use nsDependentCString instead.  this does a copy of the segment 2 times...
An nsDependentCString must be null-terminated, which is not always the case here. I kept nsAutoCString but happy to use something else if there's something that works.

> every idiff description should also say what exactly has changed in that
> structure (best also why)
> ::: dom/security/ContentVerifier.cpp:119
> (Diff revisions 7 - 10)
> > +NS_IMETHODIMP
> > +ContentVerifier::OnStopRequest(nsIRequest* aRequest, nsISupports* aContext,
> > +                               nsresult aStatus)
> > +{
> > +  mRequest = aRequest;
> > +  mContext = aContext;
> 

added a comment.

> ::: netwerk/protocol/http/nsHttpChannel.cpp:1088
> (Diff revisions 7 - 10)
> > +        rv = ProcessContentSignatureHeader(mResponseHead);
> > +        if (NS_FAILED(rv)) {
> > +            LOG(("Content-signature verification failed.\n"));
> > +            return rv;
> > +        }
> > +    }
> 
> I think it's very important to note somewhere that ContentVerifier expects
> OnStartRequest has already been called on the target listener before
> ContentVerifier has being injected.

added a comment.

> ::: security/manager/ssl/ContentSignatureVerifier.cpp:186
> (Diff revisions 7 - 10)
> >                            CertPolicyId::anyPolicy,
> >                            nullptr/*stapledOCSPResponse*/);
> >    if (result != Success) {
> > -    // the chain is bad
> > +    // if there was a library error, return an appropriate error
> > +    if (IsFatalError(result)) {
> > +      return NS_ERROR_FAILURE;
> 
> nit: isn't there some kind of mapping function between Result and nsresult?

There's a mapping function between nsresult and SECStatus, but this is a pkix::Result. But since no one checks the return value for anything other than failure, signature failure, or success I don't think we need a more precise error mapping here.

> ::: dom/security/ContentVerifier.cpp:82
> (Diff revision 10)
> > -      rv = NS_ERROR_INVALID_SIGNATURE;
> > -    }
> >      CSV_LOG(("failed to verify content\n"));
> > -    mNextListener->OnStartRequest(aRequest, aContext);
> > +    mNextListener->OnStopRequest(mRequest, mContext, NS_ERROR_INVALID_SIGNATURE);
> > -    mNextListener->OnStopRequest(aRequest, aContext, rv);
> >      return NS_ERROR_INVALID_SIGNATURE;
> 
> where goes this result?  should the method rather be void?

right, void makes more sense here.

> ::: dom/security/ContentVerifier.cpp:163
> (Diff revision 10)
> >  {
> > -  nsNSSShutDownPreventionLock locker;
> > -  if (isAlreadyShutDown()) {
> > -    return NS_ERROR_INVALID_SIGNATURE;
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  if (!successful) {
> > +    // In this case something went wrong with the cert. Let's stop this load.
> > +    CSV_LOG(("failed to get a valid cert chain\n"));
> > +    mRequest->Cancel(NS_ERROR_INVALID_SIGNATURE);
> 
> what if mRequest is still null (you didn't get OnStartRequest)?
> 
> I though you kept the content request channel from the very start?  your
> code is very confusing in this and I sense problems here.

It should never be the case that mRequest is null because it's set in OnStartRequest and we never invoke the ContentSignatureVerifier before OnStartRequest gets called (and thus we can't land in here). But I added an assert for that case.

Comment 51

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/10-11/

Comment 52

[Honza Bambas (:mayhemer)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #50)
> > didn't spot this first time:  please use nsDependentCString instead.  this does a copy of the segment 2 times...
> An nsDependentCString must be null-terminated, which is not always the case
> here. I kept nsAutoCString but happy to use something else if there's
> something that works.

There is nsDependentCSubstring (-> nsCStringContainer -> nsACString).  It is capable of keeping just a subpart non-null-terminated string.

> There's a mapping function between nsresult and SECStatus, but this is a
> pkix::Result. But since no one checks the return value for anything other
> than failure, signature failure, or success I don't think we need a more
> precise error mapping here.

That's a question.  When this fails from some pkix/security error, user will not have anything good to report to us.

> It should never be the case that mRequest is null because it's set in
> OnStartRequest and we never invoke the ContentSignatureVerifier before
> OnStartRequest gets called (and thus we can't land in here). But I added an
> assert for that case.

Hmm.. I checked the code using your test and you never get call to ContentVerifier::OnStartRequest, obviously.  nsHttpChannel->mListener->OnStartRequest is called before you even add ContentVerifier to the listener chain.  This is a pretty odd bit, but I understand that there is no way around.  We must add ContentVerifier where we do it.

Anyway, please pass the request and context through the ContentVerifier::Init method instead.

Make ContentVerifier::OnStartRequest do just MOZ_CRASH(some good text).

Comment 53

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review60974

one more round needed.

dom/security/test/contentverifier/browser_verify_content_about_newtab.js times out for me (debug build, fast machine)

::: dom/security/ContentVerifier.cpp:71
(Diff revisions 10 - 11)
>  ContentVerifier::FinishSignature()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  nsCOMPtr<nsIStreamListener> nextListener;
> +  nextListener.swap(mNextListener);
> +  mNextListener = nullptr;

no need to do mNextListener = nullptr;

::: dom/security/ContentVerifier.cpp:101
(Diff revisions 10 - 11)
>      if (NS_FAILED(rv)) {
>        break;
>      }
>      // let the next listener know that there is data in oInStr
> -    rv = mNextListener->OnDataAvailable(mRequest, mContext, oInStr, 0,
> -                                        mContent[i].Length());
> +    rv = nextListener->OnDataAvailable(mContentRequest, mContentContext, oInStr,
> +                                       0, mContent[i].Length());

didn't see this before: the '0' you are passing is wrong.  it has to be sum of all previous counts, see nsIStreamListener.idl.

::: dom/security/ContentVerifier.cpp:117
(Diff revisions 10 - 11)
>  ContentVerifier::OnStartRequest(nsIRequest* aRequest, nsISupports* aContext)
>  {
> -  mRequest = aRequest;
> -  mContext = aContext;
> +  // Keep references to the request and context. We need them in FinishSignature
> +  // and the ContextCreated callback.
> +  mContentRequest = aRequest;
> +  mContentContext = aContext;

please don't forget to remove this (see comment 52 in bugzilla)

::: dom/security/ContentVerifier.cpp:131
(Diff revisions 10 - 11)
> +  // Keep references to the request and context. We need them in FinishSignature
> +  // and the ContextCreated callback.
> +  mContentRequest = aRequest;
> +  mContentContext = aContext;
> +
> +  // If we don't have a next listener, return, there's nothing to do here.

please explain when it can happen, why it is legal to don't have it and to just return.

::: dom/security/ContentVerifier.cpp:140
(Diff revisions 10 - 11)
> +
>    if (NS_FAILED(aStatus)) {
>      CSV_LOG(("Stream failed\n"));
> -    return mNextListener->OnStopRequest(aRequest, aContext, aStatus);
> +    nsCOMPtr<nsIStreamListener> nextListener;
> +    nextListener.swap(mNextListener);
> +    mNextListener = nullptr;

no need to for null assignment

::: dom/security/ContentVerifier.cpp:150
(Diff revisions 10 - 11)
>    if (mContextCreated) {
> -    return FinishSignature();
> +    FinishSignature();
> +    return aStatus;
>    }
>  
>    mContentRead = true;

nit: should this be set before FinishSignature call?

::: dom/security/ContentVerifier.cpp:184
(Diff revisions 10 - 11)
>    if (!successful) {
> +    // Get local reference to mNextListener and null it to ensure that we don't
> +    // call it twice.
> +    nsCOMPtr<nsIStreamListener> nextListener;
> +    nextListener.swap(mNextListener);
> +    mNextListener = nullptr;

no need..

and, any non-null checks here?  I believe you could get here after mNextListener has already been dropped as well.

::: dom/security/ContentVerifier.cpp:192
(Diff revisions 10 - 11)
> +    MOZ_ASSERT(mContentRequest);
> +
>      // In this case something went wrong with the cert. Let's stop this load.
>      CSV_LOG(("failed to get a valid cert chain\n"));
> -    mRequest->Cancel(NS_ERROR_INVALID_SIGNATURE);
> -    mNextListener->OnStopRequest(mRequest, mContext, NS_ERROR_INVALID_SIGNATURE);
> +    if (mContentRequest && nextListener) {
> +      mContentRequest->Cancel(NS_ERROR_INVALID_SIGNATURE);

maybe drop mContextRequest+Context here?

::: dom/security/ContentVerifier.cpp:199
(Diff revisions 10 - 11)
> +                                         NS_ERROR_INVALID_SIGNATURE);
> +    }
> +
> +    // We should never get here!
> +    MOZ_ASSERT_UNREACHABLE(
> +      "ContentVerifier was used without getting OnStartRequest!");

probably not a valid assertion?

::: netwerk/protocol/http/nsHttpChannel.cpp:1082
(Diff revisions 10 - 11)
>      // requested and available.
>      // If requested (mLoadInfo->GetVerifySignedContent), but not present, or
>      // present but not valid, fail this channel and return
>      // NS_ERROR_INVALID_SIGNATURE to indicate a signature error and trigger a
>      // fallback load in nsDocShell.
> +    // Note that OnStartRequest has already been called at this point.

...been called _on the target stream listener_ at...

::: security/manager/ssl/ContentSignatureVerifier.h:71
(Diff revisions 10 - 11)
>    nsresult ParseContentSignatureHeader(const nsACString& aContentSignatureHeader);
>  
>    // verifier context for incremental verifications
>    mozilla::UniqueVFYContext mCx;
>    bool mInitialised;
> -  bool mDownloadedCertChain;
> +  // Indicates whether we hold a cert chain to verify the signature or not.

and how is this flag changing?  where is it set?  what effect does it have?

::: dom/security/ContentVerifier.cpp:57
(Diff revision 11)
>                    const char* aRawSegment, uint32_t aToOffset, uint32_t aCount,
>                    uint32_t* outWrittenCount)
>  {
>    FallibleTArray<nsCString>* decodedData =
>      static_cast<FallibleTArray<nsCString>*>(aClosure);
>    nsAutoCString segment(aRawSegment, aCount);

s/nsAutoCString/nsDependentCSubstring/

::: dom/security/nsCSPUtils.cpp:8
(Diff revision 11)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsAttrValue.h"
> +#include "nsCharSeparatedTokenizer.h"

is this your change?  if so, why is it here?

::: netwerk/protocol/http/nsHttpChannel.cpp:1082
(Diff revision 11)
> +    // requested and available.
> +    // If requested (mLoadInfo->GetVerifySignedContent), but not present, or
> +    // present but not valid, fail this channel and return
> +    // NS_ERROR_INVALID_SIGNATURE to indicate a signature error and trigger a
> +    // fallback load in nsDocShell.
> +    // Note that OnStartRequest has already been called at this point.

Please add a good comment why we add the ContentVerifier to the listener chain that late (here).

Comment 54

[Franziskus Kiefer [:franziskus]]

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

Comment 55

[Franziskus Kiefer [:franziskus]]

(In reply to Honza Bambas (:mayhemer) from comment #53)
> Comment on attachment 8754401 [details]
> dom/security/test/contentverifier/browser_verify_content_about_newtab.js
> times out for me (debug build, fast machine)

hm, they work for me on Linux and Mac. I put up a try run (comment 54) to see if it works there. If not, I'll split the test.

> didn't see this before: the '0' you are passing is wrong.  it has to be sum
> of all previous counts, see nsIStreamListener.idl.

done

> ::: dom/security/ContentVerifier.cpp:184
> (Diff revisions 10 - 11)
> >    if (!successful) {
> > +    // Get local reference to mNextListener and null it to ensure that we don't
> > +    // call it twice.
> > +    nsCOMPtr<nsIStreamListener> nextListener;
> > +    nextListener.swap(mNextListener);
> > +    mNextListener = nullptr;
> 
> no need..
> 
> and, any non-null checks here?  I believe you could get here after
> mNextListener has already been dropped as well.

added a null check.

> ::: dom/security/ContentVerifier.cpp:199
> (Diff revisions 10 - 11)
> > +                                         NS_ERROR_INVALID_SIGNATURE);
> > +    }
> > +
> > +    // We should never get here!
> > +    MOZ_ASSERT_UNREACHABLE(
> > +      "ContentVerifier was used without getting OnStartRequest!");
> 
> probably not a valid assertion?

There's no way we can get here unless something is really messed up. We return before the assertion or have another assertion if we don't have mNextListerner, if we don't have mContentRequest, and if we have both. So no way to get here under normal circumstances.

> ::: dom/security/ContentVerifier.cpp:57
> (Diff revision 11)
> >                    const char* aRawSegment, uint32_t aToOffset, uint32_t aCount,
> >                    uint32_t* outWrittenCount)
> >  {
> >    FallibleTArray<nsCString>* decodedData =
> >      static_cast<FallibleTArray<nsCString>*>(aClosure);
> >    nsAutoCString segment(aRawSegment, aCount);
> 
> s/nsAutoCString/nsDependentCSubstring/

done 

> ::: dom/security/nsCSPUtils.cpp:8
> (Diff revision 11)
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include "nsAttrValue.h"
> > +#include "nsCharSeparatedTokenizer.h"
> 
> is this your change?  if so, why is it here?

Yep that was me. It's necessary because the unified build moved some headers around such that nsCSPUtils doesn't get nsCharSeparatedTokenizer included from somewhere else anymore.


(In reply to Honza Bambas (:mayhemer) from comment #52)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #50)
> > There's a mapping function between nsresult and SECStatus, but this is a
> > pkix::Result. But since no one checks the return value for anything other
> > than failure, signature failure, or success I don't think we need a more
> > precise error mapping here.
> 
> That's a question.  When this fails from some pkix/security error, user will
> not have anything good to report to us.

If this fails, i.e. CreateContext fails, the caller has to assume the signature is not valid (and take appropriate steps). The actual reason for the error is not relevant and will never be shown to the user.

> Hmm.. I checked the code using your test and you never get call to
> ContentVerifier::OnStartRequest, obviously. 
> nsHttpChannel->mListener->OnStartRequest is called before you even add
> ContentVerifier to the listener chain.  This is a pretty odd bit, but I
> understand that there is no way around.  We must add ContentVerifier where
> we do it.
> 
> Anyway, please pass the request and context through the
> ContentVerifier::Init method instead.
> 
> Make ContentVerifier::OnStartRequest do just MOZ_CRASH(some good text).

done

Comment 56

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53954/diff/11-12/

Comment 57

[Honza Bambas (:mayhemer)]

Comment on attachment 8754401 [details]
Bug 1263793 - Using content signature verifier for verifying remote newtab,

https://reviewboard.mozilla.org/r/53954/#review61628

I'm missing tests for this, or are there any existing?  They could be added in a separate patch/bug tho.  Still, this is rather visible and complicated feature, a test should be there, landed in the same release as this patch.

::: dom/security/ContentVerifier.cpp:107
(Diff revisions 11 - 12)
>      if (NS_FAILED(rv)) {
>        break;
>      }
>      // let the next listener know that there is data in oInStr
>      rv = nextListener->OnDataAvailable(mContentRequest, mContentContext, oInStr,
> -                                       0, mContent[i].Length());
> +                                       mOffset, mContent[i].Length());

mOffset has to be increased _here_ not in ContentVerifier::OnDataAvailable...

and you then don't need it to be a member..

::: dom/security/ContentVerifier.cpp:131
(Diff revisions 11 - 12)
>  {
> -
>    // Keep references to the request and context. We need them in FinishSignature
>    // and the ContextCreated callback.
>    mContentRequest = aRequest;
>    mContentContext = aContext;

this has to be removed as well.  you already have the reference from Init()

::: dom/security/ContentVerifier.cpp:176
(Diff revisions 11 - 12)
>    if (mContextCreated) {
>      return mVerifier->Update(mContent.LastElement());
>    }
>  
> +  // update offset
> +  mOffset += aCount;

see above comment...

::: dom/security/ContentVerifier.cpp:201
(Diff revisions 11 - 12)
>      MOZ_ASSERT(mContentRequest);
>  
>      // In this case something went wrong with the cert. Let's stop this load.
>      CSV_LOG(("failed to get a valid cert chain\n"));
>      if (mContentRequest && nextListener) {
>        mContentRequest->Cancel(NS_ERROR_INVALID_SIGNATURE);

I still think you should drop mContent* members when you don't need them.  it's just a precaution from potential leaks.

Comment 58

[Franziskus Kiefer [:franziskus]]

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

Comment 59

[Franziskus Kiefer [:franziskus]]

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

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d8bb5af7b4
Using content signature verifier for verifying remote newtab, r=keeler,mayhemer

Comment 61

[Franziskus Kiefer [:franziskus]]

> I'm missing tests for this, or are there any existing?  They could be added in a separate patch/bug tho.  Still, this is rather visible and complicated feature, a test should be there, landed in the same release as this patch.

There are tests (dom/security/test/contentverifier/ and security/manager/ssl/tests/unit/test_content_signing.js). But this will also need some manual testing. We'll get onto this tomorrow.

Comment 62

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

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/62a913298af6 for leaks in various tests.

One example: https://treeherder.mozilla.org/logviewer.html#?job_id=32242178&repo=mozilla-inbound

Comment 63

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

(In reply to Wes Kocher (:KWierso) from comment #62)
> I had to back this out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/62a913298af6 for
> leaks in various tests.
> 
> One example:
> https://treeherder.mozilla.org/logviewer.html#?job_id=32242178&repo=mozilla-
> inbound

Er, that's the wrong backout commit. Correct one: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f816e3b56be

Comment 64

[Franziskus Kiefer [:franziskus]]

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

Comment 65

[Franziskus Kiefer [:franziskus]]

oosp, that shouldn't have happened, sorry! This try run looks clean.

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38d540b40e58
Using content signature verifier for verifying remote newtab, r=keeler,mayhemer

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38d540b40e58

Comment 68

[Franziskus Kiefer [:franziskus]]

Attachment: update-san.patch

Comment 69

[Julien Vehent]

Comment on attachment 8773753 [details] [diff] [review]
update-san.patch

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

Verified to match against issued certificates.

Comment 70

[Franziskus Kiefer [:franziskus]]

Attachment: update-san.patch

updated tests to new SAN

Comment 71

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8774235 [details] [diff] [review]
update-san.patch

Looks good to me.

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe979de162e
update SAN, r=mgoodwin,ulfr

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cfe979de162e