1606317 - ignorehttpserrors.spec.js:30:5 - FAILED while running "beforeEach" in suite "Firefox ignoreHTTPSErrors"

Status: RESOLVED FIXED

(Remote Protocol :: CDP, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Puppeteer unit tests for ignorehttpserrors.spec.js currently fail because of:

Error: Protocol error (Security.setIgnoreCertificateErrors): Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsICertOverrideService.setDisableAllSecurityChecksAndLetAttackersInterceptMyData] setIgnoreCertificateErrors@chrome://remote/content/domains/parent/Security.jsm:51:25

Reason here is that this method cannot be called via the Remote Protocol right now due to access limitations:

https://searchfox.org/mozilla-central/rev/a46d0fc0e7bf06b64a8d27e2cdbeeee94ecc7ad1/security/manager/ssl/nsCertOverrideService.cpp#614-615

We will have to add an exclusion for the Remote Protocol too. I wonder why the tests in remote/tests/browser/security/browser_setIgnoreCertificateErrors.js work. Maybe it's failing by Puppeteer because it's getting set before the browser gets started?

Andreas, given that you implemented that for Marionette and Remote Agent maybe you could have a look at? Right now this is causing a crash of Firefox when running Puppeteer unit tests.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Thanks for the bug and doing all the investigation work!

We will have to add an exclusion for the Remote Protocol too. I wonder why the tests in remote/tests/browser/security/browser_setIgnoreCertificateErrors.js work. Maybe it's failing by Puppeteer because it's getting set before the browser gets started?

I think I know why: Marionette is being used to bootstrap bc tests, so MOZ_MARIONETTE is already set.

I’m not sure exactly how valuable or safe that security check is… it seems almost too easy to bypass.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1606317: security: use nsIMarionette.running() for disable security check;

It must only be possible to call
nsCertOverrideService::SetDisableAllSecurityChecksAndLetAttackersInterceptMyData()
when Marionette is actually active, but the MOZ_MARIONETTE environment
variable can in theory be set by any user.

MOZ_MARIONETTE was introduced to support in-application restarts
so that the forked main process knows to re-initialise Marionette.
This makes it approximately equivalent to passing the --marionette flag.

Because Marionette can be started and stopped at runtime through
modifying the marionette.enabled preference, and Marionette never
resets MOZ_MARIONETTE, this makes it theoretically possible that
a future caller could circumvent this security check.

This is however not a security problem at present because the
method is only ever called from within testing/marionette/cert.js,
which itself is never called unless Marionette indeed is active.

Still, it would be safer for this to use nsIMarionette.running()
which returns true whenever the Marionette server is listening for
connections, and false when the Marionette automation protocol is
not enabled.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1606317: security: allow remote agent to disable security checks;

The remote agent is an implementation of a subset of
the Chromium Remote Debugging Protocol (CDP) for Gecko.
For similar reasons as Marionette it needs the ability to call
nsCertOverrideService::SetDisableAllSecurityChecksAndLetAttackersInterceptMyData().

It calls this method from remote/domains/parent/Security.jsm which
implements the Security.setIgnoreCertificateErrors protocol method.

The remote agent is slated to replace Marionette, but there is
currently no timeline for this.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1606317 - [remote] Disable Puppeteer tests in ignorehttpserrors.spec.js which crash the browser. r=#remote!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f6327c0d829
[remote] Disable Puppeteer tests in ignorehttpserrors.spec.js which crash the browser.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andreas, I haven't seen that you already attached patches on this bug. As such the Puppeteer tests are disabled by the patch as just pushed. Can you please make sure that you revert this change with your patches, and test that those unit tests pass now? Thanks.

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9f6327c0d829

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1606317: remote: revert "Disable Puppeteer tests in ignorehttpserrors.spec.js which crash the browser."

This reverts the commit:

git:	d640ab64b993431099cd9205297bfff464d76927
hg:	9f6327c0d8298ec58a6c9f8d51526872d01d765e

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andreas, can you please run a try build with your changes? Thanks.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like that was already done via:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=34f6ec7febd53eb3784e6f41291d08fd5f7c65ef

Comment 11

[Andreas Tolfsen ❲:ato❳]

There are definitely still some errors originating in ignorehttpserrors.spec.js:
https://firefoxci.taskcluster-artifacts.net/I-XgnN_qS0WH1J3lp26_pQ/0/public/logs/live_backing.log

But I can’t see us failing on NS_ERROR_NOT_AVAILABLE anymore.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that there are no crashes we should clearly get those tests enabled, and seeing them failing. Thanks.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b12669c3ade7
security: use nsIMarionette.running() for disable security check; r=keeler
https://hg.mozilla.org/integration/autoland/rev/467db2b310a7
security: allow remote agent to disable security checks; r=keeler
https://hg.mozilla.org/integration/autoland/rev/9c34a0a40e62
remote: revert "Disable Puppeteer tests in ignorehttpserrors.spec.js which crash the browser." r=remote-protocol-reviewers,whimboo

Comment 14

[Cristina Coroiu [:ccoroiu]]

Backed out 3 changesets (bug 1606317) for build bustage at build/src/security/manager/ssl/nsCertOverrideService.cpp

Backout: https://hg.mozilla.org/integration/autoland/rev/43bbf560ac63559ffd2bdf9aafe3a62df87dd55d

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9c34a0a40e623f5d2f8937b7899c62e53d6a65cd

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283338331&repo=autoland&lineNumber=24764

[task 2020-01-03T13:02:26.204Z] 13:02:26 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 --target=arm-linux-androideabi -o CoseTest.o -c -flto=thin -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -fstack-protector-strong -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/security/manager/ssl/tests/gtest -I/builds/worker/workspace/build/src/obj-firefox/security/manager/ssl/tests/gtest -I/builds/worker/workspace/build/src/security/certverifier -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/third_party/rust/cose-c/include -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include/arm-linux-androideabi -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include -gcc-toolchain /builds/worker/fetches/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 -D__ANDROID_API_=16 -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -mno-unaligned-access -fno-sized-deallocation -fno-aligned-new -fno-short-enums -fno-exceptions -stdlib=libstdc++ -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/fetches/android-ndk/sources/android/support/include -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++abi/include -march=armv7-a -mthumb -mfpu=neon -mfloat-abi=softfp -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Oz -fno-omit-frame-pointer -funwind-tables -Werror -Wno-error=shadow -MD -MP -MF .deps/CoseTest.o.pp /builds/worker/workspace/build/src/security/manager/ssl/tests/gtest/CoseTest.cpp
[task 2020-01-03T13:02:26.204Z] 13:02:26 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/manager/ssl/tests/gtest'
[task 2020-01-03T13:02:26.206Z] 13:02:26 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/downloads'
[task 2020-01-03T13:02:26.206Z] 13:02:26 INFO - mkdir -p '.deps/'
[task 2020-01-03T13:02:26.206Z] 13:02:26 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/downloads'
[task 2020-01-03T13:02:26.206Z] 13:02:26 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/downloads'
[task 2020-01-03T13:02:26.206Z] 13:02:26 INFO - toolkit/components/downloads/DownloadPlatform.o
[task 2020-01-03T13:02:26.206Z] 13:02:26 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/downloads'
[task 2020-01-03T13:02:26.381Z] 13:02:26 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/manager/ssl'
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 --target=arm-linux-androideabi -o Unified_cpp_security_manager_ssl1.o -c -flto=thin -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -fstack-protector-strong -DNDEBUG=1 -DTRIMMED=1 -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES=True -DNSS_ENABLE_ECC=True -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/obj-firefox/security/manager/ssl -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/crypto -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/security/certverifier -I/builds/worker/workspace/build/src/obj-firefox/dist/public/nss -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include/arm-linux-androideabi -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include -gcc-toolchain /builds/worker/fetches/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 -D__ANDROID_API_=16 -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -mno-unaligned-access -fno-sized-deallocation -fno-aligned-new -fno-short-enums -fno-exceptions -stdlib=libstdc++ -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/fetches/android-ndk/sources/android/support/include -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++abi/include -march=armv7-a -mthumb -mfpu=neon -mfloat-abi=softfp -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Oz -fno-omit-frame-pointer -funwind-tables -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -MD -MP -MF .deps/Unified_cpp_security_manager_ssl1.o.pp Unified_cpp_security_manager_ssl1.cpp
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - In file included from Unified_cpp_security_manager_ssl1.cpp:47:
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - /builds/worker/workspace/build/src/security/manager/ssl/nsCertOverrideService.cpp:23:10: fatal error: 'nsIRemoteAgent.h' file not found
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - #include "nsIRemoteAgent.h"
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - ^~~~~~~~~~~~~~~~~~
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - 1 error generated.
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - /builds/worker/workspace/build/src/config/rules.mk:736: recipe for target 'Unified_cpp_security_manager_ssl1.o' failed
[task 2020-01-03T13:02:26.385Z] 13:02:26 ERROR - make[4]: *** [Unified_cpp_security_manager_ssl1.o] Error 1
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/manager/ssl'
[task 2020-01-03T13:02:26.385Z] 13:02:26 INFO - make[4]: *** Waiting for unfinished jobs....
[task 2020-01-03T13:02:26.386Z] 13:02:26 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/extensions'
[task 2020-01-03T13:02:26.386Z] 13:02:26 INFO - mkdir -p '.deps/'

Comment 15

[Andreas Tolfsen ❲:ato❳]

Missing #ifdef ENABLE_REMOTE_AGENT because it is not available on Windows AArch64 or Android.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cfc768072ca
security: use nsIMarionette.running() for disable security check; r=keeler
https://hg.mozilla.org/integration/autoland/rev/021ae503f6db
security: allow remote agent to disable security checks; r=keeler
https://hg.mozilla.org/integration/autoland/rev/2f1189b07810
remote: revert "Disable Puppeteer tests in ignorehttpserrors.spec.js which crash the browser." r=remote-protocol-reviewers,whimboo

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2cfc768072ca
https://hg.mozilla.org/mozilla-central/rev/021ae503f6db
https://hg.mozilla.org/mozilla-central/rev/2f1189b07810

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Note that even with all of these tests enabled, they are still failing. But as it looks like not due a missing feature of the ignoreHTTPSErrors option, but for the missing implementation for response.securityDetails().

Andreas, thanks for getting this fixed!

Comment 19

[Andreas Tolfsen ❲:ato❳]

One step further (-:

securityDetails appears to be a field on Network.Response which is returned as part of the Network.responseReceived event, tracked by bug 1549510.