Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build-for-aur.sh security issue #1175

Closed
dandelionred opened this Issue Apr 8, 2019 · 18 comments

Comments

Projects
None yet
2 participants
@dandelionred
Copy link
Contributor

commented Apr 8, 2019

First of all: I've never heard about tuxfamily.org so please don't blame me in case I hurt your feelings about it. All I know about the site atm is this https://siterankdata.com/tuxfamily.org (look at the wide graph in the page's bottom) which says after six consecutive years spent in alexa top 100k it was suddenly wiped out of alexa top 1m and never reentered there since Sep 16 2017.

Back to the issue.

ARCHIVE_SHA256=`wget -qO- https://download.tuxfamily.org/qownnotes/src/qownnotes-${QOWNNOTES_VERSION}.tar.xz.sha256`

So in case tuxfamily.org is hacked (or does it on intention) it is possible for the AUR pkgbuild to deliver modified sources with valid sha256 sum.

@pbek pbek added the question label Apr 8, 2019

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

this script is called immediately after the package is uploaded to tuxfamily, so someone would have just a few seconds to "hack" it...

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

I've never heard about tuxfamily.org

I just needed a free file hosting for FOSS projects, Sourceforge was down too often...

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

In case the site is hacked malicious patching can be done automaticly on package upload hook.
Even more. wget manual (in ubuntu) says

The only timeout enabled by default is a 900-second read timeout.

Since the script doesn't set any timeout on the wget call it is possible for the call to last for 900s at least which is pretty much enough for tampering even without automation.

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

Since the script doesn't set any timeout on the wget call it is possible for the call to last for 900s at least which is pretty much enough for tampering even without automation.

I sit in front of my computer and wait until the deployment scripts are finished

what do you want to achieve with this issue you posted?

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

this script is called immediately after the package is uploaded to tuxfamily, so someone would have just a few seconds to "hack" it...

Please, consider reusing local checksums generated in build-tuxfamily-src.sh without relying on some remote party to store it for those few seconds.

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

I will consider, thank you for your feedback

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

I sit in front of my computer and wait until the deployment scripts are finished
what do you want to achieve with this issue you posted?

I'm reading into the sources and the build scripts. I like the thing you've made very much and would like more people to use it. Fixing things which look like security issues is important in not averting people from using it.

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

My main script is https://github.com/pbek/QOwnNotes/blob/develop/build-systems/build-all.sh, from there I upload the source bundles and then run all deployment scripts in parallel via tmux...

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Slackware and gentoo builds believe in tuxfamily as well and generate checksums out of downloaded files.

ARCHIVE_FILE=qownnotes-${QOWNNOTES_VERSION}.tar.xz
wget https://download.tuxfamily.org/qownnotes/src/${ARCHIVE_FILE}
ARCHIVE_SHA512=`sha512sum ${ARCHIVE_FILE} | awk '{ print $1 }'`
ARCHIVE_MD5=`md5sum ${ARCHIVE_FILE} | awk '{ print $1 }'`

ARCHIVE_FILE=qownnotes-${QOWNNOTES_VERSION}.tar.xz
wget https://download.tuxfamily.org/qownnotes/src/${ARCHIVE_FILE}
ARCHIVE_SHA512=`sha512sum ${ARCHIVE_FILE} | awk '{ print $1 }'`


snapcraft.yaml doesn't utilize source-checksum feature and believes in tuxfamily

parts:
qownnotes:
source: https://download.tuxfamily.org/qownnotes/src/qownnotes-VERSION-STRING.tar.xz
# see https://docs.snapcraft.io/reference/plugins/qmake
plugin: qmake
qt-version: qt5

(also notice the URL to the qmake plugin redirects to https://docs.snapcraft.io/writing-local-plugins/5125. Should it be https://docs.snapcraft.io/the-qmake-plugin/8628 ?)

Here is a sample how source-checksum is used
https://github.com/snapcrafters/gimp/blob/a388e709da9352444e8c69093b4eafa1a05d5c79/snap/snapcraft.yaml#L171-L172

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

If you want you can alter the tuxfamily script to export an environment variable with the checksum and modify the other scripts to optionally (if available) use that variable to set the checksum instead of downloading the remote checksum. I'm open for a pull requests. 😸

pbek added a commit that referenced this issue Apr 10, 2019

pbek added a commit that referenced this issue Apr 10, 2019

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Great to see you're already fixing the problem!

But why still attempting to use remote checksums here?

# check variable from build-systems/tuxfamily/build-tuxfamily-src.sh
if [ -z ${QOWNNOTES_ARCHIVE_SHA256} ]; then
echo "QOWNNOTES_ARCHIVE_SHA256 was not set! downloading checksum..."
ARCHIVE_SHA256=`wget -qO- https://download.tuxfamily.org/qownnotes/src/qownnotes-${QOWNNOTES_VERSION}.tar.xz.sha256`
else
ARCHIVE_SHA256=${QOWNNOTES_ARCHIVE_SHA256}
fi

It should be an immediate exit in case $QOWNNOTES_ARCHIVE_SHA256 is not set.

For example when we install packages with apt from some (probably compromised) mirror there is no such problem as tampered packages with valid checksums because there is another layer of security: the gpg-signed Release file like the one under http://ftp.ubuntu.com/ubuntu/dists/trusty/. More details about the matter: https://wiki.debian.org/SecureApt

Since you don't sign anything when uploading to the remote, the stuff downloaded back from there is absolutely not trusted, you can't check if it was tampered or not. Basicly it is the reason why we have hashes explicitly inside PKGBUILD/snaps/flatpaks: a package can be tampered by the remote side but it can't fake the hash because the hash comes from another source (e.g. PKGBUILD).

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 12, 2019

Great to see you're already fixing the problem!

Problem is that exporting environment variables doesn't work across scripts, so this attempt doesn't work. 😞

But why still attempting to use remote checksums here?

because the scripts need to work stand alone too if need arises
I need an other solution for the checksums... any suggestions?

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Problem is that exporting environment variables doesn't work across scripts, so this attempt doesn't work.

Mby build-tuxfamily-src.sh should produce a temporary file vars with all the hashes used in the various build scripts

QOWNNOTES_ARCHIVE_MD5=...
QOWNNOTES_ARCHIVE_SHA256=....
QOWNNOTES_ARCHIVE_SHA512=....

and the build scripts should source the file on start

#!/bin/bash

if [[ ! -f vars ]]; then
	echo "vars" doesn't exist. build-tuxfamily-src.sh must be run ahead of any other build scripts!
	exit 1
fi

source vars

# the rest of the code, all hashes should be taken from the vars only now

pbek added a commit that referenced this issue Apr 19, 2019

pbek added a commit that referenced this issue Apr 19, 2019

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 19, 2019

now it should work for the AUR

pbek added a commit that referenced this issue Apr 19, 2019

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

now it should work for the AUR

Great! Will you also fix the same for slackware, gentoo and snapcraft as per #1175 (comment) ?

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

now it should work for the AUR

Also this

# check variable from build-systems/tuxfamily/build-tuxfamily-src.sh
if [ -z ${QOWNNOTES_ARCHIVE_SHA256} ]; then
echo "QOWNNOTES_ARCHIVE_SHA256 was not set! downloading checksum..."
ARCHIVE_SHA256=`wget -qO- https://download.tuxfamily.org/qownnotes/src/qownnotes-${QOWNNOTES_VERSION}.tar.xz.sha256`
else
ARCHIVE_SHA256=${QOWNNOTES_ARCHIVE_SHA256}
fi

should be patched as

--- <unnamed>
+++ <unnamed>
@@ -1,7 +1 @@
-# check variable from build-systems/tuxfamily/build-tuxfamily-src.sh
-if [ -z ${QOWNNOTES_ARCHIVE_SHA256} ]; then
-    echo "QOWNNOTES_ARCHIVE_SHA256 was not set! downloading checksum..."
-    ARCHIVE_SHA256=`wget -qO- https://download.tuxfamily.org/qownnotes/src/qownnotes-${QOWNNOTES_VERSION}.tar.xz.sha256`
-else
-    ARCHIVE_SHA256=${QOWNNOTES_ARCHIVE_SHA256}
-fi
+ARCHIVE_SHA256=${QOWNNOTES_ARCHIVE_SHA256}

pbek added a commit that referenced this issue Apr 23, 2019

pbek added a commit that referenced this issue Apr 23, 2019

pbek added a commit that referenced this issue Apr 23, 2019

pbek added a commit that referenced this issue Apr 23, 2019

pbek added a commit that referenced this issue Apr 23, 2019

pbek added a commit that referenced this issue Apr 23, 2019

@pbek

This comment has been minimized.

Copy link
Owner

commented Apr 23, 2019

Now everything should be in place. Did you see anything else?

@dandelionred

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Nope, that was all I noticed related to the original AUR flaw.

@pbek pbek closed this Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.