Opened 4 months ago

Closed 4 months ago

Last modified 4 days ago

#27220 closed defect (fixed)

Allow TBA to install tor button, tor launcher and https everywhere extensions without signatures

Reported by: igt0 Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201808, tbb-mobile
Cc: sysrqb, gk, dmr Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor8

Description

When trying to install torbutton, torlauncher or https-e. TBA doesn't allow it because they don't have signatures.

The distribution code path is different for mobile and desktop.

Child Tickets

Attachments (2)

Change History (20)

comment:1 Changed 4 months ago by igt0

Cc: sysrqb gk added
Status: newneeds_review

comment:2 Changed 4 months ago by sysrqb

We don't modify https-everywhere, so it shouldn't fail the signing requirement. Maybe we can only add the exception for torbutton and tor-launcher?

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

Replying to sysrqb:

We don't modify https-everywhere, so it shouldn't fail the signing requirement. Maybe we can only add the exception for torbutton and tor-launcher?

Agreed. I think it's worth being even more conservative and adding only exceptions for things we actually need, meaning only Torbutton for the alpha for now.

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

comment:5 Changed 4 months ago by igt0

Status: needs_revisionneeds_review

comment:6 Changed 4 months ago by gk

Looks good to me. But the security settings are still not enabled by default on my phone. I.e. while messing with the about:config is not necessary anymore, I still need sysrqb's trick in comment:11:ticket:26884. The bundle I used is available in comment:23:ticket:26884.

comment:7 Changed 4 months ago by sysrqb

Do we need to change mustSign()? I haven't tested this yet, but the code path I see will still fail - maybe I'm looking in the wrong place. I'll test this, as well.

comment:8 Changed 4 months ago by igt0

The issue is not about signatures anymore. Looks like there is a race condition in the addons code. If i close the browser and open again. The addon is loaded.

comment:9 in reply to:  8 ; Changed 4 months ago by sysrqb

Replying to igt0:

The issue is not about signatures anymore. Looks like there is a race condition in the addons code. If i close the browser and open again. The addon is loaded.

Ah, I see, the patch is good. loadManifest() => loadManifestFromZipReader() => verifyZipSignedState() => shouldVerifySignedState().

verifyZipSignedState() sets

signedState: AddonManager.SIGNEDSTATE_NOT_REQUIRED

(where AddonManager.SIGNEDSTATE_NOT_REQUIRED has value undefined), so the inner conditional block isn't executed:

    if (mustSign(this.addon.type)) {
      if (this.addon.signedState <= AddonManager.SIGNEDSTATE_MISSING) {
        [...]

        if (state == AddonManager.SIGNEDSTATE_MISSING)
          return Promise.reject([AddonManager.ERROR_SIGNEDSTATE_REQUIRED,
                                 "signature is required but missing"]);

       [...]
      }
    }

As for the race condition, I don't think that's true. I think that is because torbutton isn't a restartless extension. It requires restarting after installation. I don't think there's anyway way we can prevent this. I wonder if we can force a restart (semi-transparently) at the end of the firstrun onboarding screen.

comment:10 in reply to:  4 Changed 4 months ago by gk

Replying to igt0:

Updated patch.

https://trac.torproject.org/projects/tor/attachment/ticket/27220/0001-Bug-27220-Don-t-verify-signatures-for-tb-tl-and-http.2.patch

Applied to tor-browser-60.1.0esr-8.0-1 (commit b9aecf44142c7874de51bd29abdcd40dcf3e6cb2).

comment:11 in reply to:  9 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to sysrqb:

As for the race condition, I don't think that's true. I think that is because torbutton isn't a restartless extension. It requires restarting after installation. I don't think there's anyway way we can prevent this. I wonder if we can force a restart (semi-transparently) at the end of the firstrun onboarding screen.

I agree that leaving the user without Torbutton is suboptimal especially as they would be looking for the slider settings (I assume we advertise those) and might not restart the browser (who is restarting things today anyways unless one is not forced too?). Is there some quick fix for this then we could include it in the alpha. If it's more elaborate and we need to think more I don't want to delay the alpha further for that (we can add the restart this to the TBA alpha "manual"). Anyway, I think that's for a new ticket.

comment:12 Changed 4 months ago by tom

Can anyone confirm that this doesn't allow someone to install any random add-on that they name torbutton@… ?

comment:13 in reply to:  12 Changed 4 months ago by gk

Replying to tom:

Can anyone confirm that this doesn't allow someone to install any random add-on that they name torbutton@… ?

Good question. I've not looked into the mobile side of this but for the desktop I wrote up https://lists.torproject.org/pipermail/tbb-dev/2017-April/000517.html and it seemed to me that we are not in a bad shape back then.

comment:14 in reply to:  12 Changed 4 months ago by gk

Replying to tom:

Can anyone confirm that this doesn't allow someone to install any random add-on that they name torbutton@… ?

FWIW this gets addressed with the patch for #27271.

comment:15 Changed 4 months ago by dmr

Cc: dmr added

comment:16 Changed 4 months ago by gk

Keywords: TorBrowserTeam201808 tbb-mobile added

comment:17 Changed 7 weeks ago by pili

Sponsor: Sponsor8

comment:18 Changed 4 days ago by gk

Sponsor8 in August 2018.

Note: See TracTickets for help on using tickets.