Opened 4 years ago

Last modified 21 months ago

#15532 new defect

Tor Browser 4.5 displays signature validation error during update

Reported by: mikeperry Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-firefox-patch, ff38-esr
Cc: mcs, brade, gk, sukhbir, Digitalcourage Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I suspect this is due to the fact that we allow an update to proceed if it is signed with either my mar signing key or gk's mar signing key, but nonetheless TBB 4.5 displays two error messages while updating on Linux:
"ERROR: Error verifying signature"
"ERROR: Not all signatures were verified".

We should ensure the signature validation behavior is actually correct, and if so remove these error messages for the stable release.

Child Tickets

Change History (12)

comment:1 Changed 4 years ago by mikeperry

Oh, also these messages were printed to the terminal. Not sure if it was stdout or stderr.

comment:2 Changed 4 years ago by arma

I got this too. Does this error block the upgrade or just chatter in the log window?

(My upgrade turned into a terrible disaster because I was low on disk space, and I ended up blowing away the whole thing, which is why I don't know the answer.)

comment:3 in reply to:  2 Changed 4 years ago by gk

Replying to arma:

I got this too. Does this error block the upgrade or just chatter in the log window?

"Just" the latter. IIRC the code is trying the first key first (which Mike has) and is complaining with said error message if the MAR files are not signed with it.

comment:4 Changed 4 years ago by mcs

The difficulty in fixing this bug is that Mozilla tends to consider stderr as something that most users will ignore. In this specific case, a function named mar_verify_signatures() that is in modules/libmar/verify/mar_verify.c is called twice by the updater code. The first call, with the cert that was not used to sign the 4.5a5 MAR files, causes two error lines to be sent to stderr.

Unfortunately, the code in mar_verify.c contains 32 fprintf(stderr, "ERROR: ...") statements. The information that is output could be very useful if there is a real problem with MAR file signature verification, and mar_verify.c is also used by the signmar command line program (where it makes sense to have the stderr output).

Maybe we can add a bool parameter to mar_verify_signatures() that controls whether messages are written to stderr. Then we could enable them for the signmar command line program and disable them for the updater. I think the patch to do this will be somewhat large but fairly simple. We might even be able to make it so that the messages are logged to stderr when the app.update.log pref. is true, although that is a little tricky because the updater program itself cannot read prefs. But we could use an environment variable like Mozilla does for other things; look for PR_SetEnv() calls inside toolkit/xre/nsUpdateDriver.cpp.

comment:5 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201504 added

comment:6 Changed 4 years ago by mikeperry

The other thing to consider is that stdout+stderr is now hidden from the user on all platforms. Linux was the last one, and that was fixed with #13375+#12468. So this should be way less visible now than it was in 4.5a4... If this is an involved change, it may not be worth it for 4.5-stable?

Perhaps we can revisit it after dealing with the ff38-esr bitrot?

comment:7 in reply to:  6 Changed 4 years ago by mcs

Keywords: tbb-firefox-patch ff38-esr added; tbb-4.5-alpha TorBrowserTeam201504 removed

Replying to mikeperry:

The other thing to consider is that stdout+stderr is now hidden from the user on all platforms. Linux was the last one, and that was fixed with #13375+#12468. So this should be way less visible now than it was in 4.5a4... If this is an involved change, it may not be worth it for 4.5-stable?

Kathy and I agree that it makes sense to postpone this.

comment:8 Changed 3 years ago by verifiervictim

I'm still getting this even when upgrading from 5.0.2 to 5.0.3 on Linux.

This is going to make users worry that they're installing hacked software from a malicious exit node, if they download updates via Tor.

comment:9 Changed 2 years ago by mcs

Cc: sukhbir added
Severity: Normal

This will also affect Tor Messenger's updater.

comment:10 Changed 2 years ago by sukhbir

I had the same error message with Tor Messenger's updater and I fixed it by copying the DER to both release_primary and release_secondary. (I actually did think there was an error in verifying the update and spent quite a while trying to figure out why.)

IMO this would be the easiest way to fix it unless I am missing something else.

comment:11 Changed 2 years ago by gk

As I said on IRC you don't want to have just one key baked in. Think about losing the key/having it compromised. How are you updating your users? You can't sign the MAR files with the new key you are about to bake in. Even if that would still work (because you just want to rotate to a new key) every user would need to update to that particular version. Let's assume you need to get a chemspill release out the week afterwards if you used your new key to sign the MAR files a considerable amount of users will have a broken update experience as they won't have updated to the version with the new signing key baked in yet.

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

comment:12 Changed 21 months ago by gk

Cc: Digitalcourage added
Note: See TracTickets for help on using tickets.