Closed Bug 1547217 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Application Update, defect, P3)

Product:
Toolkit
Component:
Application Update
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: petr.sumbera, Assigned: glandium)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Bug 1547217 - Reshuffle how verifymar is linked.
47 bytes, text/x-phabricator-request
jbeich
: feedback+
jcristau
: approval-mozilla-beta+
Details | Review

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

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)

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

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
status-firefox67: --- → unaffected
status-firefox68: --- → affected
status-firefox69: --- → affected
status-firefox-esr60: --- → unaffected
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.

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.

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

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

Jan, are you building using system NSS?

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.

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

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.

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.

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.

Use NSS on all nix platforms for verifying mar files

Assignee: nobody → jbeich
Status: REOPENED → ASSIGNED
Priority: -- → P3

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
Attachment #9070683 - Attachment is obsolete: true

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

Built fine on FreeBSD 13 (-CURRENT) amd64.

Attachment #9071431 - Flags: feedback+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/de9be674a6db
Reshuffle how verifymar is linked. r=chmanchester

https://hg.mozilla.org/mozilla-central/rev/de9be674a6db

Status: ASSIGNED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox69: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Can you backport to Firefox 68 (ESR)?

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

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+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: