Opened 6 weeks ago

Closed 4 weeks ago

#30480 closed task (fixed)

rbm should check that a signed tag object contains the expected tag name

Reported by: boklm Owned by: boklm
Priority: Medium Milestone:
Component: Applications/rbm Version:
Severity: Normal Keywords: TorBrowserTeam201905R, tbb-rbm
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When we use the tag_gpg_id option, rbm will check that a tag is gpg signed. However it does not check that the tag object contains the expected tag name, and git does not check that either. As discussed in #30479, this can allow rollback attacks.

Child Tickets

Change History (5)

comment:1 Changed 6 weeks ago by boklm

Keywords: TorBrowserTeam201905R added
Status: newneeds_review

There is a patch for review in branch bug_30480:
https://gitweb.torproject.org/user/boklm/rbm.git/commit/?h=bug_30480&id=6e60b0bd52a8e85c6f85eb531737258be914fc2d

To check that it is working correctly, you can use the branch test_bug_30480 from my tor-browser-build repo:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=test_bug_30480&id=d406833d7b39c63a2808ea66b99dec9bca460fc8

My tor-browser-build repo includes 4 tags:

  • test_bug_30480-t1 is a correct signed tag
  • test_bug_30480-t2 is a copy of test_bug_30480-t1, so its name is incorrect
  • test_bug_30480-t3 is a correct signed tag. However it also includes the line tag test_bug_30480-t4 in the commit message (which should be ignored).
  • test_bug_30480-t4 is a copy of test_bug_30480-t3, so its name is incorrect (but the correct name is included in the commit message)

Which can be tested with:

./rbm/rbm tar bug_30480 --target t1
./rbm/rbm tar bug_30480 --target t2
./rbm/rbm tar bug_30480 --target t3
./rbm/rbm tar bug_30480 --target t4

With the rbm patch, t2 and t4 are now failing, but do not fail without the patch.

comment:2 Changed 5 weeks ago by boklm

In branch bug_30480_v2 I updated the commit message to mention that the issue was reported by Santiago Torres-Arias and Keving Gallagher from NYU:
https://gitweb.torproject.org/user/boklm/rbm.git/commit/?h=bug_30480_v2&id=4981411ac1981ccc9080da75563a81b5c37c6ece

comment:3 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201905R removed
Status: needs_reviewneeds_revision

I guess

+        return $1 if $l =~ m/^tag (.*)$/;

is assuming that the first such line showing up is the one we want and an attacker can't get to enter fake tag lines (like they can do with a commit message) before that? If so, could we add a comment here?

s/helping fix/helping to fix/

Otherwise this looks good to me.

comment:4 in reply to:  3 Changed 5 weeks ago by boklm

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201905 removed
Status: needs_revisionneeds_review

Replying to gk:

I guess

+        return $1 if $l =~ m/^tag (.*)$/;

is assuming that the first such line showing up is the one we want and an attacker can't get to enter fake tag lines (like they can do with a commit message) before that? If so, could we add a comment here?

The tag message is separated from the headers by an empty line, so we ignore anything after the first empty line. I added a comment saying that in branch bug_30480_v3:
https://gitweb.torproject.org/user/boklm/rbm.git/commit/?h=bug_30480_v3&id=e04f03f9626e993bb66d7784d258f95ca07bc769

comment:5 Changed 4 weeks ago by gk

Keywords: tbb-rbm added
Resolution: fixed
Status: needs_reviewclosed

Looks good now, thanks. Merged to rbm's master (commit e04f03f9626e993bb66d7784d258f95ca07bc769) and bumped version we use in tor-browser-build on master(commit 7526dc507738c0816d84e0746b041867e2b35f90) and maint-8.5 (commit 018ff64dc7fc5542dd8bca6621e2d86bd8ea06fd).

Note: See TracTickets for help on using tickets.