Opened 6 months ago

Closed 4 months ago

#25331 closed defect (fixed)

Test from #18912 failing

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201804R
Cc: mcs, brade, arthuredelstein, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

While preparing to uplift our patch for #18912, I rebased it to mozilla-central but found one of the mochitests is failing. I then ran the test in the latest tor-browser.git branch (tor-browser-52.6.0esr-8.0-2) and saw the same test failure:

0 INFO TEST-START | Shutdown 1 INFO Passed: 30 2 INFO Failed: 1 3 INFO Todo: 0 4 INFO Mode: non-e10s 5 INFO SimpleTest FINISHED The following tests failed: 56 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul | Checking update.errorCode == MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE - got 2153390067, expected 2153398272 SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:271:5 checkErrorCode@chrome://mochitests/content/chrome/toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul:61:3 defaultCallback@chrome://mochitests/content/chrome/toolkit/mozapps/update/tests/chrome/utils.js:429:5

Somehow we're not getting the desired error code.

Child Tickets

Attachments (1)

bug25331-patch.txt (1.8 KB) - added by mcs 6 months ago.
patch for test

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 months ago by mcs

Status: newneeds_information

After poking around for a while with a 8.0a1-ish build of Tor Browser, Kathy and I learned that the test is failing due to #18087. When we run the test using the command below it works (i.e., the expected error code is generated):

./mach mochitest --setpref=security.nocertdb=false toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul

Arthur, when you ran the test on mozilla-central was security.nocertdb set to false? I am not sure if the build you are using has the Tor Browser default prefs or not.

comment:2 Changed 6 months ago by arthuredelstein

Thanks for looking into this!

I ran some tests with your patch rebased on top of mozilla-central (https://github.com/arthuredelstein/tor-browser/1436226+2). First I ran ./mach run and in the browser console I entered Services.prefs.prefHasUserValue("security.nocertdb") and got false.

Then I ran the test as before:

./mach mochitest toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul

and got

  TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul | Checking update.errorCode == MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE. If removing or changing this test, please notify tbb-dev@lists.torproject.org - got 2153394164, expected 2153398272

Next, I tried running the test with the --setpref flag as you suggested:

./mach mochitest --setpref=security.nocertdb=false toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul

and the result I got was:

  TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul | Checking update.errorCode == MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE. If removing or changing this test, please notify tbb-dev@lists.torproject.org - got 2153394164, expected 2153398272
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
checkErrorCode@chrome://mochitests/content/chrome/toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul:61:3

I then tried setting the flag to true:

./mach mochitest --setpref=security.nocertdb=true toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul

and I got:

  TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul | Checking update.errorCode == MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE. If removing or changing this test, please notify tbb-dev@lists.torproject.org - got 2153390067, expected 2153398272
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
checkErrorCode@chrome://mochitests/content/chrome/toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul:61:3

In sum, I think my build does not have Tor Browser pref settings, by default it should be operating equivalent to the pref being set to false. So when security.nocertdb is default or false, I get the unexpected error code of 2153394164 and when security.nocertdb is set to true, I get the unexpected error code of 2153390067. Both error values are different from the expected error code 2153398272.

Next I'm going to try these same set of experiments on tor-browser.git.

Last edited 6 months ago by arthuredelstein (previous) (diff)

comment:3 Changed 6 months ago by arthuredelstein

I built tor-browser.git and confirmed in about:config that security.nocertdb is set to true.

Then I ran the same 3 tests as before:

./mach mochitest toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul

gave

56 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul | Checking update.errorCode == MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE - got 2153390067, expected 2153398272

Then

./mach mochitest --setpref=security.nocertdb=false toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul

resulted in all tests passing. Finally,

./mach mochitest --setpref=security.nocertdb=true toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul

gave the result

56 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/chrome/test_0790_check_certPinning_noUpdate.xul | Checking update.errorCode == MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE - got 2153390067, expected 2153398272

So to analyze: my observation in the description tor-browser-52.6.0esr-8.0-2 was irrelevant because I had left security.nocertdb=true (Both branches give the same unexpected error code of 2153390067, but that's not very relevant.). If we look at security.nocertdb=false only, then I see that the test passes for tor-browser-52.6.0esr-8.0-2 but it fails on top of mozilla-central with a different unexpected error code of 2153394164.

comment:4 Changed 6 months ago by arthuredelstein

So my question is, does it matter that we're getting a different error code? I am wondering whether any error code will do, or if we need MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE specifically. (I'm probably still naive about the design of the test.)

I haven't figured out what this new error code, 2153394164 is intended to mean. I guess the next step for me might be to try to use gdb to track down where the old and new error codes are coming from.

comment:5 in reply to:  4 Changed 6 months ago by mcs

Replying to arthuredelstein:

So my question is, does it matter that we're getting a different error code? I am wondering whether any error code will do, or if we need MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE specifically. (I'm probably still naive about the design of the test.)

It probably does not matter too much, although it is designed to look specifically for a pinning failure. Let me think about what to do.

I haven't figured out what this new error code, 2153394164 is intended to mean. I guess the next step for me might be to try to use gdb to track down where the old and new error codes are coming from.

Yes, although I don't know how much work that will be. Let me know if you want me to make an attempt (although I won't get to it until next week at this point). The new error code is 0x805A2FF4 which is SSL_ERROR_BAD_CERT_DOMAIN so it isn't too far off in terms of the expected error. But I think we should dive deep enough to understand what is happening.

comment:6 Changed 6 months ago by mcs

After some time in the debugger comparing execution path of Tor Browser vs. a mozilla-central build, I found the problem: to get the correct error code we need to enable strict enforcement for pinning. I will attach a patch for the test which works for me.

Changed 6 months ago by mcs

Attachment: bug25331-patch.txt added

patch for test

comment:7 Changed 5 months ago by arthuredelstein

Cc: arthuredelstein added

comment:8 Changed 4 months ago by arthuredelstein

Cc: gk added
Keywords: TorBrowserTeam201804R added
Status: needs_informationneeds_review

This patch looks good to me. I have included it my nascent ESR60 branch. We could also consider including it in the ESR52 branches.

comment:9 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Fine with me. To save a round trip I just pushed the patch to tor-browser-52.7.3esr-8.0-1 (commit 292456846d8420e88b19664e36562cb4d6169776) and tor-browser-52.7.3esr-7.5-1 (commit 8592446c194092c93c854611fc7dadd1fd3f3917) mentioning that mcs and brade wrote it. Thanks!

Note: See TracTickets for help on using tickets.