Vm: Check downloaded files with GPG before using them.

[ypid]

Refer to:

• https://github.com/owncloud/vm/issues/45
• https://github.com/jchaney/owncloud/pull/12

[enoch85]

I saw you made a PR for this before, could you please add one here as well?

[enoch85]

Btw, this is not done with packages.

[ypid]

I saw you made a PR for this before, could you please add one here as well?

I think I did not submit a PR for this yet. I will be happy to review your PR :wink:

[enoch85]

@ypid Done: https://github.com/nextcloud/vm/commit/c38e0c3c80886a9c29d60919908e5cefe13ba157

[ypid]

@enoch85 Thanks very much for your work. That means a lot to me because this
will ensure that tempering with the build process will become more difficult.

However, I am afraid that you completely missed the point of checking the GPG
signature in the first place.

I refer you to this comment: https://github.com/jchaney/owncloud/pull/12#issuecomment-99544307
to which I already referred to in my opening comment in case it is unclear what the use case of GPG is here.

As it seems that the public key of NextCloud is currently not available on public key servers, you can do this little trick:

gpg --homedir /tmp/gpg --import nextcloud.asc
gpg --homedir /tmp/gpg --export 28806A878AE423A28372792ED75899B9A724937A | gpg --import -

to enforce the correct key fingerprint of the public key.

[enoch85]

I used the docs, and according to them my fix is valid: https://docs.nextcloud.org/server/9/admin_manual/installation/source_installation.html?highlight=asc#example-installation-on-ubuntu-14-04-lts-server

https://github.com/nextcloud/vm/blob/master/nextcloud_install_production.sh#L249-L264

[ypid]

Your fix is valid but as I said, does only get you very little compared to if you enforce the fingerprint in your build scripts, that will give you (and all users) a whole lot more. In my opinion both the NextCloud documentation and the ownCloud documentation about checking GPG signatures are a bit unmindful and need to be fixed. But note that setting up NextCloud or ownCloud one time as a typical admin vs. writing scripts to build a template VM has a different threat model: https://www.qubes-os.org/news/2016/05/30/build-security/

Refer to On Digital Signatures and Key Verification to learn why your current approach does not fully resolve this issue.

However, for digital signatures to make any sense, we must ensure that the public keys we use for signature verification are indeed the original ones.

Sysadmins and devs must know that.

Let paste in my linked example:

Not including the signing key from (NextCloud|ownCloud) or at least specifying the fingerprint does destroy the whole purpose of checking the signature in the first place. What would happen when in the (Dockerfile|build script) all three files are downloaded over https and someone compromised there website? Right, the adversary would create a new public/private key, sign the archive with it and upload the public key … When we remove the public key from this repo, there is no benefit in security …

In the case of NextCloud, the public key and the archives are hosted on different servers from what it appears but that is nothing that will stop a skilled adversary.

The gain in security enforcing the fingerprint in the build script will get us is that even when *nextcloud.org* would get compromised and your build environment is still trusted, the VM template could not be compromised that easy.

BTW, thanks for referring to your implementation again:

echo "$?"
if [[ $? > 0 ]]
then
        echo "Package NOT OK! Installation is aborted..."
        exit 1
else
        echo "Package OK!"
fi

$? only contains the exit code of _the last command_ (and that is echo which is very unlikely to return anything else than 0). Good that you put an set -e at the top :wink:

Disclaimer: I don’t want to be offensive in any way. I just care about security.

Please reconsider.

[enoch85]

Thank you for your long post. How would the GPG key be useful after the build is done? I don't see any usecase for it. Even you deleted the whole folder in you code.

Regarding the code, that's already fixed, but not merged. Just used it in testing purposes. ;)

Btw, this is open source - instead of writing about it, you can actually make a PR. It's an easy fix. :D In the same amount of time you've spent on writing that post, you could simply just write the piece of code instead. Now it seems like you just want to show off.

[ypid]

In the same amount of time you've spent on writing that post, you could simply just write the piece of code instead.

I did/do that often. The problem was when the maintainers don’t understand what the purpose of it is it might get broken. Seen that happen. So excuse me for that.
Can you enforce the public key as suggested now that you understand the background?

How would the GPG key be useful after the build is done?

I guess NextCloud updates happen via NextCloud’s update mechanism? If not, a new archive might need to be downloaded again. Anyway, I think it makes sense to ship the GPG key with the VM.

[enoch85]

Can you enforce the public key as suggested now that you understand the background?

I guess that's just a flag that need to be set?

I guess NextCloud updates happen via NextCloud’s update mechanism?

They don't use package managers like apt-get, so every new version has a new key.

[ypid]

I guess that's just a flag that need to be set?

I have not found one yet. If you do let me know :wink:

They don't use package managers like apt-get, so every new version has a new key.

You sure? The whole purpose of a public key is that people can use it to verify signed releases.
Also looks like NextCloud git release tags are signed now which is awesome: https://github.com/nextcloud/server/releases

[ypid]

@enoch85 Can you maybe reopen?

[enoch85]

If someone else wants to work on this, sure.

[ypid]

I just had a look at nextcloud_update.sh and it does not check OpenPGP signatures as the installer does.

I then greped over the repo to find other cases of wget which potentially also don’t yet do this:

static/passman.sh:23:wget -q -T 10 -t 2 $PASSVER_REPO/$PASSVER_FILE > /dev/null
static/passman.sh:39:wget -q $PASSVER_REPO/$PASSVER_FILE -P $SHA256
static/passman.sh:40:wget -q $PASSVER_REPO/$PASSVER_FILE.sha256 -P $SHA256
static/passman.sh:59:    wget -q $PASSVER_REPO/$PASSVER_FILE -P $NCPATH/apps
static/test_connection.sh:3:WGET="/usr/bin/wget"
static/test_connection.sh:5:$WGET -q --tries=20 --timeout=10 http://www.google.com -O /tmp/google.idx &> /dev/null
static/spreedme.sh:67:SPREEDME_VER=$(wget -q https://raw.githubusercontent.com/strukturag/nextcloud-spreedme/master/appinfo/info.xml && grep -Po "(?<=<version>)[^<]*(?=</version>)" info.xml && rm info.xml)
static/spreedme.sh:78:    wget -q $SPREEDME_REPO/$SPREEDME_FILE -P $NCPATH/apps
static/spreedme.sh:84:    wget -q $SPREEDME_REPO/$SPREEDME_FILE -P $NCPATH/apps
static/collabora.sh:68:    echo "sudo wget https://raw.githubusercontent.com/nextcloud/vm/master/static/collabora.sh"
static/collabora.sh:79:if wget -q -T 10 -t 2 --spider $SUBDOMAIN; then
static/collabora.sh:81:elif wget -q -T 10 -t 2 --spider --no-check-certificate https://$SUBDOMAIN; then
static/collabora.sh:311:    wget -q $COLLVER_REPO/$COLLVER/$COLLVER_FILE -P $NCPATH/apps
static/nextant.sh:53:wget -q $SOLR_DL --show-progress
static/nextant.sh:59:    wget -q https://raw.githubusercontent.com/apache/lucene-solr/master/solr/bin/install_solr_service.sh -P $SCRIPTS/
static/nextant.sh:104:wget -q -P $NC_APPS_PATH $NT_DL
static/nextant.sh:112:    wget -q $STATIC/setup_secure_permissions_nextcloud.sh -P $SCRIPTS
static/update.sh:18:    wget -q https://raw.githubusercontent.com/nextcloud/vm/master/$FILE -P $SCRIPTS
static/update.sh:21:    wget -q https://raw.githubusercontent.com/nextcloud/vm/master/$FILE -P $SCRIPTS
static/ntpdate.sh:2:wget -q -T 10 -t 2 http://google.com > /dev/null
lets-encrypt/test-new-config.sh:44:    wget -q $STATIC/update-config.php -P $SCRIPTS
lets-encrypt/test-new-config.sh:46:    wget -q $STATIC/update-config.php -P $SCRIPTS
lets-encrypt/test-new-config.sh:53:    wget -q $STATIC/trusted.sh -P $SCRIPTS
lets-encrypt/test-new-config.sh:58:    wget -q $STATIC/trusted.sh -P $SCRIPTS
lets-encrypt/activate-ssl.sh:178:    wget -q https://raw.githubusercontent.com/nextcloud/vm/master/lets-encrypt/test-new-config.sh -P $SCRIPTS
lets-encrypt/activate-ssl.sh:181:    wget -q https://raw.githubusercontent.com/nextcloud/vm/master/lets-encrypt/test-new-config.sh -P $SCRIPTS
lets-encrypt/activate-ssl.sh:188:if wget -q -T 10 -t 2 --spider $domain; then
lets-encrypt/activate-ssl.sh:190:elif wget -q -T 10 -t 2 --spider --no-check-certificate https://$domain; then
nextcloud_install_production.sh:72:wget -q $STATIC/adduser.sh
nextcloud_install_production.sh:109:if wget -q -T 10 -t 2 "$NCREPO" > /dev/null
nextcloud_install_production.sh:321:wget -q $NCREPO/$STABLEVERSION.zip -P $HTML
nextcloud_install_production.sh:323:wget -q $NCREPO/$STABLEVERSION.zip.asc -P $GPGDIR
nextcloud_install_production.sh:343:wget -q $STATIC/setup_secure_permissions_nextcloud.sh -P $SCRIPTS
nextcloud_install_production.sh:521:    wget -q $CALVER_REPO/v$CALVER/$CALVER_FILE -P $NCPATH/apps
nextcloud_install_production.sh:541:    wget -q $CONVER_REPO/v$CONVER/$CONVER_FILE -P $NCPATH/apps
nextcloud_install_production.sh:560:wget -q http://www.webmin.com/jcameron-key.asc -O- | sudo apt-key add -
nextcloud_install_production.sh:590:    wget -q $STATIC/change-root-profile.sh -P $SCRIPTS
nextcloud_install_production.sh:598:    wget -q $STATIC/change-ncadmin-profile.sh -P $SCRIPTS
nextcloud_install_production.sh:606:    wget -q $STATIC/instruction.sh -P $SCRIPTS
nextcloud_install_production.sh:614:    wget -q $GITHUB_REPO/nextcloud-startup-script.sh -P $SCRIPTS
nextcloud_install_production.sh:622:    wget -q $STATIC/history.sh -P $SCRIPTS
nextcloud_install_production.sh:654:    wget -q $STATIC/redis-server-ubuntu16.sh -P $SCRIPTS
nextcloud_update.sh:42:    wget -q $STATIC/setup_secure_permissions_nextcloud.sh -P $SCRIPTS
nextcloud_update.sh:48:wget -q -T 10 -t 2 $NCREPO/nextcloud-$NCVERSION.tar.bz2 > /dev/null
nextcloud_update.sh:112:wget -q -T 10 -t 2 $NCREPO/nextcloud-$NCVERSION.tar.bz2 -P $HTML
nextcloud_update.sh:161:    wget $STATIC/spreedme.sh -P $SCRIPTS
nextcloud_update.sh:170:wget -q $STATIC/recover_apps.py -P $SCRIPTS
nextcloud-startup-script.sh:45:    wget -q -T 10 -t 2 http://github.com > /dev/null
nextcloud-startup-script.sh:60:    wget -q -T 10 -t 2 http://github.com > /dev/null
nextcloud-startup-script.sh:114:    wget -q $STATIC/passman.sh -P $SCRIPTS
nextcloud-startup-script.sh:116:    wget -q $STATIC/passman.sh -P $SCRIPTS
nextcloud-startup-script.sh:131:    wget -q $STATIC/nextant.sh -P $SCRIPTS
nextcloud-startup-script.sh:133:    wget -q $STATIC/nextant.sh -P $SCRIPTS
nextcloud-startup-script.sh:148:    wget -q $STATIC/collabora.sh -P $SCRIPTS
nextcloud-startup-script.sh:150:    wget -q $STATIC/collabora.sh -P $SCRIPTS
nextcloud-startup-script.sh:165:    wget -q $STATIC/spreedme.sh -P $SCRIPTS
nextcloud-startup-script.sh:167:    wget -q $STATIC/spreedme.sh -P $SCRIPTS
nextcloud-startup-script.sh:182:    wget -q $STATIC/temporary-fix.sh -P $SCRIPTS
nextcloud-startup-script.sh:184:    wget -q $STATIC/temporary-fix.sh -P $SCRIPTS
nextcloud-startup-script.sh:199:    wget -q $STATIC/security.sh -P $SCRIPTS
nextcloud-startup-script.sh:201:    wget -q $STATIC/security.sh -P $SCRIPTS
nextcloud-startup-script.sh:216:    wget -q $STATIC/update.sh -P $SCRIPTS
nextcloud-startup-script.sh:218:    wget -q $STATIC/update.sh -P $SCRIPTS
nextcloud-startup-script.sh:233:    wget -q $STATIC/phpmyadmin_install_ubuntu16.sh -P $SCRIPTS
nextcloud-startup-script.sh:235:    wget -q $STATIC/phpmyadmin_install_ubuntu16.sh -P $SCRIPTS
nextcloud-startup-script.sh:250:    wget -q $STATIC/update-config.php -P $SCRIPTS
nextcloud-startup-script.sh:252:    wget -q $STATIC/update-config.php -P $SCRIPTS
nextcloud-startup-script.sh:267:    wget -q $LETS_ENC/activate-ssl.sh -P $SCRIPTS
nextcloud-startup-script.sh:269:    wget -q $LETS_ENC/activate-ssl.sh -P $SCRIPTS
nextcloud-startup-script.sh:284:    wget -q $STATIC/trusted.sh -P $SCRIPTS
nextcloud-startup-script.sh:286:    wget -q $STATIC/trusted.sh -P $SCRIPTS
nextcloud-startup-script.sh:301:    wget -q $STATIC/ip.sh -P $SCRIPTS
nextcloud-startup-script.sh:303:    wget -q $STATIC/ip.sh -P $SCRIPTS
nextcloud-startup-script.sh:318:    wget -q $STATIC/test_connection.sh -P $SCRIPTS
nextcloud-startup-script.sh:320:    wget -q $STATIC/test_connection.sh -P $SCRIPTS
nextcloud-startup-script.sh:335:    wget -q $STATIC/setup_secure_permissions_nextcloud.sh -P $SCRIPTS
nextcloud-startup-script.sh:337:    wget -q $STATIC/setup_secure_permissions_nextcloud.sh -P $SCRIPTS
nextcloud-startup-script.sh:352:    wget -q $STATIC/change_mysql_pass.sh
nextcloud-startup-script.sh:354:    wget -q $STATIC/change_mysql_pass.sh -P $SCRIPTS
nextcloud-startup-script.sh:369:    wget -q $STATIC/nextcloud.sh -P $SCRIPTS
nextcloud-startup-script.sh:371:    wget -q $STATIC/nextcloud.sh -P $SCRIPTS
nextcloud-startup-script.sh:385:    wget -q $GITHUB_REPO/index.php -P $SCRIPTS
nextcloud-startup-script.sh:387:    wget -q $GITHUB_REPO/index.php -P $SCRIPTS
nextcloud-startup-script.sh:464:        wget -q $STATIC/ip2.sh -P $SCRIPTS
nextcloud-startup-script.sh:863:    wget -q $STATIC/update-config.php -P $SCRIPTS
nextcloud-startup-script.sh:865:    wget -q $STATIC/update-config.php -P $SCRIPTS
nextcloud-startup-script.sh:872:    wget -q $STATIC/trusted.sh -P $SCRIPTS
nextcloud-startup-script.sh:877:    wget -q $STATIC/trusted.sh -P $SCRIPTS
README.md:31:`wget https://raw.githubusercontent.com/nextcloud/vm/master/nextcloud_install_production.sh`

Can you reopen the issue?

To solve this, I would propose to drop the single wget’s and do a full git clone of this repo, checking the signature and then using it.

[enoch85]

To solve this, I would propose to drop the single wget’s and do a full git clone of this repo, checking the signature and then using it.

@ypid Wow, you mean rewrite everything, again? ;) We are just about to finish the refactoring of the current code base. Feel free to open a PR and fix this when that branch is merged to master.

[ypid]

Wow, you mean rewrite everything, again? ;)

Not everything. Basically just the download part. :wink:

Feel free to open a PR and fix this when that branch is merged to master.

This is too much work for me as I don’t actually use the scripts and I already fixed one instance in https://github.com/nextcloud/vm/pull/19. Would be great if people who use it to fix this.

[enoch85]

I'll look into this for the updater, but the rest of the code is long term.

[morph027]

Either git clone or wget'ing the master.zip might be easy task. We could get rid of a huge potion of code. But sure, let's finish the cleanup, then improve this.

[enoch85]

@ypid Fixed in the update script: https://github.com/morph027/vm/pull/30/commits/d7e9c1bea7c3d69ecaf3c43b27d7d1108fc5acd9

[enoch85]

@ypid Hey, any idea how to solve this?

[ypid]

One of those options should do:

Either git clone or wget'ing the master.zip might be easy task. We could get rid of a huge potion of code. But sure, let's finish the cleanup, then improve this.

To solve this, I would propose to drop the single wget’s and do a full git clone of this repo, checking the signature and then using it.

@morph027 and me both mentioned git cloning. I don’t really know the code how it could be implemented in practice but the basic steps could be:

• git clone
• gpg --recv-keys (with full fingerprint)
• git verify-tag || exit 1
• run the scripts

[enoch85]

That would require pretty much refactoring as ALL functions and files are fetched fresh from the repo with wget or curl, this is so that we don't have to publish a brand new VM everytime we fix smaller things, or add new scripts. Your solution would mean that once released, all scripts are static in the VM. It's a brand new approach.

I mean, how "dangerous" is wget and curl really? What could happen? Master is protected and only admins can write to it if I approve.

[ypid]

I mean, how "dangerous" is wget and curl really? What could happen?

Depends who you ask :wink:

https://www.qubes-os.org/doc/user-faq/#what-does-it-mean-to-distrust-the-infrastructure

Ideally, the VM would be shipped with a set of build-in trusted OpenPGP keys which are then used to verify all scripts and updates to those. You could extend your current approach and download and check signatures to all scripts along with the scripts themself with wget but that would be pretty difficult to audit and so on.

You can still do updates using the schema proposed. Something like git fetch origin && git verfiy-commit origin/master && git reset --hard origin/master could do (untested).

[enoch85]

@ypid Hehe Qubes, cool stuff.

Yeah, we'll see, nothing that I feel eager to do atm I'm afraid. Made a new label just for this issue. Feel free to dig in :wink:

[enoch85]

@small1 As we discussed on the conf today. This is the issue I was talking about.

[enoch85]

@morph027 Hey! Long time ago. Hope all is well!

This issue is nagging my eye every time I visit the issue section. Do you think you could help me do something about it?

[morph027]

Hi!

Sure. After re-reading all comments and thinking about it, i guess instead og downloading all files from the repo itself, we should probably use the releases section of github and distribute a signed archive of all files plus the public signing key itself to check against.

[enoch85]

@morph027

we should probably use the releases section of github and distribute a signed archive of all files plus the public signing key itself to check against.

Sound like a plan! So basically:

• Remove all the wget stuff.
• Fetch the released version scripts and check signature
• Unpack the scripts to /var/scripts
• Fetch the lib.sh within the VM itself (aka no more looking on Github to get the latest stuff)

Anything I missed, or do you have a better idea?

[ypid]

Thanks guys for putting an effort into this. I appreciate it.

Unpack the scripts to /var/scripts

According to the Filesystem Hierarchy Standard I would suggest to store the scripts under /usr/local/lib/nextcloud-vm/ or /usr/local/bin depending on if they should be in the $PATH. /var/ is not the right place.

[enoch85]

@ypid

/var/scripts has been used since the beginning. I'm thinking backwards compatibility here. The update script is built so that it can be run on all Nextcloud versions with different outputs depending on which version you run. Changing something that ancient is asking for trouble. Sure, /usr/local/scripts maybe would make more sense but I chose var/scripts back in the days (when I was new to linux) cause I thought that was the best place.

@morph027

we should probably use the releases section of github and distribute a signed archive of all files plus the public signing key itself to check against.

https://wiki.debian.org/Creating%20signed%20GitHub%20releases

[ypid]

Thanks for your input. /usr/local/scripts would also be somewhat conflicting. /usr/local/lib/nextcloud-vm/ could be used. Maybe symlinks could be used which link /var/scripts to /usr/local/lib/nextcloud-vm/ but that is your call. Just an idea.

[enoch85]

@ypid

but that is your call. Just an idea.

No, I will not change the location of such an important place. Not even with symlinks, sorry. :)

What I actually did a few days ago though was to introduce the lib.sh file as a file to be downloaded in the installation script so that it can be used locally when the first setup is run. It's also used in two places starting from VM 17.0.0: static_ip.sh and nextcloud--śtartup-script.sh

I've been thinking about this issue for some time now, and it's growing on me. :)

I'm just saying that right now, I'm leaning towards making each release "release-specific". I'm not quite there yet though - in other words, I'm not ready to abandon the old way since it's super convenient when it comes to fixing bugs in old VMs as well, and many other things.

As always, feel free to dig in!

[szaimen]

What I actually did a few days ago though was to introduce the lib.sh file as a file to be downloaded in the installation script so that it can be used locally when the first setup is run. It's also used in two places starting from VM 17.0.0: static_ip.sh and nextcloud--śtartup-script.sh

That sounds like a really good plan. Why not doing it this way in all scripts? (use the local script if available)

The best way imo would be letting the user the choice: so you could do a signed release and the user just could download this signed release with all neded scripts inside. If the user doesn't want the highest security but stability (bugfixes) and newest features, he could choose during the startup to remove all local files and use the repo-files instead. The other way around: If you have chosen security, only the local files get used. But you could just remove a file afterwards and you would get automatically the latest script from the repo. (or just substitute a local script one time manually)

So a bit shorter:

1. Sign the releases
2. All scripts are inside this and can be used locally (which is the best for security)
3. if you choose for stability (bugfixes) and latest features during the start of the startup script, all fitting local scripts get removed and you fetch always the newest ones from the repo.

How sound that?

[enoch85]

The best way imo would be letting the user the choice: so you could do a signed release and the user just could download this signed release with all neded scripts inside.

More work, less time, more confusing. IMO, either we do a signed release or not.

As you say, making a signed release would mean pre-downloading all the scripts from each release, and be version specific. Also more work, hence this ticket is old. :)

[szaimen]

Actually, I would like to help you coding that option (using all scripts locally) but not if it is a signed release only and without the ability to always get the latest scripts from the repo.

[enoch85]

@szaimen Please make a POC, I don't make any promises though.

It would be really great to be able to finally close this ticket once and for all!

[szaimen]

Then a short question from my side: is it enough to download all scripts during the install_production script (and somehow sign the finished files) or do we have to actually sign the released code, extract the code manually once to the right directory and use the local files then? (Also already for the install_production script?)

[enoch85]

@szaimen Please read the issue and all the links and get your own picture. Sorry, but I don't have time to think right now. Very stressed out.

When done, please do what you think would be best - make the PR in question and we can discuss there.

Thanks!

[szaimen]

Okay, I will think through it and make a POC. But as always: it will take its time.

[szaimen]

If somebody wants to test the latest implementation of this feature, please check out https://github.com/nextcloud/vm/pull/1263#issuecomment-635357376

[enoch85]

I actually don't think this will ever happen.

We use GPG inside the scripts were we can, and the user can verify against SHA256 mdsum before downloading the VM. Taking it a step further to actually sign each Github release is not on my roadmap. Sorry.

This has been stale for several years now, I've kept it open if someone wanted to implement it, @szaimen did a nice try but it never led to something.

@ypid If you really want this, please contribute by making a PR. I'll keep this open for few more days, then I'm closing if nothing happens.

Thanks for your efforts!

[ypid]

Thanks for the attempts so far. I cannot go down that rabbit whole however of doing it myself unfortunately. See my comment from https://github.com/nextcloud/vm/pull/1263#issuecomment-638455859:

[...] Note that I am not using this project I just happened to get involved somehow. So what I can offer is review a proposal/specification on how to do secure script distribution. Something summarized. I guess I have mentioned it already. Just use the feature that git provides together with OpenPGP so there should not be much programming needed nor wanted (in this area).

[enoch85]

Seems like it's possible but requires an extra step: https://wiki.debian.org/Creating%20signed%20GitHub%20releases