Closed Bug 1902078 Opened 5 months ago Closed 3 months ago

Make shlibsign output reproducible

Categories

(NSS :: Tools, enhancement)

Product:
NSS
Component:
Tools
Version:
3.99
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ghs9az, Assigned: rrelyea)

Details

Attachments

(3 files)

fixed-hmac-key.patch
2.89 KB, patch
Details | Diff | Splinter Review
Bug 1902078 - Allow handing in keymaterial to shlibsign to make the output reproducible (r=#nss-reviewers)
48 bytes, text/x-phabricator-request
Details | Review
Bug 1902078 - Adding basicutil.h to use HexString2SECItem function
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/125.0.0.0 Safari/537.36

Steps to reproduce:

shlibsign is used to sign libfreebl3.so, libnssdbm3.so, and libsoftokn3.so. shlibsign generates a unique key on every invocation. This means that the resulting signature will always be different.

ChromeOS is working on switching to building packages using bazel and if things are not reproducible, we end up cache busting all the reverse dependencies anytime a rebuild happens. Debian also has a note about shsignlib not being reproducable: https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/nss.html.

We should add a mechanism that allows generating a stable CHK file. The simplest solution would be to hard code a HMAC key into shlibsign.c and add a flag to use that instead of generating a random one. The other option is to have a flag that allows passing in the key or a file containing the key.

Thoughts?

Actual results:

I ran shsignlib twice and ended up with different signatures:

shlibsign -vv -i /build/brya/usr/lib64/libfreebl3.so -o /tmp/3.chk
moduleSpec configdir='' certPrefix='' keyPrefix='' secmod='' flags=noCertDB, noModDB
Library File: /build/brya/usr/lib64/libfreebl3.so
Check File: /tmp/3.chk
Generate an HMAC key ...
Library File Size: 7584 bytes
  key: 32 bytes
    11 94 8a ac ee 93 c6 6f f5 fe
    77 7f 52 fd f1 c4 8b a7 08 42
    7a d2 9b 65 2b 79 a8 c1 fd bb
    09 94
  signature: 32 bytes
    93 14 8a da ea c2 9e 7b d5 c1
    f3 ad 47 ba 20 f4 b5 09 b4 4f
    36 a6 1a b6 97 b8 be 97 d6 a7
    b8 7d

shlibsign -vv -i /build/brya/usr/lib64/libfreebl3.so -o /tmp/3.chk
moduleSpec configdir='' certPrefix='' keyPrefix='' secmod='' flags=noCertDB, noModDB
Library File: /build/brya/usr/lib64/libfreebl3.so
Check File: /tmp/3.chk
Generate an HMAC key ...
Library File Size: 7584 bytes
  key: 32 bytes
    ca 9a 28 68 4e 43 dd 2d 86 a9
    92 b4 d1 a6 da aa 63 c9 62 5a
    dc a5 19 92 98 b7 39 b6 9e 8f
    57 fa
  signature: 32 bytes
    70 79 b1 7f 1d 33 0b 23 68 94
    ac 1b 43 26 47 38 1e 77 67 e7
    c6 ed b0 2a 9e 33 06 e7 e9 49
    66 e8

Expected results:

The output should have been identical.

Uses a hard coded HMAC key when the hash type is not specified.

I've attached a patch that uses a fixed HMAC key if the hash wasn't specified on the command line.
FWIW, I also pushed this up to the chromiumos repo: https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/5632908

I'm not a huge fan of fixed magic keys, so I've built on top of Rauls patch and created an option to hand in key-material from the commandline, in order to get the output reproducible.

Bob, I think this is sort-of your purview - what do you think? Also, I was curious what exactly FIPS says about the integrity self-test requirement. Would just hashing the library files with e.g. SHA256 and comparing the value be acceptable? Storing the secret HMAC key along with the HMAC itself doesn't provide any meaningful security...

Flags: needinfo?(rrelyea)

So yes, I don't think we can do this (replace HMAC with Hash). FIPS explicitly requires signing (even though we pointed out that it's not real signing). At least we are able to sign with HMAC rather than DSA/ECDSA like we used to. These values are only relevant to FIPS mode (when your not in FIPS mode, they may be checked, but they aren't determinative.

I think msirringhaus's patch is the better idea. Chromiumos can have their own builds with their own fixed keys if variable keys causes them problems. The can even take other's prebuilt versions and sign them themselves with their own keys. My only comment on the patch would be there has got to be a hex 2 bin function somewhere in cmd/lib that we can use rather than coding a new one. If not we should put the hex2bin in cmd/lib for future use in other places.

Flags: needinfo?(rrelyea)

The problem is that shlibsign was intentionally built to not use anything other than NSPR and libsoftokn, see bug 347037.
I didn't want to change that, so I can't really pull in anything external.
So, we would need to add that function to NSPR, make a new release of that, etc. Also I'm not 100% sure it would fit into NSPR in the first place.

To add insult to injury, the initial function I based this on is already copy&pasted in three different locations, too:
https://searchfox.org/mozilla-central/source/security/nss/cmd/bltest/blapitest.c#254
https://searchfox.org/mozilla-central/source/security/nss/cmd/fipstest/fipstest.c#59
https://searchfox.org/mozilla-central/source/security/nss/cmd/pk11gcmtest/pk11gcmtest.c#14

I'm also not happy with this, but currently see no 'easy' way to consolidate that. I'm open to suggestions, though! I'd be happy to also implement this the 'hard' way, if we can agree on one.

cmd/lib is a static library that all of cmd links with, so we could put it in cmd/lib and have all of those call into it.

Ok, after talking with our FIPS-people, it seems that in this particular situation the FIPS-standard only cares about the verification-part of the checksums, not how they are created. So we are free to link even lib/util, which has a dedicated function already, making this patch considerably shorter.
I've updated the patch accordingly (even though this basically reverts the Makefile-part of bug 347037).

Assignee: nobody → rrelyea
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Dana, For some reason lando doesn't work for me. I can land this, it's just I have to extract the patch and apply it, then check it into hg by hand, Can you push the lando button for me? (If not I'll go ahead and land this myself in the next day or so).

Flags: needinfo?(dkeeler)

I don't actually have landing permissions for NSS. John?

Flags: needinfo?(dkeeler) → needinfo?(jschanck)

https://hg.mozilla.org/projects/nss/rev/be8e6fa285fa4642bf66b339fecce1fca78cbb05

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Flags: needinfo?(jschanck)
Resolution: --- → FIXED

This change is breaking NSS builds. Surprised that our CI didn't catch that. See https://github.com/mozilla/neqo/actions/runs/10157687760/job/28088270178#step:5:1306

cc -MMD -MF obj/cmd/shlibsign/shlibsign.shlibsign.o.d '-DSHLIB_SUFFIX="so"' '-DSHLIB_PREFIX="lib"' -DNSS_FIPS_DISABLED -DNSS_NO_INIT_SUPPORT -DNSS_X86_OR_X64 -DNSS_X64 -DNSS_USE_64 -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -DSDB_MEASURE_USE_TEMP_DIR -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_DBM -DNSS_DISABLE_LIBPKIX -DNDEBUG -I/home/runner/work/neqo/neqo/dist/Release/include/nspr -I/home/runner/work/neqo/neqo/dist/private/nss -I/home/runner/work/neqo/neqo/dist/public/nss -fPIC -pipe -ffunction-sections -fdata-sections -m64 -Werror -Wall -Wshadow -O2 -ggdb3 -fno-omit-frame-pointer -std=c99  -c ../../cmd/shlibsign/shlibsign.c -o obj/cmd/shlibsign/shlibsign.shlibsign.o
../../cmd/shlibsign/shlibsign.c: In function ‘shlibSignHMAC’:
../../cmd/shlibsign/shlibsign.c:1135:13: error: implicit declaration of function ‘SECU_HexString2SECItem’ [-Werror=implicit-function-declaration]
 1135 |         if (SECU_HexString2SECItem(NULL, &keyitem, key) == NULL) {
      |             ^~~~~~~~~~~~~~~~~~~~~~
../../cmd/shlibsign/shlibsign.c:1135:57: error: comparison between pointer and integer [-Werror]
 1135 |         if (SECU_HexString2SECItem(NULL, &keyitem, key) == NULL) {
      |                                                         ^~
cc1: all warnings being treated as errors

I wonder if we can add a task to our CI that we can catch these type of bugs

Upd: ah, Lars told pretty much the same

Oh, sorry! I forgot an #include "basicutil.h", it seems. I think it slides through, if export NSS_ENABLE_WERROR=0 is set.
Does this get backed out now, or do I have to open a follow-up bug to submit the fix?

Attachment #9416759 - Attachment description: WIP: Bug 1902078 - Adding basicutil.h to use HexString2SECItem function → Bug 1902078 - Adding basicutil.h to use HexString2SECItem function

(In reply to msirringhaus from comment #15)

I think it slides through, if export NSS_ENABLE_WERROR=0 is set.

Is the CI setting this? (It shouldn't.)

Just adding basicutils.h does not seem to solve that: https://treeherder.mozilla.org/jobs?repo=nss-try&revision=73498b0e1076

(In reply to Anna Weine from comment #18)

Just adding basicutils.h does not seem to solve that: https://treeherder.mozilla.org/jobs?repo=nss-try&revision=73498b0e1076

I don't think thats my bug now. Looks like fallout from https://phabricator.services.mozilla.com/D217915 ? There, dirent was removed, and now the build fails, because it wants to link dirent.o

Yes, I am checking the same thing

I'm think you have to remove those:

❯ rg dirent
lib/dbm/src/src.gyp
14:        'dirent.c',

lib/dbm/src/manifest.mn
25:     dirent.c   \

Then it builds locally for me.

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: