updater now uses Unified_c_libmar_verify0.o and fails to link due missing symbols from nss3 and nspr4

RESOLVED FIXED in Firefox 68

Status

()

defect
P3
normal
RESOLVED FIXED
2 months ago
6 days ago

People

(Reporter: petr.sumbera, Assigned: glandium)

Tracking

(Regression, {regression})

Trunk
mozilla69
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox67 unaffected, firefox68 fixed, firefox69 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 months ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

/usr/bin/g++ -o ../../../../dist/bin/updater -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++1z-compat -Wduplicated-cond -Wimplicit-fallthrough -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -Wformat-overflow=2 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O -fno-omit-frame-pointer -funwind-tables
@/builds/psumbera/FIREFOX/obj-sparc64-sun-solaris2.11/toolkit/mozapps/update/updater/updater.list -lpthread -fstack-protector-strong -Wl,-z,text -L/builds/psumbera/FIREFOX/obj-sparc64-sun-solaris2.11/dist/bin -pie -lsocket -L/usr/lib/sparcv9 -lgtk-3 -lgdk-3 -lpangocairo-1.0 -lpango-1.0 -latk-1.0 -lcairo-gobject -lcairo -lgdk_pixbuf-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 Undefined first referenced
symbol in file
VFY_DestroyContext ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
VFY_CreateContext ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
VFY_Update ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
CERT_GetDefaultCertDB ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
SECKEY_PublicKeyStrength ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
CERT_ExtractPublicKey ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
SECKEY_DestroyPublicKey ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
CERT_DestroyCertificate ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
VFY_EndWithSignature ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
NSS_NoDB_Init updater.o
PR_GetError updater.o
VFY_Begin ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
CERT_NewTempCertificate ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o
PR_ErrorToName updater.o
ld: fatal: symbol referencing errors
collect2: error: ld returned 1 exit status
gmake[4]: *** [/builds/psumbera/FIREFOX/config/rules.mk:527: ../../../../dist/bin/updater] Error 1
gmake[4]: Target 'target' not remade because of errors.
gmake[4]: Leaving directory '/builds/psumbera/FIREFOX/obj-sparc64-sun-solaris2.11/toolkit/mozapps/update/updater'

The same happens with updater-dep and updater-xpcshell.

This was not issue e.g. with changeset 465877:4a692c812a3f (Sun Mar 24 21:51:01 2019). Where updater simply didn't link Unified_c_libmar_verify0.o.

Component: Untriaged → Application Update
Product: Firefox → Toolkit
Reporter

Comment 1

2 months ago

It might be related to fix for Bug 1545712. At least 'hg bisect' tells me that...

Component: Application Update → Untriaged
Product: Toolkit → Firefox
Component: Untriaged → Application Update
Product: Firefox → Toolkit
Regressed by: 1545712

Most likely. Adding --disable-verify-mar should get it back to where it was before bug 1545712.

glandium, is there a way to make Solaris default to --disable-verify-mar? Thanks

Flags: needinfo?(mh+mozilla)
Reporter

Comment 4

2 months ago

I can confirm that '--disable-verify-mar' helps.

Comment 6

19 days ago

Same error on OpenBSD[1] and FreeBSD. Either --disable-verify-mar or --disable-updater should be default on Tier3 platforms or the build fixed.

[1] http://buildbot.rhaalovely.net/nine/#/builders/2/builds/489

$ c++ -v
FreeBSD clang version 8.0.0 (tags/RELEASE_800/final 356365) (based on LLVM 8.0.0)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

$ ./mach bootstrap
$ ./mach build -v
[...]
gmake[4]: Entering directory 'objdir/toolkit/mozapps/update/updater'
toolkit/mozapps/update/updater/updater
c++ -o ../../../../dist/bin/updater -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -I/usr/local/include -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 -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -O -fomit-frame-pointer -funwind-tables objdir/toolkit/mozapps/update/updater/updater.list -pthread -fstack-protector-strong -Wl,-z,noexecstack -Wl,-z,text -Wl,-z,relro -Wl,-z,nocopyreloc -Wl,-Bsymbolic-functions -Wl,-rpath-link,objdir/dist/bin -Wl,-rpath-link,/usr/local/lib -fcolor-diagnostics -pie -L/usr/local/lib -lgtk-3 -lgdk-3 -lpangocairo-1.0 -lpango-1.0 -latk-1.0 -lcairo-gobject -lcairo -lpthread -lgdk_pixbuf-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl
updater.o: In function main': updater.cpp:(.text.main+0x2f): undefined reference toNSS_NoDB_Init'
updater.cpp:(.text.main+0x7e3): undefined reference to PR_GetError' updater.cpp:(.text.main+0x7f6): undefined reference toPR_ErrorToName'
../../../../modules/libmar/verify/Unified_c_libmar_verify0.o: In function NSS_LoadPublicKey': Unified_c_libmar_verify0.c:(.text.NSS_LoadPublicKey+0x3e): undefined reference toCERT_GetDefaultCertDB'
Unified_c_libmar_verify0.c:(.text.NSS_LoadPublicKey+0x55): undefined reference to CERT_NewTempCertificate' Unified_c_libmar_verify0.c:(.text.NSS_LoadPublicKey+0x65): undefined reference toCERT_ExtractPublicKey'
Unified_c_libmar_verify0.c:(.text.NSS_LoadPublicKey+0x70): undefined reference to CERT_DestroyCertificate' ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o: In functionNSS_VerifyBegin':
Unified_c_libmar_verify0.c:(.text.NSS_VerifyBegin+0x24): undefined reference to SECKEY_PublicKeyStrength' Unified_c_libmar_verify0.c:(.text.NSS_VerifyBegin+0x5f): undefined reference toVFY_CreateContext'
Unified_c_libmar_verify0.c:(.text.NSS_VerifyBegin+0x6f): undefined reference to VFY_Begin' ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o: In functionNSS_VerifySignature':
Unified_c_libmar_verify0.c:(.text.NSS_VerifySignature+0x38): undefined reference to VFY_EndWithSignature' ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o: In functionReadAndUpdateVerifyContext':
Unified_c_libmar_verify0.c:(.text.ReadAndUpdateVerifyContext+0x6c): undefined reference to VFY_Update' ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o: In functionmar_verify_signatures':
Unified_c_libmar_verify0.c:(.text.mar_verify_signatures+0xa1): undefined reference to CERT_GetDefaultCertDB' Unified_c_libmar_verify0.c:(.text.mar_verify_signatures+0xb8): undefined reference toCERT_NewTempCertificate'
Unified_c_libmar_verify0.c:(.text.mar_verify_signatures+0xc8): undefined reference to CERT_ExtractPublicKey' Unified_c_libmar_verify0.c:(.text.mar_verify_signatures+0xd5): undefined reference toCERT_DestroyCertificate'
Unified_c_libmar_verify0.c:(.text.mar_verify_signatures+0x189): undefined reference to SECKEY_DestroyPublicKey' ../../../../modules/libmar/verify/Unified_c_libmar_verify0.o: In functionmar_verify_signatures_for_fp':
Unified_c_libmar_verify0.c:(.text.mar_verify_signatures_for_fp+0xb1): undefined reference to SECKEY_PublicKeyStrength' Unified_c_libmar_verify0.c:(.text.mar_verify_signatures_for_fp+0xd1): undefined reference toVFY_CreateContext'
Unified_c_libmar_verify0.c:(.text.mar_verify_signatures_for_fp+0xea): undefined reference to VFY_Begin' Unified_c_libmar_verify0.c:(.text.mar_verify_signatures_for_fp+0x1ea): undefined reference toVFY_DestroyContext'
Unified_c_libmar_verify0.c:(.text.mar_verify_signatures_for_fp+0x3ac): undefined reference to VFY_Update' Unified_c_libmar_verify0.c:(.text.mar_verify_signatures_for_fp+0x453): undefined reference toVFY_EndWithSignature'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Summary: Solaris - updater now uses Unified_c_libmar_verify0.o and fails to link due missing symbols from nss3 and nspr4 → updater now uses Unified_c_libmar_verify0.o and fails to link due missing symbols from nss3 and nspr4

glandium, do you know of a way to disable the updater by default on these platforms (e.g. Solaris, FreeBSD, etc.)?

Flags: needinfo?(mh+mozilla)

Jan, do you actually use the updater? If not, I'd prefer not building it by default on these platforms.

Comment 10

19 days ago

Updater maybe useful if Tor Browser starts supporting BSDs before Mozilla. Also the plan here is to backport the fix to ESR68. Changing defaults is too late and risky.

The problem I have with that is the updater without signing support is missing a very significant security implementation.

Comment 12

19 days ago

Can you point to the bug of the Linux implementation? If it's generic to ELF then it may work as is on BSDs.

Comment 13

19 days ago

--disable-updater is very poorly tested, see bug 1554742 or bug 1371159.

Jan, are you building using system NSS?

Comment 15

19 days ago

No. I'm building with empty .mozconfig or something close. There're no special instructions to build on BSDs compared to Linux.

$ pkg install python27
$ hash git 2>/dev/null || pkg install mercurial
$ hg clone https://hg.mozilla.org/mozilla-unified firefox ||
  git clone https://github.com/mozilla/gecko-dev firefox
$ cd firefox
$ hg update central || git checkout origin/master
$ echo "export CC=clang80 CXX=clang++80 # whichever mesa installs" >>.mozconfig
$ echo "ac_add_options --disable-debug-symbols" >>.mozconfig
$ ./mach bootstrap # select Firefox for Desktop
$ ./mach build
$ ./mach run

ftr, my OpenBSD buildbot uses

mk_add_options MOZ_OBJDIR=/build/obj/buildslave/mozilla-central
mk_add_options PYTHON=/usr/local/bin/python2.7
mk_add_options AUTOCLOBBER=1
export LLVM_CONFIG=/usr/local/bin/llvm-config
export LDFLAGS=-Wl,--no-keep-memory
export CC=/usr/bin/clang
export CXX=/usr/bin/clang++

only the betas and releases are built against system NSS.

I don't really see the point of enabling/building the updater on OpenBSD, since the binaries are provided by the packaging system (those are depending on specific versions of dependencies, since we control the versioning of all shared libraries), and i doubt mozilla (or anyone else) would ever provide mar/updates to tier3.

Comment 17

19 days ago

The distro package is a different story. Not even Linux uses updater downstream because it interferes with updates done by the packaging system. There .mozconfig is far from empty as some distros (including well-known like Fedora or Debian) enable official branding, system libs, etc. pass some variables and apply their own collection of patches that are either not suitable for upstream in current form or cherry-picks from upstream.

We're not going to bypass that by default anymore since it is a security feature and choosing to build the updater without that feature should at the very least require opting in and changing the mozconfig by adding ac_add_options --disable-verify-mar

The code that utilizes nss for mar verification is in updater.cpp and if it were to be fixed it should be done in a new bug.
https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#107
https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2695

Comment 19

19 days ago

updater.cpp doesn't need to be fixed, Solaris and BSDs already use the same code as Linux and Android. Do you actually object to syncing build glue?

The updater isn't used on Android but I think I get what you're saying. I'm in the middle of something at the moment and will look at this again on Monday or over the weekend.

Comment hidden (obsolete)

Comment 25

18 days ago

Can someone review the patch in comment 8 that was green on Try? I can't set review flag to ? because it was disabled in favor of Phabricator.

Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)

I can't set r? either but I have been told that phabricator will retain the author attribution so I'll submit it to phabricator.

Assignee: nobody → jbeich
Status: REOPENED → ASSIGNED
Assignee

Comment 32

15 days ago

The conditions under which verifymar is built were not aligned with what
kind of setups are actually doing something. For instance
--disable-signmar --enable-verify-mar was building the verifymar library
but not doing anything with it.

OTOH, building with --enable-signmar --disable-verify-mar did build it
but its code was eliminated at link time because it's unused.

Finally, the conditions between modules/libmar/verify/moz.build and
toolkit/mozapps/update/updater/updater-common.build weren't aligned and
broke some non-Linux tier-3 platforms. We remedy the latter by moving
the flags and libraries verifymar needs to verifymar, so that things
that link verifymar inherit them.

And while in the vicinity, replace a use of NSPR_LIBS with the
pseudo-library nspr which has the same effect.

Attachment #9071058 - Attachment is obsolete: true
Assignee

Updated

15 days ago
Attachment #9070683 - Attachment is obsolete: true

Comment 33

14 days ago

Comment on attachment 9071431 [details]
Bug 1547217 - Reshuffle how verifymar is linked.

Built fine on FreeBSD 13 (-CURRENT) amd64.

Attachment #9071431 - Flags: feedback+

Comment 34

7 days ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/de9be674a6db
Reshuffle how verifymar is linked. r=chmanchester

Comment 35

7 days ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 19 days ago7 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Comment 36

7 days ago

Can you backport to Firefox 68 (ESR)?

Assignee: jbeich → mh+mozilla
Flags: needinfo?(mh+mozilla)
Assignee

Comment 37

6 days ago

Comment on attachment 9071431 [details]
Bug 1547217 - Reshuffle how verifymar is linked.

Beta/Release Uplift Approval Request

  • User impact if declined: Build failure on non-Linux tier-3 platforms. Nice to have on esr68 for downstreams.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The only bad thing that can happen from this patch is things failing to build, which didn't happen when this landed on central.
  • String changes made/needed:
Flags: needinfo?(mh+mozilla)
Attachment #9071431 - Flags: approval-mozilla-beta?

Comment on attachment 9071431 [details]
Bug 1547217 - Reshuffle how verifymar is linked.

approved for 68.0b13

Attachment #9071431 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.