Opened 7 months ago

Last modified 2 months ago

#32895 needs_review enhancement

Improve marsigning_check.sh script to deal better with non-reproducible, signed macOS mar files

Reported by: gk Owned by: gk
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-sign, tbb-maint, GeorgKoppen202006, TorBrowserTeam202006R
Cc: tbb-team Actual Points:
Parent ID: Points:
Reviewer: sysrqb Sponsor:

Description (last modified by gk)

Our current mar-signing check script does two things:

1) It checks whether the SHA-256 sum from the signed .mar file is the same one as from the unsigned one and returns an error if so.

2) It strips the signature and compares the SHA-256 sum of the resulting .mar file with the unsigned one.

Step 2) essentially tries to do 2 checks in one: a) that there is a proper signature that can get stripped and b) that the resulting .mar file is the same as the unsigned one. That's cool in theory as we want to have both checks but it has a number of issues in practice. The most important ones are:

i) The script fails the mar-signing check for macOS as stripping the signatures from those files does not give us the unsigned .mar yet due to the content signing. (see: #20254)

ii) It's not clear we signed actually with the right key (although that is in practice not much of an issue) or whether the signature verifies later on (which is actually what we want to know).

Child Tickets

Change History (9)

comment:1 Changed 7 months ago by gk

I guess what we could do here is to point to the cert which got supposedly used (like we point to the SIGNMAR binary) and the nickname and then do the actual verification + we make the output for 2a) and 2b) separate.

comment:2 Changed 3 months ago by gk

Description: modified (diff)
Keywords: tbb-maint GeorgKoppen202005 added
Owner: changed from tbb-team to gk
Status: newassigned

comment:3 Changed 3 months ago by gk

Cc: tbb-team added
Keywords: TorBrowserTeam202005R added
Reviewer: sysrqb
Status: assignedneeds_review

bug_32895 (https://gitweb.torproject.org/user/gk/tor-browser-build.git/commit/?h=bug_32895&id=fe4c5cf57e52aba7acb8b08ac80bd2c62672d7b7) has a patch for review. I fixed all the issues shellcheck found, too, while I was at it.

Last edited 3 months ago by gk (previous) (diff)

comment:4 Changed 3 months ago by gk

Type: defectenhancement

comment:5 Changed 2 months ago by gk

Keywords: TorBrowserTeam202006R added; TorBrowserTeam202005R removed

Moving review tickets.

comment:6 Changed 2 months ago by gk

Last edited 2 months ago by gk (previous) (diff)

comment:7 Changed 2 months ago by sysrqb

Status: needs_reviewneeds_revision

Can we print the error message for only non-osx64? Printing 35+ non-match messages seems like unnecessary noise.

+    if ! [ "$sha256_txt" = "$(sha256sum "$f")" ]
     then
       echo "$f does not have the SHA-256 sum of the unsigned MAR file!"
-      BADSIGNED_MARS=`expr $BADSIGNED_MARS + 1`
+      not_reproduced_mars=$((not_reproduced_mars + 1))
+      case "$f" in
+        *osx64*)
+          not_reproduced_mars_expected=$((not_reproduced_mars_expected + 1))

comment:8 Changed 2 months ago by sysrqb

Keywords: TorBrowserTeam202006 added; TorBrowserTeam202006R removed

comment:9 Changed 2 months ago by gk

Keywords: GeorgKoppen202006 TorBrowserTeam202006R added; GeorgKoppen202005 TorBrowserTeam202006 removed
Status: needs_revisionneeds_review
Note: See TracTickets for help on using tickets.