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

Check downloaded files with GPG before using them. #11

Closed
ypid opened this issue Jun 22, 2016 · 43 comments
Closed

Check downloaded files with GPG before using them. #11

ypid opened this issue Jun 22, 2016 · 43 comments

Comments

@ypid
Copy link
Contributor

ypid commented Jun 22, 2016

@enoch85
Copy link
Member

enoch85 commented Jun 22, 2016

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

@enoch85
Copy link
Member

enoch85 commented Jun 22, 2016

Btw, this is not done with packages.

@ypid
Copy link
Contributor Author

ypid commented Jun 22, 2016

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 😉

@enoch85
Copy link
Member

enoch85 commented Jun 22, 2016

@ypid Done: c38e0c3

@enoch85 enoch85 closed this as completed Jun 22, 2016
@ypid
Copy link
Contributor Author

ypid commented Jun 22, 2016

@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: jchaney/owncloud#12 (comment)
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.

@ypid
Copy link
Contributor Author

ypid commented Jun 23, 2016

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 😉

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

Please reconsider.

@enoch85
Copy link
Member

enoch85 commented Jun 23, 2016

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
Copy link
Contributor Author

ypid commented Jun 23, 2016

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
Copy link
Member

enoch85 commented Jun 24, 2016

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
Copy link
Contributor Author

ypid commented Jun 24, 2016

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

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

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
Copy link
Contributor Author

ypid commented Jul 5, 2016

@enoch85 Can you maybe reopen?

@enoch85
Copy link
Member

enoch85 commented Jul 11, 2016

If someone else wants to work on this, sure.

@ypid
Copy link
Contributor Author

ypid commented Apr 7, 2017

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
Copy link
Member

enoch85 commented Apr 7, 2017

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.

@enoch85 enoch85 reopened this Apr 7, 2017
@ypid
Copy link
Contributor Author

ypid commented Apr 7, 2017

Wow, you mean rewrite everything, again? ;)

Not everything. Basically just the download part. 😉

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 #19. Would be great if people who use it to fix this.

@enoch85
Copy link
Member

enoch85 commented Apr 7, 2017

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

@szaimen
Copy link
Collaborator

szaimen commented Jan 26, 2020

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
Copy link
Member

enoch85 commented Jan 29, 2020

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
Copy link
Collaborator

szaimen commented Jan 29, 2020

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
Copy link
Member

enoch85 commented Jan 29, 2020

@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
Copy link
Collaborator

szaimen commented Jan 30, 2020

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
Copy link
Member

enoch85 commented Jan 30, 2020

@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
Copy link
Collaborator

szaimen commented Jan 30, 2020

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

szaimen referenced this issue May 22, 2020
we should add a script that fetches the latest `server_configuration` script @szaimen 

That way we don't need to keep it in the scripts folder, and it would always be the latest version of the script
@szaimen
Copy link
Collaborator

szaimen commented May 28, 2020

If somebody wants to test the latest implementation of this feature, please check out #1263 (comment)

@enoch85
Copy link
Member

enoch85 commented Jul 4, 2020

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
Copy link
Contributor Author

ypid commented Jul 5, 2020

Thanks for the attempts so far. I cannot go down that rabbit whole however of doing it myself unfortunately. See my comment from #1263 (comment):

[...] 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
Copy link
Member

enoch85 commented Jul 5, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants