diff mbox

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

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

Commit Message

Eli Schwartz July 5, 2017, 5:48 p.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`

Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
---

v2: use git's built-in format specifier to obtain the real tagname with
a single command. I didn't realize in v1 that this was possible.

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

Comments

Allan McRae July 28, 2017, 2:42 a.m. UTC | #1
On 06/07/17 03:48, 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`
> 

For reference, I'd like to see some links in the commit message to where
this is discussed with git developers and with them saying that this is
not an issue to be fixed in git.

> Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
> ---
> 
> v2: use git's built-in format specifier to obtain the real tagname with
> a single command. I didn't realize in v1 that this was possible.
> 
>  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 5468f977..93d88006 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 "forged tag, you have been hacked!")" >&2

Just:
"the git tag has been forged"

It is not necessarily the person running makepkg that has been hacked.

> +		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
>
diff mbox

Patch

diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in
index 5468f977..93d88006 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 "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