928536 - Verify signatures on windows binaries

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

We need to implement something like https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/safe_browsing/signature_util_win.cc&sq=package:chromium&type=cs&q=WTHelperProvDataFromStateData

which uses a couple of windows native verification checks:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa388208%28v=vs.85%29.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/aa388451%28v=vs.85%29.aspx

Otherwise, Google has said that we can't send queries on signed binaries, as sending queries with incorrect signature information affects reputation.

Comment 1

[:Paolo Amadini]

I have the feeling that these synchronous Windows APIs may take a very long time (for example, if they determine that the system-level certificate revocation list should be updated) meaning that we should provide specific user interface feedback in that phase. Being synchronous, they should also be executed on a background thread, with a level of complexity similar to the IAttachmentExecute calls.

Is this a blocker for enabling application reputation checks?

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #0)
> Otherwise, Google has said that we can't send queries on signed binaries, as
> sending queries with incorrect signature information affects reputation.

I don't quite understand what this means exactly. We don't send any signature information to the service, just the hash of the downloaded file.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to :Paolo Amadini from comment #1)
> I have the feeling that these synchronous Windows APIs may take a very long
> time (for example, if they determine that the system-level certificate
> revocation list should be updated) meaning that we should provide specific
> user interface feedback in that phase. Being synchronous, they should also
> be executed on a background thread, with a level of complexity similar to
> the IAttachmentExecute calls.
> 
> Is this a blocker for enabling application reputation checks?

It is a blocker for enabling it on Windows.

> (In reply to Monica Chew [:mmc] (please use needinfo) from comment #0)
> > Otherwise, Google has said that we can't send queries on signed binaries, as
> > sending queries with incorrect signature information affects reputation.
> 
> I don't quite understand what this means exactly. We don't send any
> signature information to the service, just the hash of the downloaded file.

The ClientDownloadRequest has fields for a signature, which are not filled in at the moment:

https://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/csd.pb.h#1079

Google has said that omitting this signature (if it is available) will adversely affect reputation computation on the server side.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Needs to be split, not ready for review

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use WinVerifyTrust to get certificate information on downloaded binaries (

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use WinVerifyTrust to get certificate information on downloaded binaries (

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Send signature information in remote lookups (

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate signature verification info into application reputation call in download manager (

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8366185 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

Hi Paolo,

I thought about spinning up another thread to do the checks on -- but since ultimately the application reputation verdict needs to be received before state can reach DOWNLOAD_FINISHED, I didn't think that would save any time than just using an existing background thread.

Hi Brian,

If you have time for feedback on this patch, that would be great. The background behind this patch is that in order to use Google's safebrowsing application reputation check for malicious binaries, we need to extract MS authenticode signatures whenever possible.

I notice that you are the author of the WinVerifyTrust code in http://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/maintenanceservice/certificatecheck.cpp. Any advice you have on testing would be most appreciated. I could make a signed binary, but I think the signature would expire after some time so it might be brittle.

The equivalent call in Chrome for the safebrowsing checks is here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/safe_browsing/signature_util_win.cc&q=WinVerifyTrust&sq=package:chromium&type=cs&l=54

Thanks,
Monica

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8366185 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

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

::: netwerk/test/unit/test_backgroundfilesaver.js
@@ +687,5 @@
> +  saver.finish(Cr.NS_OK);
> +  yield completionPromise;
> +  yield promiseVerifyContents(destFile, TEST_DATA_SHORT);
> +
> +  try {

oops, this try-catch should've been deleted

Comment 10

[:Paolo Amadini]

Comment on attachment 8366185 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

I took a quick look at the patch and this actually looks like the simplest way to handle the check.

This creates a dependency from code located in "netwerk" to code in "toolkit/components/downloads". I'm not sure whether a dependency in this direction is possible, requesting feedback from Patrick on this.

In fact, sigantureInfo is a serialized ClientDownloadRequest_SignatureInfo that can only ever be useful for the reputation check. Unfortunately, I don't see any other data structures in the process that can be easily serialized.

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #8)
> I thought about spinning up another thread to do the checks on -- but since
> ultimately the application reputation verdict needs to be received before
> state can reach DOWNLOAD_FINISHED, I didn't think that would save any time
> than just using an existing background thread.

While I don't think that getting signature info is an operation that belongs to BackgroundFileSaver, the current solution is probably the simplest for various reasons:
- As you pointed out, we don't need to write more boilerplate to work on a background thread.
- The intermediate data structures are difficult to serialize.
- Access to ClientDownloadRequest_SignatureInfo needs to be done in C++ anyways.

If we had an easy way to serialize trust information into an independent data structure, then a separate OS.File.winVerifyTrust method would have worked better, and allowed us to treat the (possibly long) pause this would create at the end of a download as a separate phase in the future.

Comment 11

[:Paolo Amadini]

Hm, thinking more about this, we could remove the "netwerk" > "downloads" dependency by defining signatureInfo as an opaque nsISupports, conveniently populated with the list of certificate chains, as an nsIArray of nsIArray of C strings. This would be nullptr if the file is not trusted.

This is what nsIApplicationReputationQuery would accept as its input. This is loosely typed, but not more loosely typed than a generic ACString as suggested in the current proposal, and doesn't depend on ClientDownloadRequest_SignatureInfo that is an implementation detail of the remote service we're currently using. (If there are better types to represent certificate chains in our code, we could use them. I don't know our XPCOM security code enough.)

This would allow us to later separate the operation into an OS.File or other worker call at the downloads layer, thus providing relevant status feedback in the UI during the check. (I think we don't need to do this now for the simplicity reasons above, but we can file a bug about that.)

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8366192 [details] [diff] [review]
Send signature information in remote lookups (

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +164,5 @@
>      return OnComplete(true, NS_OK);
>    }
>  
>    Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, NO_LIST);
> +#if 0 and defined(XP_WIN)

0 and xxx ? This makes no sense.

Comment 13

[Brian R. Bondy [:bbondy]]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #8)
> Comment on attachment 8366185 [details] [diff] [review]
> Use WinVerifyTrust to get certificate information on downloaded binaries (
> 
> Hi Brian,
> 
> If you have time for feedback on this patch, that would be great. The
> background behind this patch is that in order to use Google's safebrowsing
> application reputation check for malicious binaries, we need to extract MS
> authenticode signatures whenever possible.

Extract but not remove right? Do what with it?

> 
> I notice that you are the author of the WinVerifyTrust code in
> http://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/
> maintenanceservice/certificatecheck.cpp. Any advice you have on testing
> would be most appreciated. 

Did you want to test updates in some way? Or just test that the signature is valid for arbitrary downloaded binaries?

> I could make a signed binary, but I think the
> signature would expire after some time so it might be brittle.

It shouldn't expire as I understand it. The binary would be signed before the date of the expiration on the cert. So only new things signed after that would fail.

> 
> The equivalent call in Chrome for the safebrowsing checks is here:
> 
> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/
> safe_browsing/signature_util_win.cc&q=WinVerifyTrust&sq=package:
> chromium&type=cs&l=54
> 
> Thanks,
> Monica

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to :Paolo Amadini from comment #11)
> Hm, thinking more about this, we could remove the "netwerk" > "downloads"
> dependency by defining signatureInfo as an opaque nsISupports

Hi Paolo,

After investigating this, if you want to go this route the best thing would be to return an nsIArray of nsIX509CertLists of nsIX509Cert, created with nsIX509CertDB.constructX509FromBase64 after encoding the raw cert bytes to base64. Unfortunately it looks like that code was not designed to be called from outside of security/ssl so it would require some cooperation from those module owners as well, and would introduce dependencies from netwerk > security/ssl. I'm not sure if that's better or worse than the current dependency (which is really on the protobuf library -- we could move csd.pb.h to netwerk).

Ultimately this security-related code should not pollute downloads, and should live probably with other safebrowsing code.

(In reply to Gian-Carlo Pascutto (:gcp) from comment #12)
> Comment on attachment 8366192 [details] [diff] [review]
> Send signature information in remote lookups (
> 
> Review of attachment 8366192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/downloads/ApplicationReputation.cpp
> @@ +164,5 @@
> >      return OnComplete(true, NS_OK);
> >    }
> >  
> >    Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, NO_LIST);
> > +#if 0 and defined(XP_WIN)
> 
> 0 and xxx ? This makes no sense.

Yeah, I will add a comment pointing to bug 964465, which we should do before turning on the remote checks.

(In reply to Brian R. Bondy [:bbondy] from comment #13)

> > application reputation check for malicious binaries, we need to extract MS
> > authenticode signatures whenever possible.
> 
> Extract but not remove right? Do what with it?

Yes, extract but not remove. There are 2 things:
1) Lookup the certificate signers in the local safebrowsing whitelist to know if the file is not malicious
2) If we don't get any hits, pass on certificate information to the safebrowsing lookup API

> Did you want to test updates in some way? Or just test that the signature is
> valid for arbitrary downloaded binaries?

It would be nice to automatically test that the WinVerifyTrust function is working as expected when it gets a known signed binary. I didn't know if you had any tricks from testing the update installer stuff :)

Thanks,
Monica

Comment 15

[Brian R. Bondy [:bbondy]]

> It would be nice to automatically test that the WinVerifyTrust function is working as expected 
> when it gets a known signed binary. I didn't know if you had any tricks from testing 
> the update installer stuff :)

Yep so you should be able to sign an old binary and verify it. Even in the future when the cert expires it should be valid since the binary was signed before it expired.

Comment 16

[:Paolo Amadini]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #14)
> After investigating this, if you want to go this route the best thing would
> be to return an nsIArray of nsIX509CertLists of nsIX509Cert

Looks like this could definitely provide better typing and even meaningful signature inspection. Are the certificates returned by the Windows API always valid X509 certificates?

> created with
> nsIX509CertDB.constructX509FromBase64 after encoding the raw cert bytes to
> base64.

Since we already use NSS in BackgroundFileSaver, I guess we can easily implement there only the portion after base64 decoding, with no need for a round-trip:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#1425

> Unfortunately it looks like that code was not designed to be called
> from outside of security/ssl so it would require some cooperation from those
> module owners as well

If the above works, we probably don't need to touch the "security" folder.

> and would introduce dependencies from netwerk > security/ssl.

We already have that dependency, and also use the X509 type there:
http://mxr.mozilla.org/mozilla-central/search?string=nsIX509&find=netwerk
 
> Ultimately this security-related code should not pollute downloads, and
> should live probably with other safebrowsing code.

Encapsulating everything into nsIApplicationReputation is another option, but probably more work, for the need to spawn a separate thread.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8366185 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

(In reply to :Paolo Amadini from comment #16)
> Looks like this could definitely provide better typing and even meaningful
> signature inspection. Are the certificates returned by the Windows API
> always valid X509 certificates?

No, they may be PKCS7 or X509 blobs: http://msdn.microsoft.com/en-us/library/windows/desktop/aa377189%28v=vs.85%29.aspx

However, despite implementing nsIX509, the nsNSSCertificate constructor eventually calls into http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pkcs7/certread.c#258 which does handle PKCS7. I need to double-check that I'm reading that right.

I'll give it another try with the nss stuff.

Thanks,
Monica

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #17)
> Comment on attachment 8366185 [details] [diff] [review]
> Use WinVerifyTrust to get certificate information on downloaded binaries (
> 
> (In reply to :Paolo Amadini from comment #16)
> > Looks like this could definitely provide better typing and even meaningful
> > signature inspection. Are the certificates returned by the Windows API
> > always valid X509 certificates?
> 
> No, they may be PKCS7 or X509 blobs:
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa377189%28v=vs.
> 85%29.aspx

Chrome says it only cares about X509 in the documentation. We could just ignore any PKCS7 blobs.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe_browsing/csd.proto&l=180

Comment 19

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate signature verification info into application reputation call in download manager (

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Send signature information in remote lookups (

Comment 21

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use WinVerifyTrust to get certificate information on downloaded binaries (

Comment 22

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368103 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

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

Hi Paolo,

I re-wrote it to use an nsIArray of nsIX509CertLists. The combination of extra NSS code and XPCOM code makes it less readable in my opinion, but it's up to you. Adding David for the NSS stuff.

Thanks,
Monica

Comment 23

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368102 [details] [diff] [review]
Send signature information in remote lookups (

+David for the NSS stuff.

Comment 24

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368102 [details] [diff] [review]
Send signature information in remote lookups (

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +185,5 @@
>  
>    Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, NO_LIST);
> +  // Revert to just ifdef XP_WIN when remote lookups are enabled (bug 933432)
> +//#if 0 and defined(XP_WIN)
> +#if defined(XP_WIN)

Will revert to if 0 before checkin.

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368102 [details] [diff] [review]
Send signature information in remote lookups (

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

I forgot this needs to do the NSS shutdown prevention dance. Update coming.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "ApplicationReputation.h"
>  #include "csd.pb.h"
> +#include "ScopedNSSTypes.h"

I forgot that this needs to do the NSS shutdown prevent

Comment 26

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

Comment on attachment 8368103 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

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

The NSS-related parts look good.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +875,5 @@
> +          break;
> +        nsCOMPtr<nsIX509CertList> chain = new nsNSSCertList(nullptr, nssLock);
> +        for (DWORD k = 0; k < simpleChain->cElement; ++k) {
> +          CERT_CHAIN_ELEMENT* element = simpleChain->rgpElement[k];
> +          nsCOMPtr<nsIX509Cert> pCert = nsNSSCertificate::ConstructFromDER(

This can return null (e.g. decoding failed for some reason), which probably isn't safe to pass to AddCert without checking first.

Comment 27

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

Comment on attachment 8368102 [details] [diff] [review]
Send signature information in remote lookups (

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

The NSS parts look good.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "ApplicationReputation.h"
>  #include "csd.pb.h"
> +#include "ScopedNSSTypes.h"

Actually, as long as a class doesn't have any members that are directly NSS objects, my understanding is that this isn't necessary. However, any call on an object that is an nsNSSShutDownObject may result in NS_ERROR_NOT_AVAILABLE, so just be aware of that.

@@ +250,5 @@
> +  rv = chains->HasMoreElements(&hasMoreChains);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  while (hasMoreChains) {
> +    nsCOMPtr<nsIX509CertList> certList;
> +    rv = chains->GetNext(getter_AddRefs(certList));

NS_ENSURE_SUCCESS(rv, rv);

@@ +259,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Each chain has at least one certificate.
> +    bool hasMoreCerts = false;
> +    rv = chainElt->HasMoreElements(&hasMoreCerts);

NS_ENSURE_SUCCESS(rv, rv);

@@ +271,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      // Add this certificate to the protobuf to send remotely.
> +      certChain->add_element()->set_certificate(data, len);
> +      PORT_Free(data);

Looks like GetRawDER uses nsMemory::Alloc, so this should probably be nsMemory::Free.

@@ +273,5 @@
> +      // Add this certificate to the protobuf to send remotely.
> +      certChain->add_element()->set_certificate(data, len);
> +      PORT_Free(data);
> +
> +      bool hasMoreCerts = false;

Isn't this declaration masking the previous one?

@@ +274,5 @@
> +      certChain->add_element()->set_certificate(data, len);
> +      PORT_Free(data);
> +
> +      bool hasMoreCerts = false;
> +      rv = chainElt->HasMoreElements(&hasMoreCerts);

NS_ENSURE_SUCCESS(rv, rv);

Comment 28

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368102 [details] [diff] [review]
Send signature information in remote lookups (

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +273,5 @@
> +      // Add this certificate to the protobuf to send remotely.
> +      certChain->add_element()->set_certificate(data, len);
> +      PORT_Free(data);
> +
> +      bool hasMoreCerts = false;

Yes it is, thanks. Will fix these before checking in.

Comment 29

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: 8368103: Use WinVerifyTrust to get certificate information on downloaded binaries

Address David's comments.

Comment 30

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Send signature information in remote lookups

Address David's comments.

Comment 31

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate signature verification into application reputation call in download manager

Fix orange mochitest on try due to missing comma in mockTransfer: https://tbpl.mozilla.org/?tree=Try&rev=190b78de7943

Comment 32

[:Paolo Amadini]

Comment on attachment 8368312 [details] [diff] [review]
8368103: Use WinVerifyTrust to get certificate information on downloaded binaries

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

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #22)
> I re-wrote it to use an nsIArray of nsIX509CertLists. The combination of
> extra NSS code and XPCOM code makes it less readable in my opinion

As long as we document the interface appropriately, this provides much more utility and clarity to consumers than what an opaque blob could provide. This also gives us the ability to move the signature reading process to OS.File or another module in the future, which we won't have otherwise.

Tests should now be able to verify the contents of the signature (like the certificate issuers).

By the way, we can go ahead and write a proper test for a valid signature now.

::: netwerk/base/public/nsIBackgroundFileSaver.idl
@@ +52,5 @@
>     */
>    attribute nsIBackgroundFileSaverObserver observer;
>  
>    /**
> +   * An nsIArray of nsIX509CertLists.

Please use the exact type name even in comments, singular in this case, otherwise it seems a different type.

This should also specify what the attribute represents (signatures with X.509 certificates on the saved file). Each entry is a different signer, if I understand correctly, and the nsIX509CertList contains the certificates up to the root one (which is the last one, right?).

nit: empty line after paragraph

@@ +78,5 @@
> +   *
> +   * @remarks This must be set on the main thread before the first call to
> +   *          setTarget.
> +   */
> +  void enableSha256();

We should have an enableSignatureInfo() method as well, since BackgroundFileSaver might be used just to serialize streams by other code that is not download-related.

With this new method and the comment changes above, feel free to ask for super-review on the new interface.

Also, is it correct that we attempt to call WinVerifyTrust on every download regardless of file type? This includes HTML and text files.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +30,5 @@
> +#include <softpub.h>
> +#include <wintrust.h>
> +
> +#pragma comment(lib, "wintrust.lib")
> +#pragma comment(lib, "crypt32.lib")

It doesn't seem we call any function from "crypt32.lib" in this file? Does the code still work if you remove the line?

@@ +44,5 @@
> +#define LOG_ENABLED() PR_LOG_TEST(BackgroundFileSaver::prlog, 4)
> +#else
> +#define LOG(args)
> +#define LOG_ENABLED() (false)
> +#endif

We should include the instance address in each log line, as well as logging the object creation and destruction, similar to HTTP logging for example:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#211

@@ +730,5 @@
>  
> +  // Compute the signature of the binary. For some reason, mActualTarget on
> +  // Linux is null by this point. However, since ExtractSignatureInfo doesn't
> +  // do anything on non-Windows platforms except return an empty protocol
> +  // buffer, it doesn't matter.

I think mActualTarget can be null only if no target file has ever been set. Maybe the tests on Linux are different?

In any case, if this still happens when the target has been set, we should understand the cause because there should be no platform difference here.

nit: leftover reference to "protocol buffer"

@@ +735,5 @@
> +  if (!failed) {
> +    nsString filePath;
> +    if (mActualTarget) {
> +      mActualTarget->GetTarget(filePath);
> +    }

When mActualTarget is null, ExtractSignatureInfo should not be called.

@@ +812,5 @@
> + * the signature data in mSignatureInfo.
> + *
> + * @param filePath The file path to check.
> + * @return NS_OK if unsigned or verification was successful, or the last error
> + * code otherwise.

Specify that we handle only X.509 certificates and why. We also store the outcome in a member variable.

nit: indent like other comments in the file and IDL. @return is not required for nsresult functions if it doesn't have special meaning.

@@ +857,5 @@
> +  // Check if the file is signed by something that is trusted.
> +  LONG ret = WinVerifyTrust(nullptr, &policyGUID, &trustData);
> +  CRYPT_PROVIDER_DATA* prov_data = WTHelperProvDataFromStateData(
> +    trustData.hWVTStateData);
> +  if (ret == ERROR_SUCCESS && prov_data && prov_data->csSigners > 0) {

Shouldn't we check ret before calling WTHelperProvDataFromStateData?

nit: since the return value is LONG, Microsoft documentation recommends checking it against 0 instead of ERROR_SUCCESS.

It's not clear from the documentation if we should still call WTD_STATEACTION_CLOSE if the return value is nonzero.

@@ +867,5 @@
> +    for (DWORD i = 0; i < prov_data->csSigners; ++i) {
> +      const CERT_CHAIN_CONTEXT* certChainContext =
> +        prov_data->pasSigners[i].pChainContext;
> +      if (!certChainContext)
> +        break;

Please brace single-line statements in all cases, per the recent Mozilla style choice.

Also, comments on what it means if fields are null might be helpful.

@@ +872,5 @@
> +      for (DWORD j = 0; j < certChainContext->cChain; ++j) {
> +        const CERT_SIMPLE_CHAIN* simpleChain = certChainContext->rgpChain[j];
> +        if (!simpleChain)
> +          break;
> +        nsCOMPtr<nsIX509CertList> chain = new nsNSSCertList(nullptr, nssLock);

Should null-check "chain".

nit: consistent naming would improve readability, like chain => nssCertList, simpleChain => certSimpleChain, prov_data => cryptProviderData, cert => nssCertificate, element => certChainElement

@@ +882,5 @@
> +          nsCOMPtr<nsIX509Cert> cert = nsNSSCertificate::ConstructFromDER(
> +            reinterpret_cast<char *>(element->pCertContext->pbCertEncoded),
> +            element->pCertContext->cbCertEncoded);
> +          if (!cert) {
> +            break;

I guess it doesn't make sense to return truncated chains if construction fails, i.e. AppendObject should not be called.

We should probably log this error as well.

::: netwerk/base/src/BackgroundFileSaver.h
@@ +237,5 @@
>    ScopedPK11Context mDigestContext;
>  
> +  /**
> +   * Store the signature info in raw bytes representing a serialized
> +   * safe_browsing::ClientDownloadrequest_SignatureInfo protocol buffer.

Update comment (no need to repeat everything from the IDL, though).

::: netwerk/test/unit/test_backgroundfilesaver.js
@@ +300,5 @@
> +  function onTargetChange(aTarget) {
> +    do_print("Target file changed to: " + aTarget.leafName);
> +    currentFile = aTarget;
> +  }
> +

Is this done to avoid a strict warning? In any case, currentFile should be reset to null during each iteration of the loop.

@@ +687,5 @@
> +  saver.finish(Cr.NS_OK);
> +  yield completionPromise;
> +  yield promiseVerifyContents(destFile, TEST_DATA_SHORT);
> +
> +  let sig = saver.signatureInfo;

nit: let signatureInfo (or just inline the check)

Comment 33

[:Paolo Amadini]

Comment on attachment 8368315 [details] [diff] [review]
Integrate signature verification into application reputation call in download manager

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

Looks good, might need to be updated to explicitly enable the signature retrieval.

::: toolkit/components/downloads/nsDownloadManager.h
@@ +429,5 @@
> +  /**
> +   * Stores the certificate chains in an nsIArray of nsIX509CertLists of
> +   * nsIX509Certs, if this binary is signed.
> +   */
> +  nsCOMPtr<nsIArray> mSignatureInfo;

Also here and below, use nsIX509CertList, nsIX509Cert.

nit: empty line before comment

::: uriloader/base/nsITransfer.idl
@@ +73,5 @@
> +     * @param aSignatureInfo The signature as a serialized
> +     *                       ClientDownloadRequest_SignatureInfo of the
> +     *                       downloaded file.
> +     */
> +    void setSignatureInfo(in nsIArray aSignatureInfo);

Update comment.

nit: empty line before comment and before @param

Comment 34

[:Paolo Amadini]

Comment on attachment 8368314 [details] [diff] [review]
Send signature information in remote lookups

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +56,5 @@
> +#define LOG_ENABLED() PR_LOG_TEST(ApplicationReputationService::prlog, 4)
> +#else
> +#define LOG(args)
> +#define LOG_ENABLED() (false)
> +#endif

We'll need instance logging here as well.

@@ +549,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Check local lists to see if the URI has already been whitelisted or
> +  // blacklisted. TODO(mmc): If it is signed, check the whitelists for the
> +  // certificate chain as well.

nit: we generally prefer references to bug number (in the style of the one you put earlier in this file) rather than TODO items.

Comment 35

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use WinVerifyTrust to get certificate information on downloaded binaries (

Comment 36

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Send signature information in remote lookups (

Comment 37

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate signature verification info into application reputation call in download manager (

Comment 38

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368888 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

Address Paolo's comments.

Comment 39

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368890 [details] [diff] [review]
Integrate signature verification info into application reputation call in download manager (

Add enableSignatureInfo where appropriate.

Comment 40

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368889 [details] [diff] [review]
Send signature information in remote lookups (

Address Paolo's comments.

Comment 41

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368888 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

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

This patch contains netwerk/test/unit/data/signed_win.exe (the smallest signed binary I could find) which is showing up here: https://bug928536.bugzilla.mozilla.org/attachment.cgi?id=8368888 but not on the review view. I filed bug 913582 a while back to fix this.

Comment 42

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8368888 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

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

Mossop, please review idl changes. Paolo reviewed in comment 32.

Comment 43

[:Paolo Amadini]

Comment on attachment 8368888 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

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

Looks quite good to me, with some minor tweaks.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +124,3 @@
>  }
>  
>  BackgroundFileSaver::~BackgroundFileSaver()

Please log object creation and destruction like HTTP logging does.

@@ +882,5 @@
> +        bool extractionSuccess = true;
> +        const CERT_SIMPLE_CHAIN* certSimpleChain =
> +          certChainContext->rgpChain[j];
> +        if (!certSimpleChain) {
> +          extractionSuccess = false;

No need to set extractionSuccess to false here and below (the boolean can be declared before the inner loop).

@@ +884,5 @@
> +          certChainContext->rgpChain[j];
> +        if (!certSimpleChain) {
> +          extractionSuccess = false;
> +          break;
> +       }

indentation

@@ +894,5 @@
> +        }
> +        for (DWORD k = 0; k < certSimpleChain->cElement; ++k) {
> +          CERT_CHAIN_ELEMENT* certChainElement = certSimpleChain->rgpElement[k];
> +          if (certChainElement->pCertContext->dwCertEncodingType !=
> +            X509_ASN_ENCODING) {

indent two more spaces

@@ +910,5 @@
> +          nssCertList->AddCert(nssCert);
> +          nsString subjectName;
> +          nssCert->GetSubjectName(subjectName);
> +          LOG(("Adding cert %s [this = %p]",
> +               NS_ConvertUTF16toUTF8(subjectName).get(), this));

Is a null subjectName (in case of failure) handled correctly?

@@ +919,5 @@
> +      }
> +    }
> +    // Free the provider data
> +    trustData.dwStateAction = WTD_STATEACTION_CLOSE;
> +    WinVerifyTrust(nullptr, &policyGUID, &trustData);

I guess we should call this when ret is nonzero even if WTHelperProvDataFromStateData returns null.

::: netwerk/test/unit/test_backgroundfilesaver.js
@@ +322,1 @@
>      // Create the object and register the observers.

Should add "currentFile = null;" before this for test correctness, to ensure we're not using the value from the previous iteration in case we don't receive the notification for any reason.

::: netwerk/test/unit/test_signature_extraction.js
@@ +3,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * This file tests components that implement nsIBackgroundFileSaver.

nit: update description in header.

Comment 44

[:Paolo Amadini]

Comment on attachment 8368889 [details] [diff] [review]
Send signature information in remote lookups (

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +144,4 @@
>  }
>  
> +PendingLookup::~PendingLookup()
> +{

Log creation and destruction.

Comment 45

[:Paolo Amadini]

Comment on attachment 8368890 [details] [diff] [review]
Integrate signature verification info into application reputation call in download manager (

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

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +1465,5 @@
>     */
>    _sha256Hash: null,
>  
>    /**
> +   * Save the signature info as an nsIArray of nsIX509CertLists of nsIX509Certs

nit: plurals (and also in the same comment below)

Comment 46

[Dave Townsend [:mossop]]

Comment on attachment 8368888 [details] [diff] [review]
Use WinVerifyTrust to get certificate information on downloaded binaries (

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

I can't say I'm a fan of the form of this API but I guess it matches what is already there and consistency wins

::: netwerk/base/public/nsIBackgroundFileSaver.idl
@@ -60,2 @@
>     */
> -  void enableSha256();

Any reason you're moving this down? It complicates blame unnecessarily

Comment 47

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Dave Townsend (:Mossop) from comment #46)
> Any reason you're moving this down? It complicates blame unnecessarily

Fortunately I am to blame for the whole thing, anyway. I thought it would be nice to have member variables above member functions instead of intermixing them, but I see now that our style guide doesn't specify that.

> Is a null subjectName (in case of failure) handled correctly?

Yes, it will just be an empty string.

Unfortunately since the time I converted to using all that nss code on Wednesday, the dependencies of nsNSSCertificate.h have changed to include more unexported libraries, and the module owner re-iterated his desire to not call NSS outside of the NSS code base. So I'll have to figure out another way to construct the nsIX509 certs.

Comment 48

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Paolo and Brian,

It turns out there is no constructor for nsIX509CertList outside of nsNSSCertificate.h, which Brian doesn't want to export. There are 4 options:

1) Revert back to the original serialized protobuf solution, and do the certificate testing stuff in ApplicationReputation in bug 964465, which I have to do anyway.
2) Use nsIArray instead of nsIX509CertList.
3) Add a constructor for nsIX509CertList to nsIX509CertDB.
4) Use nsIX509CertDb.getCerts to get the real nsIX509CertList and call deleteCert repeatedly until it's empty.

What do you think?

Thanks,
Monica

Comment 49

[[:mmc] Monica Chew (no longer reading bugmail)]

I started implementing 2) on the theory that Paolo won't want 1) and Brian won't want 3) and no one wants 4).

Comment 50

[:Paolo Amadini]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #49)
> I started implementing 2) on the theory that Paolo won't want 1) and Brian
> won't want 3) and no one wants 4).

That would have been my suggestion as well, on the assumption that the reason Brian doesn't want to use the nsIX509CertList object here is that it may have meaning only in the context of the certificate DB, in other words it is considered to be more than a typed array.

In that case I'd suggest to remove this unreferenced line (though it's Brian's call):
http://mxr.mozilla.org/mozilla-central/search?string=NS_X509CERTLIST_CONTRACTID

Comment 51

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to :Paolo Amadini from comment #50)
> (In reply to Monica Chew [:mmc] (please use needinfo) from comment #49)
> > I started implementing 2) on the theory that Paolo won't want 1) and Brian
> > won't want 3) and no one wants 4).
> 
> That would have been my suggestion as well, on the assumption that the
> reason Brian doesn't want to use the nsIX509CertList object here is that it
> may have meaning only in the context of the certificate DB, in other words
> it is considered to be more than a typed array.

Ugh, so I am running into problems because nsIArray is not thread-safe and I need to access from multiple threads. nsIX509CertList is threadsafe. There is another structure, nsISupportsArray that has both an enum interface and is threadsafe, but its IDL says "Do not use."

I think it will have to be option 3) after all.

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

mmc seems to have resolved this already. Clearing NEEDINFO.

Comment 53

[[:mmc] Monica Chew (no longer reading bugmail)]

Addressed Paolo's comments, added logging where requested, waiting on try.

https://tbpl.mozilla.org/?tree=Try&rev=fb9a2a6cdce1

> +    // Free the provider data
> +    trustData.dwStateAction = WTD_STATEACTION_CLOSE;
> +    WinVerifyTrust(nullptr, &policyGUID, &trustData);


>I guess we should call this when ret is nonzero even if WTHelperProvDataFromStateData returns null.

There is nothing to free in this case, updated comment.

> Also, is it correct that we attempt to call WinVerifyTrust on every download regardless of file type? > This includes HTML and text files.

It is a no-op in this case, so I left it as is and updated the comment.

Comment 54

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use WinVerifyTrust to get certificate information on downloaded binaries (

Comment 55

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Send signature information in remote lookups (

Comment 56

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate signature verification info into application reputation call in download manager (

Comment 57

[[:mmc] Monica Chew (no longer reading bugmail)]

Try looks good, carrying over flags, tree is closed so marking checkin-needed.

https://tbpl.mozilla.org/?tree=Try&rev=fb9a2a6cdce1

Comment 58

[[:mmc] Monica Chew (no longer reading bugmail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff2aed7f76f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/baa309383864
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/750054102e68

Comment 59

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ecf12320de7d for the so-far puzzling build failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=34141187&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=34139730&tree=Mozilla-Inbound

Comment 60

[[:mmc] Monica Chew (no longer reading bugmail)]

This failed for 2 reasons: I didn't check b2g on try, and try included #if defined (XP_WIN) instead of #if 0 and defined(XP_WIN) around the SendRemoteQuery check.

Comment 61

[[:mmc] Monica Chew (no longer reading bugmail)]

I don't understand the b2g failures, they are breaking on nsDownloadManager including nsILocalFileWin.h, which hasn't been touched since 2012.

Comment 62

[[:mmc] Monica Chew (no longer reading bugmail)]

It's because of a missing include for nsIArray in nsDownloadManager, which triggers on debug builds but not opt ones.

Comment 63

[Masatoshi Kimura [:emk]]

Probably unified vs. non-unified builds issue.

Comment 64

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use WinVerifyTrust to get certificate information on downloaded binaries (

Comment 65

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Send signature information in remote lookups (

Comment 66

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate signature verification info into application reputation call in download manager (

Comment 67

[[:mmc] Monica Chew (no longer reading bugmail)]

This time, try with -p all: https://tbpl.mozilla.org/?tree=Try&rev=4308bb5c0343 with the attached patches.

The 2 b2g ICS reds are due to https://hg.mozilla.org/integration/mozilla-inbound/rev/ec696ad6ff10

Comment 68

[[:mmc] Monica Chew (no longer reading bugmail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c388e9bc5b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe77ed33ca3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a35eb8ef75a1

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c388e9bc5b5
https://hg.mozilla.org/mozilla-central/rev/6fe77ed33ca3
https://hg.mozilla.org/mozilla-central/rev/a35eb8ef75a1