diff mbox

[pacman-dev] libmakepkg/integrity: check for invalid tags

Message ID 20170704031535.18151-1-eschwartz93@gmail.com
State Superseded, archived
Headers show

Commit Message

Eli Schwartz July 4, 2017, 3:15 a.m. UTC
As per https://lists.archlinux.org/pipermail/arch-general/2017-July/043876.html
git doesn't check that the tag name matches what an annotated tag object
*thinks* it should be called. This is a bit of a theoretical attack and
some would argue that we should always use commits since upstream can
legitimately change a tag, but nevertheless this can result in a
downgrade attack if the git download transport was manipulated.

So, check the tag blob to make sure the tag actually matches the name we
used for `git checkout`

Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
---
 scripts/libmakepkg/integrity/verify_signature.sh.in | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Allan McRae July 4, 2017, 3:19 a.m. UTC | #1
On 04/07/17 13:15, Eli Schwartz wrote:
> As per https://lists.archlinux.org/pipermail/arch-general/2017-July/043876.html
> git doesn't check that the tag name matches what an annotated tag object
> *thinks* it should be called. This is a bit of a theoretical attack and
> some would argue that we should always use commits since upstream can
> legitimately change a tag, but nevertheless this can result in a
> downgrade attack if the git download transport was manipulated.
> 
> So, check the tag blob to make sure the tag actually matches the name we
> used for `git checkout`
> 
> Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>

This should be fixed in git.
Eli Schwartz July 4, 2017, 4:47 a.m. UTC | #2
On 07/03/2017 11:19 PM, Allan McRae wrote:
> This should be fixed in git.

Not that I disagree, but it appears this potential attack was discovered
a year ago, and sangy has in the meantime (since I started writing this
patch and before I emailed it and then noticed his link) posted a link
on IRC to the upstream discussion on fixing this in git(1).

http://public-inbox.org/git/20161007210721.20437-1-santiago@nyu.edu/

It would appear that upstream believes the fix is by adding git
porcelain/plumbing around the process of determining $tagname (rather
than relying on git-cat-file(1) piped to awk) and allowing git itself to
consider such tags as valid refs in the git repository.

Or, actually, I am not sure when this was added or what that patchset
accomplished, because it appeared to target `git tag -v` and `git
verify-tag`... by completely deleting everything but the now-valid
--format value, while presumably `git tag -l` which always accepted
--format continued to do what it always did?

What I do know is that the current version of git accepts
--format='%tag' everywhere relevant, and checking for this kind of
modification can and apparently should be done by every single project
or human interface that wants to make sure it isn't a problem. Go figure.

So I guess we could use instead:
tagname="$(git -C "$dir" tag -l --format='%(tag)' "$fragval")"
diff mbox

Patch

diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in
index 5468f977..3783dbb2 100644
--- a/scripts/libmakepkg/integrity/verify_signature.sh.in
+++ b/scripts/libmakepkg/integrity/verify_signature.sh.in
@@ -187,6 +187,13 @@  verify_git_signature() {
 
 	printf "    %s git repo ... " "${dir##*/}" >&2
 
+	tagname="$(git -C "$dir" cat-file tag "$fragval" 2>/dev/null | awk 'FNR == 3 {print $2}')"
+	if [[ $fragtype = tag && -n $tagname && $tagname != $fragval ]]; then
+		printf "%s (%s)" "$(gettext "FAILED")" "$(gettext "forged tag, you have been hacked!")" >&2
+		errors=1
+		return 1
+	fi
+
 	git -C "$dir" verify-$fragtype --raw "$fragval" > "$statusfile" 2>&1
 	if ! grep -qs NEWSIG "$statusfile"; then
 		printf '%s\n' "$(gettext "SIGNATURE NOT FOUND")" >&2