flathub / org.signal.Signal

https://flathub.org/apps/details/org.signal.Signal
64 stars 37 forks source link

Trustworthiness of non-official Flatpak packages #171

Closed kees-closed closed 2 years ago

kees-closed commented 3 years ago

Hi,

Although I greatly appreciate that the community stepped in and created this Flatpak, I have some concerns regarding the authenticity of the installs.

If this was an official RPM in the Fedora repository, I would have less doubts. Even if that of course is also managed by a community. Fedora has crypto authenticity checks, build check pipelines, review and testing procedures, macro's to enforce system wide security and configuration defaults, etc.

Flatpak seems more "do whatever you think is best". I therefore see more of an issue with authenticity. For example, someone could probably change the code to exfiltrate messages to a 3rd party. Stuff like that. If there are no authenticity checks, this won't be detected. Unless I'm wrong, then please correct me.

Flatpak is originally meant for developers to directly offer their app to the community. But Signal doesn't care about Flatpak and only supports a .deb file for Debian/Ubuntu...

So my questions come down to the following and I hope you can take some doubts away:

bermeitinger-b commented 3 years ago

I understand and support your issue regarding the security of the flatpak builds. You mostly answered your question with

But Signal doesn't care about Flatpak and only supports a .deb file for Debian/Ubuntu...

This is the reason (among sandboxing and runtime requirements) for the existence of this flatpak repo. If there would be none, there is no (easy) possibility to install Signal Desktop on non-apt distributions.

The update process is now done with a bot which scans for new versions directly from the Signal's apt-repo and then creates a Merge Request with the new version. The "only" verification is the SHA256 sum, you are correct. If you find a way to verify the binary with Signal's key, you're open to work on it and we'll happily accept the added security. A problem here is that the deb file itself is not signed and there is no specific signature file on the server. Also, flatpak does not support GPG verification the same way as hash verification, so we would have to implement it manually.

The signal.sh file is needed for launching Signal Desktop because it is an Electron application which needs the zygote-wrapper.

If you follow the discussion about the RPM distribution, you can see that their main argument is that they do not have the time/money/personnel to maintain different packaging formats. If they add flatpak, the next person comes with snap support, then AppImage, and so on.

There might be ways to attack this application by attacking any of the intermediate layers (Flatpak, Electron, node, etc.), the apt-repository, the Flathub repository, this repository, Flathub's build server, and also any npm dependency, and many more.

This Flatpak is not endorsed or in any kind supported by anyone from Signal.

tjburrows commented 3 years ago

I'm not sure what they did it but Opensuse at least has a package. Thankfully I use Opensuse.

kees-closed commented 3 years ago

The update process is now done with a bot which scans for new versions directly from the Signal's apt-repo

As far as I understand Debian-based repo's, they don't sign packages, but only the release file in their repo which contains the hashes of the packages. The Signal repo has disabled file listing, so I can't find the release file. But maybe you know where the release file is? Since you were also able to find the package in their repo. With that file I can create a script that verifies the release file with the Signal key. And then loops through the package names and filters out the signal-desktop hash. Then simply do a compare with the .deb file. If that matches, then continue.

kees-closed commented 3 years ago

Got the info from the Signal forum. I'll work on a script to extract this information soon.

kees-closed commented 3 years ago

Here is the script, sorry for the delay: https://github.com/AquaL1te/secure-signal-download/blob/main/secure-signal-download I don't see how I can do a pull-request in this repo for this script. Any advice?

bermeitinger-b commented 3 years ago

Thank you for this script. However, how does this differ from the current process and against which attack will this be needed?

The public key keys.asc, the InRelease and Packages file, and the deb file are all coming from the same source.

If I'm able to modify the deb file on the server, I'll also modify the other files. This adds a false sense of security, if a file is signed it matters who signed it.

We could add the keys.asc to this repo and verify it somehow. I still think this should be integrated into the builder and not here into this repo. The update checker receives the files directly from this server but lacks the GPG verification.

If you have a clean pull request, then fine but if this verification process takes more lines than we currently have, I think it's not feasible to maintain and implement this process.

For the PR: You have to fork this repo, commit the changes to your fork and then you can do a Pull Request.

kees-closed commented 3 years ago

I updated the script with an extra check that it really downloads and verifies the signal-desktop file.

against which attack will this be needed?

This is to make sure the hash is really verified and not altered when the download is finished. As I understand your process as it is now, the Signal file is downloaded, a hash is made, and then compared to the hash in the Flatpak XML? In that case you could download the same corrupted file twice.

This hash check compares it to the signed file by the Signal devs. This makes sure we really have the same file as Signal uploaded. APT does the same kind of checks.

If I'm able to modify the deb file on the server, I'll also modify the other files.

If you mean your own Flatpak build server, then yes. The people and systems there should be considered secure in this context. The gap this script fills is the uncertainty of the file you pull from the Signal repo. Whatever happens on your build server is in your hands. An infected Debian/Ubuntu system has the same weakness, when it has a malicious root user on the system. But what's important here is to make sure the correct file is downloaded before extracting it into the Flatpak.

The public key keys.asc, the InRelease and Packages file, and the deb file are all coming from the same source.

True. And they are downloaded over a TLS connection. The HTTPS CA infrastructure surely has its flaws, but since the files are signed by their private GPG key, the verification is solid enough. APT by default doesn't use HTTPS anyway and fully relies on GPG signatures.

If you have a clean pull request

The reason I asked about this is because I don't see the script/process you use to download the DEB file and verify the hash. So I don't know where to add this. But I suppose it's that Python script you mention.

bermeitinger-b commented 3 years ago

This is to make sure the hash is really verified and not altered when the download is finished. As I understand your process as it is now, the Signal file is downloaded, a hash is made, and then compared to the hash in the Flatpak XML? In that case you could download the same corrupted file twice.

The reason I asked about this is because I don't see the script/process you use to download the DEB file and verify the hash. So I don't know where to add this. But I suppose it's that Python script you mention.

This was the case before, now, this Python script checks the actual apt-repo and gets the information there. It is wrapping python-apt to check for new versions. If I read the code correctly, it takes this information from the Packages file. So, there is at least a check if the file in the repo is the same as the builder downloads. However, the update checker script does not verify the Packages file against the public key. I recommend pushing into that direction instead of this repo.

Additionally, isn't the flatpak-builder process sandboxed so that there is no possibility to wget/curl something that is not defined as a source? This way, we would have to update the SHA256 sums of the keys.asc, InRelease, and Packages files in the manifest as well because each source must have a checksum.

kees-closed commented 3 years ago

Additionally, isn't the flatpak-builder process sandboxed so that there is no possibility to wget/curl something that is not defined as a source? This way, we would have to update the SHA256 sums of the keys.asc, InRelease, and Packages files in the manifest as well because each source must have a checksum.

I'm not informed about the Flatpak build process :) But you don't need to have all hashes.

Ideally the public key would be pulled from here: https://keys.openpgp.org/ Which fixed the flaws of the SKS infrastructure: https://gist.github.com/rjhansen/f716c3ff4a7068b50f2d8896e54e4b7e

But since Signal does not have a key on https://keys.openpgp.org/, we will have to do with this I guess.

The reason we don't need these hashes is because the InRelease file is signed by the private key of Signal. We can verify the InRelease file with the public key. In that file is the hash of the Packages file. In the Packages file is the hash of all the packages, including the signal-desktop.deb file. If anything was altered, then this verification chain would fail. And since the private key is not on the server, we can assume the InRelease file is valid. But yeah, if they used https://keys.openpgp.org/ it would've been better.

I don't think I'll have the time soon to look into the Python stuff. But changing the Bash script to validate the hash in the Flatpak can be done easily by extending the script. But I guess you know this build process better, where could be do this best? Let me know where I should send my pull-request. Thanks.

bermeitinger-b commented 3 years ago

I'm not informed about the Flatpak build process :) But you don't need to have all hashes.

Unfortunately, yes, because during the build, there is no internet access. This means that we have to include the files (1) either in this repo or (2) download them like the deb file by adding the files to the manifest (the json file). (1) complicates future updates and for (2) we have to have the checksums because the build process requires a checksum for downloaded sources.

Let me know where I should send my pull-request. Thanks.

I guess this repo is fine, why wouldn't it be? This way also the buildbot tests the process.

kees-closed commented 3 years ago

Unfortunately, yes, because during the build, there is no internet access

That's not ideal indeed. Can't we just upload the deb file we verified with the bash script I created? All the other files are not necessarily needed, they are ephemeral and only needed up until the point the deb file is downloaded. The script outputs a verified hash of the deb file, the deb file is downloaded in that same process as well.

After that we may upload that deb file with the verified hash to the Flatpak build process? Or is it really needed to have this whole process on the build server? Because if that's the case then it would be better to focus on the debianrepochecker.py I think. But since I'm quite busy, that will take some more time. Writing the bash script was quite simple because it was standalone.

I guess this repo is fine, why wouldn't it be? This way also the buildbot tests the process.

Please understand that for me the whole process is unclear. I do understand the process with this debian repo checker, but not really the custom process we are discussing now. I can send a pull-request in this Signal Flathub repo, but I have no idea how the script then will be executed and what output would be useful for the build process. I see no existing script that does something similar, I suppose that's because the debianrepochecker.py is used, which takes care of everything. So with that said, I'm not sure how that pull-request would help, that's why I ask if this is really the correct place to submit this pull-request. :) You say it is, but then I need some more info to make a useful pull-request, because I don't see how this script as-is would help at this point.

bermeitinger-b commented 3 years ago

That's not ideal indeed. Can't we just upload the deb file we verified with the bash script I created?

That would mean that we download the deb file, verify against the key, and then upload it to host it here? (Or some other host?) Now, we have to trust the one who uploads the file. There is no security gained. Also, this would break the automatic update process by the repo checker. I anticipate there will be issues regarding hosting the deb separately rather than using the official source.

You say it is, but then I need some more info to make a useful pull-request, because I don't see how this script as-is would help at this point.

Currently, your script is incompatible with the process because it needs outside access. I don't know where else a PR makes sense, this is the repo for flathub's signal-desktop repo.

Again, against which attacker do you want to protect yourself? The debian repo checker scans the official apt repo at updates.signal.org and extracts the checksum from the repo's metadata and then submits a PR to this repo with the new URL and the checksum. Then, the buildbot uses this information to download the deb file from the signal.org repo and verifies the checksum. The repo checker does not verify the files with the public key (marked as #FIXME in the linked repo above).

Let's say an attacker has full access to updates.signal.org, changing the deb file would require the InRelease and the Packages files. The build process would not detect the malicious files because the checksums would match. However, verification with the public key would also make this useless because it is hosted on the same page and therefore could be overwritten by the attacker as well.

Any changes in transit of the deb file (apt → build server) would be detected by the checksum (also, it's https).

kees-closed commented 3 years ago

Thanks for explaining these extra details. Please understand that I'm just guessing blindly here about how this process works.

If you'll re-read this thread you can see that I first got the impression that this could be fixed with a script like the one I made. There was no mention of this Python script yet that retrieves data from the Signal repo, or details about how this build process is done. All this info was provided incrementally when I asked about it. With the information provided throughout the thread I agree that the build process is indeed not compatible with my script.

However, the initial problem is still unsolved. Without a GPG check there is room for authenticity issues. CAs have been hijacked before (to e.g. read Gmail of Iranian dissidents), DNS poisoning/hijacking exists and so on. Signal is a service used and trusted by journalists, activists and dissidents. There is no doubt there is a motive to hijack traffic from the Signal service. A weakly authenticated Signal desktop app is therefore a problem. It opens up the weakest link in the Signal ecosystem. Not for me necessarily, but more for those people that trust Signal with higher stakes.

A GPG verified hash of the app would really benefit casual users and high profile users. Flatpak was intended for developers to provide packages directly to the users. This is not happening in many cases, Signal is one of them. Anything we can improve in adding integrity checks to fill in the gaps of trust should be considered seriously.

However, verification with the public key would also make this useless because it is hosted on the same page and therefore could be overwritten by the attacker as well.

This is indeed the issue I addressed that Signal doesn't use proper GPG signing/verification workflows. This key should be on https://keys.openpgp.org/ and retrieved from there. This is something Signal must address.

Like I mentioned before, creating the Bash script was easy because it was standalone. Creating a pull-request for that Python script will require some more time to produce a quality pull-request. Time is not something I have in abundance at the moment :) But it will be on my mind, hopefully I can come back to this soon. :)

bermeitinger-b commented 3 years ago

Thank you for your response. If I seem hesitant or dismissive, this is or was not intentional. I want a

If you'll re-read this thread you can see that I first got the impression that this could be fixed with a script like the one I made. There was no mention of this Python script yet that retrieves data from the Signal repo, or details about how this build process is done. All this info was provided incrementally when I asked about it. With the information provided throughout the thread I agree that the build process is indeed not compatible with my script.

I'm sorry about this, I'm learning the build process here on the go. Only after I experimented with your script inside the building environment I noticed that there is no internet access. To make things clearer, this is the current update process. update-progress

However, the initial problem is still unsolved. Without a GPG check there is room for authenticity issues. CAs have been hijacked before (to e.g. read Gmail of Iranian dissidents), DNS poisoning/hijacking exists and so on. Signal is a service used and trusted by journalists, activists and dissidents. There is no doubt there is a motive to hijack traffic from the Signal service. A weakly authenticated Signal desktop app is therefore a problem. It opens up the weakest link in the Signal ecosystem. Not for me necessarily, but more for those people that trust Signal with higher stakes.

A GPG verified hash of the app would really benefit casual users and high profile users. Flatpak was intended for developers to provide packages directly to the users. This is not happening in many cases, Signal is one of them. Anything we can improve in adding integrity checks to fill in the gaps of trust should be considered seriously.

I fully agree with you. However, currently, this is not something that I can easily implement here. The update checker would be my main target.