diff mbox

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

Message ID 20170728035914.15517-1-eschwartz@archlinux.org
State Superseded, archived
Headers show

Commit Message

Eli Schwartz July 28, 2017, 3:59 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 or the
upstream repository hacked.

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

This really should be fixed in git itself, rather than forcing all
downstream users of git verify-tag to implement their own checks, but
the git developers disagree, see the discussion surrounding
https://public-inbox.org/git/xmqqk2hzldx8.fsf@gitster.mtv.corp.google.com/

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
---

v3: Reference a discussion with the git developers. @sangy says that
thread is where the initial decision to follow their current approach
happened.

Use a better FAILED message.

 scripts/libmakepkg/integrity/verify_signature.sh.in | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Allan McRae Sept. 13, 2017, 1:48 a.m. UTC | #1
On 28/07/17 13:59, 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 or the
> upstream repository hacked.
> 
> So, check the tag blob to make sure the tag actually matches the name we
> used for `git checkout`.
> 
> This really should be fixed in git itself, rather than forcing all
> downstream users of git verify-tag to implement their own checks, but
> the git developers disagree, see the discussion surrounding
> https://public-inbox.org/git/xmqqk2hzldx8.fsf@gitster.mtv.corp.google.com/
> 
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> 
> v3: Reference a discussion with the git developers. @sangy says that
> thread is where the initial decision to follow their current approach
> happened.
> 
> Use a better FAILED message.
> 
>  scripts/libmakepkg/integrity/verify_signature.sh.in | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in
> index 24519dbe..4f9f97ca 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" tag -l --format='%(tag)' "$fragval")"
> +	if [[ $fragtype = tag && -n $tagname && $tagname != $fragval ]]; then
> +		printf "%s (%s)" "$(gettext "FAILED")" "$(gettext "the git tag has been forged")" >&2
> +		errors=1
> +		return 1
> +	fi
> +

Why is this in verify_signature?  As far as I can tell, it has nothing
to do with signatures and should just be down in the extract_git()
function when we are dealing with a tag.

>  	git -C "$dir" verify-$fragtype --raw "$fragval" > "$statusfile" 2>&1
>  	if ! grep -qs NEWSIG "$statusfile"; then
>  		printf '%s\n' "$(gettext "SIGNATURE NOT FOUND")" >&2
>
diff mbox

Patch

diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in
index 24519dbe..4f9f97ca 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" tag -l --format='%(tag)' "$fragval")"
+	if [[ $fragtype = tag && -n $tagname && $tagname != $fragval ]]; then
+		printf "%s (%s)" "$(gettext "FAILED")" "$(gettext "the git tag has been forged")" >&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