1325309 - TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a844bb9580da7b60946d7592d772c7ef5b79da71&selectedJob=55358

TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js | xpcshell return code: 1
TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js | test_something - [test_something : 111] false == true
PROCESS | 10248 | FATAL ERROR: AsyncShutdown timeout in profile-before-change Conditions: [{"name":"Sqlite.jsm shutdown blocker","state":{"description":"Waiting for connections to close","state":[{"name":"kinto.sqlite#1: waiting for shutdown","state":{"identifier":"kinto.sqlite#1","isCloseRequested":false,"hasDbConn":true,"hasInProgressTransaction":false,"pendingStatements":0,"statementCounter":4},"filename":"resource://gre/modules/Sqlite.jsm","lineNumber":259,"stack":["reso 

Caused by new test created in bug 1306470:
https://hg.mozilla.org/mozilla-central/rev/3e31e8cddf83

Mark, can you please advise us on what we need to do to get this test working for the Thunderbird Xpcshell test suite.

Comment 1

[Jorg K (CEST = GMT+2)]

Looking at https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1482354112/comm-central_win7_ix_test-xpcshell-bm112-tests1-windows-build0.txt.gz:

WARNING -  TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js | test_something - [test_something : 111] false == 

So it fails on line 111 of the test which is ...

===
  // ensure our pins are all missing before we start
  ok(!sss.isSecureHost(sss.HEADER_HPKP, "one.example.com", 0));
  ok(!sss.isSecureHost(sss.HEADER_HPKP, "two.example.com", 0));
  ok(!sss.isSecureHost(sss.HEADER_HPKP, "three.example.com", 0));

  // Test an empty db populates
  let result = yield PinningPreloadClient.maybeSync(2000, Date.now());

  let connection = yield FirefoxAdapter.openConnection({path: KINTO_STORAGE_PATH});

  // Open the collection, verify it's been populated:
  // Our test data has a single record; it should be in the local collection
  let collection = do_get_kinto_collection(connection, COLLECTION_NAME);
  let list = yield collection.list();
  do_check_eq(list.data.length, 1);

  // check that a pin exists for one.example.com
  ok(sss.isSecureHost(sss.HEADER_HPKP, "one.example.com", 0));  <== line 111
===

Comment 2

[Mark Goodwin [:mgoodwin]]

From a bit of poking around, I think the issue here is that this is a test for a key pinning feature - which (suprisingly, perhaps?) seems to be disabled on Thunderbird.

security.cert_pinning.enforcement_level has a default value of 0 - which is pinningDisabled

So possible some possible fixes are (in no particular order):
1) Enable pinning for Thunderbird
2) Disable the test for Thunderbird
3) Modify the test to enable pinning for the duration of the test

I don't know which of the above is the most appropriate for your needs but please let me know if I can help at all with any of these.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1325309-cert_pinning.patch

Mark, thanks for your comment.

With this patch, setting the preference, the test passes:
mozilla/mach xpcshell-test services/common/tests/unit/test_blocklist_pinning.js

Setting the preference can be found in IM
https://dxr.mozilla.org/comm-central/rev/31a0301ffb6abc35496db987a1b0b0a66a28e805/im/app/profile/all-instantbird.js#223
and was most probably copied form here:
https://dxr.mozilla.org/comm-central/rev/c36fbe84042debef0a5d58b7fc88185b401762ce/mozilla/browser/app/profile/firefox.js#1227

Any chance to improve the comment?
1 = allow MITM for certificate pinning checks.

MITM is "man in the middle", right? - so this reads:
allow "man in the middle" for certificate pinning checks.

Really, we want to allow a man in the middle?? I thought the idea is to make the man in the middle's life as hard as possible.

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to Jorg K (GMT+1) from comment #3)
> Created attachment 8821160 [details] [diff] [review]
> 1325309-cert_pinning.patch
> 
> Mark, thanks for your comment.
> 
> With this patch, setting the preference, the test passes:
> mozilla/mach xpcshell-test
> services/common/tests/unit/test_blocklist_pinning.js
> 
> Setting the preference can be found in IM
> https://dxr.mozilla.org/comm-central/rev/
> 31a0301ffb6abc35496db987a1b0b0a66a28e805/im/app/profile/all-instantbird.
> js#223
> and was most probably copied form here:
> https://dxr.mozilla.org/comm-central/rev/
> c36fbe84042debef0a5d58b7fc88185b401762ce/mozilla/browser/app/profile/firefox.
> js#1227
> 
> Any chance to improve the comment?
> 1 = allow MITM for certificate pinning checks.
> 
> MITM is "man in the middle", right? - so this reads:
> allow "man in the middle" for certificate pinning checks.
> 
> Really, we want to allow a man in the middle?? I thought the idea is to make
> the man in the middle's life as hard as possible.

Yes, MITM is "man in the middle". We could improve the comment, I guess.

To explain, this pref corresponds to the values of the PinningMode enum (see https://dxr.mozilla.org/mozilla-central/source/security/certverifier/CertVerifier.h#151). The intent of the pinningAllowUserCAMITM default is to allow users with local roots to have all certs issued by that local root accepted by the browser. Common use-cases for this include:
1) Antivirus software (it's common for A/V software to run a local MITM proxy which dynamically creates certificates for the sites a user is visiting)
2) Web filters. It's common for company networks to have a device which MITMs users' TLS traffic.
3) Security tools. Web sec. testing tools (like OWASP ZAP) will terminate TLS to allow you to interactively inspect and modify traffic.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Mark Goodwin [:mgoodwin] from comment #4)
> Yes, MITM is "man in the middle". We could improve the comment, I guess.
Care to make a suggestion? How about:
1 = allow local proxy (man in the middle) for certificate pinning checks.

Comment 6

[aleth [:aleth]]

Comment on attachment 8821160 [details] [diff] [review]
1325309-cert_pinning.patch

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

Thanks. If you do improve the comment, please do it in all places!

Comment 7

[Jorg K (CEST = GMT+2)]

OK, let's go with:

1 = allow "Man In The Middle" (local proxy, web filter, etc.) for certificate pinning checks.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1325309-cert_pinning.patch with improved comment.

I'll push this in a little while when the current push turns green.

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/cfe5bca5b5255cdddf4e474a8fa8bc5d94af1e70