Skip to content
This repository has been archived by the owner on Feb 4, 2020. It is now read-only.

Verifying checksum of update-resolv-conf.sh #63

Closed
Duncaen opened this issue Mar 31, 2018 · 31 comments
Closed

Verifying checksum of update-resolv-conf.sh #63

Duncaen opened this issue Mar 31, 2018 · 31 comments

Comments

@Duncaen
Copy link

Duncaen commented Mar 31, 2018

Please consider to add a checksum for the update-resolv-conf.sh script that is downloaded.
Without verifying the checksum there is no way to verify if protovpn-cli is safe, because there is no definite state. The downloaded script can change at any time and start to do something malicious.

@njfox
Copy link
Contributor

njfox commented Mar 31, 2018

Is update-resolv-conf.sh writable by anyone but root? I don't have a system handy without update-resolv-conf already installed but if an attacker has root you're already hosed anyway. If the file is writable by other users then that is a problem that would need fixed.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

The file is downloaded from https://raw.githubusercontent.com/ProtonVPN/scripts/master/update-resolv-conf.sh, you have to trust github and/or protonvpn that they don't get compromised and distribute a malicious file.

There is no reason to download a script that is later executed as root without verifying if the script is still the same.

This makes security audits of protonvpn-cli useless, because the downloaded script can just change.
If there would be a checksum check you know it will be the same script every time until protonvpn-cli changes the checksum in which case the new file can be reviewed again.

@njfox
Copy link
Contributor

njfox commented Mar 31, 2018

If an attacker has access to https://raw.githubusercontent.com/ProtonVPN/scripts/master/update-resolv-conf.sh then they also have access to protonvpn-cli and they can modify the checksum. The only way I can see to get around that is to add GPG signing, which wouldn't be a bad thing IMO but it would complicate the setup process. Or am I misunderstanding something?

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

... Right, but I would download protonvpn-cli once, review it, save the checksum and then distribute it between my systems.
Wether I just use scp to copy it from one host to another or package it using a package manager.
The file doesn't change and I have to trust that my audit found every issue.

But downloading a file without checking the checksum breaks this assumption.

Right I could just ship update-resolv-conf.sh myself, but the code that downloads it exists and it can be safer by just adding a hash check, I don't think there is any reason to not do this.

@Rafficer
Copy link
Contributor

It would be easier to download update-resolv-conf.sh once during initialization and during updates and then just copy it within the system.

@Rafficer
Copy link
Contributor

Rafficer commented Mar 31, 2018

Yeah, that's actually what happens. It only downloads that file if the requirement isn't fulfilled and you explicitly tell the program to download and execute that file.

So as long as you don't say yes to the prompt, you are good with checking once.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

Its not only about only about me, its for every user how can I tell someone to use this script if I don't know what its going to execute based on what the user chooses?

Not verifying the checksum breaks reproducibility and any review of it is useless.

Just because a user can decide if it gets installed or not doesn't mean its good practice.

Other than this, the prompt says Would you like protonvpn-cli to install openvpn-resolv-conf? (y/n): which does not mention that it downloads a file that is later executed as root without verifying the checksum.

I don't think we have to continue discussing this, I think this should be fixed.

@Rafficer
Copy link
Contributor

And what about the program itself? I mean, you download that as well without verifying any checksum.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

I reviewed it, I can distribute the file myself or download it again and compare the checksum with the code that I reviewed.
There is a difference between me downloading the programm and the programm downloading something else without checking and then later executing it as root.

Do we really have to discuss that downloading files that are executed as root without checking a checksum is not the greatest idea? Especially if you consider that its a software related to privacy, make it as safe as possible, there is NO REASON TO NOT VERIFY THE CHECKSUM.

@njfox
Copy link
Contributor

njfox commented Mar 31, 2018

NO REASON TO NOT VERIFY THE CHECKSUM.

Except that I'm pretty sure it literally doesn't do anything in this instance. But again, maybe I'm misunderstanding the issue--in which case, feel free to ignore me.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

I read the script once, download the second script check the checksum, I successfully reviewed both and I know until the main script updates the checksum I can trust both.

As long as there is no checksum check I can't tell anyone that its safe to use, because I don't know what file its going to download.

@Rafficer
Copy link
Contributor

The only benefit I see is when you really download the file once, distribute it yourself across the machines and never download another version and never update it.

And I would say 99% of users won't do it this way. And if you download the script from github before installation, there is no security benefit from it.

You could add the check yourself for your version, but overall it doesn't give a benefit to most users. And if the script gets updated, you have to take the extra step to update the checksum in the other file.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

There is no reason to verify checksums of files that are downloaded and executed as root. There is absolutely no reason to worry. IT WAS JUST A PRANK!11 APRILS FOOLS HEHEHEHE!!1

And I would say 99% of users won't do it this way. And if you download the script from github before installation, there is no security benefit from

You could add the check yourself for your version, but overall it doesn't give a benefit to most users. And if the script gets updated, you have to take the extra step to update the checksum in the other file.

Got you, right lets add an update mechanism to protonvpn-cli too, how do we update it? Can we use plaintext instead of https? Most users don't care about https and because we are using a VPN (ProtonVPN btw) we are safe anyways right?

ProtonVPN: Secure and Free VPN service for protecting your privacy

@mazen160
Copy link
Collaborator

Hi everyone

The ideas that came up to me to define the risk model is:

  • wget checks the integrity of the certificate of https://raw.githubusercontent.com. If someone is doing MITM attacks, it would fail typically.

  • If https://raw.githubusercontent.com is compromised, then Github is compromised. This is a scenario that we should have in mind that everything publicly that we use today is hosted in a way or another at Github. There is no real to escape this part. We should trust Github as everything is centralized over Github.

On the other hand, if Github is compromised, you as a user will be compromised too eventually using any of the packages that uses Github you use in your daily life.

Please put this as my personal thought (not ProtonMail's thoughts) to avoid confusion.

I read an article recently about how everything is centralized on Github, but can't find a link to it.

  • I will check how to add and generalize the process of signing the protonvpn-cli code. Meanwhile, I will add hashsum check to the external file. Please note that the external file is hosted at https://github.com/ProtonVPN/scripts. It's hosted in our repo for a reason, while referring and crediting the source of the script.

@Rafficer
Copy link
Contributor

Those are the discussions I like. Constructive and based on arguments. /s

There is no reason to verify checksums of files that are downloaded and executed as root. There is absolutely no reason to worry.

I've never said that. But there won't be a benefit from verifying the checksum within the script.

If the file is malicious, this means either ProtonVPN's github was hacked or ProtonVPN themselves turned malicious. In both scenarios, they could change the main script and attack you with that already (because that won't be verified). I really don't see the security benefit from automatically checking the file.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

@mazen160 What if someones account with push access to https://github.com/ProtonVPN/scripts gets compromised?

@Rafficer yes sorry, but its getting funny. What is the reason to not verify the checksum?

You can still have an genuine "main script" which downloads the compromised one.
You can review the script and it still can download a compromised script.

What is the reason that the OpenVPN hook script is "updatable" without checking a checksum, why is the main script not updatable?

I still don't see why you are AGAINST adding a checksum check?

In both scenarios, they could change the main script and attack you with that already (because that won't be verified). I really don't see the security benefit from automatically checking the file.

You download the main script proactively, if you don't read it and run it you are at fault, but why is it necessary to download another script without checking the checksum.
My review of the protonvpn-cli is rendered useless every time it downloads the script again, I can use a stable/known/old version of protonvpn-cli that I have personally reviewed, but I can still get compromised, because you don't see the benefit of checksums.

@mazen160
Copy link
Collaborator

@Duncaen We do put extra security measures to accounts with access to data/code such as https://github.com/ProtonVPN/scripts. Although everything should be possible in our minds, I highly don't expect this to occur.

In all cases, we should create hypothetical scenarios and build security measures to protect against it.

I'm adding the check tonight.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

In all cases, we should create hypothetical scenarios and build security measures to protect against it.
I'm adding the check tonight.

Thank you, that is the first reasonable reply.

There is no reason to not do this if the scenario exists and there is a simple way to avoid it.

Thanks, have a nice weekend.

@xilopaint
Copy link
Contributor

xilopaint commented Mar 31, 2018

If ProtonVPN GitHub account is compromised this repository would be compromised as well. It implies that even if update-resolv-conf has its integrity checked you would still need to audit the main script itself. That said, are you aware protonvpn-cli is changed several times a day? Do you have in mind to "audit" the whole main script after/before ever single update you do in your machine?

The problem here is not your concern, but the false sense of security that your solution implies. It simply doesn't address the problem in the real world.

@Rafficer
Copy link
Contributor

Yep. I'm not against it, but I just don't see the benefit, but I see the downside in terms of maintenance cost (given that the script would change, which doesn't happen anyway) and false sense of security.

The script checks if resolv-conf is installed with the requirement check. In 99% of the cases, you run into issues there when you run the script for the first time and that's also when resolv-conf.sh will be downloaded. After that, never again.

So in most (and it's by far the most) the script is downloaded from github and executed as root, without any kind of check, then it checks if resolv-conf is legit... At this point, the PC is already hijacked if ProtonVPN's github account got hacked.

That is why I don't see what it adds. The only benefit is when you download the script once, check if it's done correct and from there on never download it on any machine again ever. And also don't update it without checking the full code every time before doing so.

And I don't think we have to debate how many users will go through that procedure.

@Duncaen
Copy link
Author

Duncaen commented Mar 31, 2018

Right @xilopaint, the self updating mechanism is as dangerous as downloading the second script, but at least I have to initiate the update manually.

I personally would tag releases and if possible remove the update mechanism.
If the update mechanism is required I would consider adding signatures to the tagged releases and verify them before installing.

But lets make small steps, It took 20 replies to get one checksum lol...

@mazen160
Copy link
Collaborator

I agree with all the ideas mentioned here. The added value is quite low, as it's relatively almost impossible (we should not say impossible to any scenario) to execute the scenario. However, an added security addition that doesn't break functionality does not harm :)

@mazen160
Copy link
Collaborator

Commit: 434804c

@xilopaint
Copy link
Contributor

But lets make small steps, It took 20 replies to get one checksum lol...

It's not how security works. The approach should not to be making a step and think about the next after that. We should think on the whole scenario before moving forward.

@Rafficer
Copy link
Contributor

The only issue I see now is that the file gets saved as /etc/openvpn/update-resolv-conf anyway, but not executable. This makes it impossible to connect for the user as the program just checks if that file exists and then leaves it as it is.

I'm going to make a fix tomorrow for that if nobody comes forth.

@xilopaint
Copy link
Contributor

The only issue to me is that this file is completely useless for macOS and it keeps being installed. 😂

@mazen160
Copy link
Collaborator

mazen160 commented Apr 1, 2018

@Rafficer Done ;)

@xilopaint I'm doing something now for this part :)

@mazen160
Copy link
Collaborator

mazen160 commented Apr 1, 2018

Thanks everyone for the awesome discussion!

@mazen160 mazen160 closed this as completed Apr 1, 2018
@njfox
Copy link
Contributor

njfox commented Apr 1, 2018

I personally would tag releases and if possible remove the update mechanism.

I am actually in favor of tagging releases. master changes very rapidly and release tags (and a lack of a built in update mechanism) would make it much easier to build distribution packages for #27

@mazen160
Copy link
Collaborator

mazen160 commented Apr 1, 2018

@njfox Yup, this will change soon. Once we finish Issue:65, and implement some of the features in the roadmap, this will be done 👍

@xilopaint
Copy link
Contributor

I am actually in favor of tagging releases. master changes very rapidly

This is the main reason why I'm not in favor of tagging releases in this moment.

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

No branches or pull requests

5 participants