[PATCH] libmakepkg/integrity: check for invalid tags

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] libmakepkg/integrity: check for invalid tags

Eli Schwartz
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 <[hidden email]>
---
 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..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
--
2.13.2
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] libmakepkg/integrity: check for invalid tags

Allan McRae
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 <[hidden email]>

This should be fixed in git.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] libmakepkg/integrity: check for invalid tags

Eli Schwartz
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@.../

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")"

--
Eli Schwartz


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] libmakepkg/integrity: check for invalid tags

Eli Schwartz
In reply to this post by Eli Schwartz
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 <[hidden email]>
---

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
+ 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
--
2.13.2
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] libmakepkg/integrity: check for invalid tags

Allan McRae
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 <[hidden email]>
> ---
>
> 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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v3] libmakepkg/integrity: check for invalid tags

Eli Schwartz-2
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@.../

Signed-off-by: Eli Schwartz <[hidden email]>
---

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
+
  git -C "$dir" verify-$fragtype --raw "$fragval" > "$statusfile" 2>&1
  if ! grep -qs NEWSIG "$statusfile"; then
  printf '%s\n' "$(gettext "SIGNATURE NOT FOUND")" >&2
--
2.13.3
Loading...