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

install_release.sh wrong SERVER_URL and SHA256 #4098

Closed
2 tasks done
pupitetris opened this issue Jun 20, 2023 · 14 comments
Closed
2 tasks done

install_release.sh wrong SERVER_URL and SHA256 #4098

pupitetris opened this issue Jun 20, 2023 · 14 comments

Comments

@pupitetris
Copy link

  • I have read the FAQ.
  • I have searched in existing issues.

Environment

  • OS: Debian bookworm (latest)
  • scrcpy version: 2.0
  • installation method: manual build
  • device model: not relevant
  • Android version: not relevant

Describe the bug
After a successful installation, scrcpy won´t run because install_release.sh pulled an old version of the scrcpy-server binary.

On errors, please provide the output of the console (and adb logcat if relevant).

$ scrcpy --no-audio
scrcpy 2.0 <https://github.com/Genymobile/scrcpy>
/usr/local/share/scrcpy/scrcpy-server: 1 file pushed, 0 skipped. 20.2 MB/s (42151 bytes in 0.002s)
[server] ERROR: Exception on thread Thread[main,5,main]
java.lang.IllegalArgumentException: The server version (1.25) does not match the client (2.0)
	at com.genymobile.scrcpy.Server.createOptions(Server.java:166)
	at com.genymobile.scrcpy.Server.main(Server.java:330)
	at com.android.internal.os.RuntimeInit.nativeFinishInit(Native Method)
	at com.android.internal.os.RuntimeInit.main(RuntimeInit.java:355)
ERROR: Server connection failed

The solution was to get the url for the v2.0 binary, calculate the SHA256 hash and replace the values on the script:

PREBUILT_SERVER_URL=https://github.com/Genymobile/scrcpy/releases/download/v1.25/scrcpy-server-v1.25
PREBUILT_SERVER_SHA256=ce0306c7bbd06ae72f6d06f7ec0ee33774995a65de71e0a83813ecb67aec9bdb

to:

PREBUILT_SERVER_URL=https://github.com/Genymobile/scrcpy/releases/download/v2.0/scrcpy-server-v2.0
PREBUILT_SERVER_SHA256=9e241615f578cd690bb43311000debdecf6a9c50a7082b001952f18f6f21ddc2

And run the script again. Now scrcpy runs fine.

I suppose those values on the script should be generated automatically using a template or something when generating the release.

Cheers!

@rom1v
Copy link
Collaborator

rom1v commented Jun 21, 2023

The script install_release.sh is updated once the release binaries are generated (of course, because it includes a release checksum). But the release binaries are generated once the version is tagged (btw the tagname is used for the release name). So the tag may not contain the new install_release.sh script.

Therefore, this script is always updated one commit after the tag (along with the github pages referencing the new version): abc1be4

For install_release.sh, it would be possible to cheat, because the scrcpy-server binary is not impacted by this script, so we could re-tag after the binaries are generated, but it feels wrong.

This script is always intended to be executed on the master branch as a convenience to install the latest release.

@pupitetris
Copy link
Author

OK, gotcha. Actually, yes: the version of the code I downloaded was from a release tag. Thanks.

@rom1v rom1v mentioned this issue Aug 6, 2023
2 tasks
rom1v added a commit that referenced this issue Jul 17, 2024
The install_release.sh script is updated one commit after the release
tag, which may be confusing.

For convenience, new lightweight tags have been added (for example
v2.5-install-release) to point to the commit where install_release.sh is
updated.

But these tags interfere with "git describe" to generate pretty
filenames when executing ./release.sh on a development branch, so ignore
them.

Before:

    release-v2.5-install-release-17-gc57a0512b

After:

    release-v2.5-18-gc57a0512b

Refs #4098 comment <#4098 (comment)>
FreedomBen pushed a commit to FreedomBen/scrcpy that referenced this issue Aug 2, 2024
The install_release.sh script is updated one commit after the release
tag, which may be confusing.

For convenience, new lightweight tags have been added (for example
v2.5-install-release) to point to the commit where install_release.sh is
updated.

But these tags interfere with "git describe" to generate pretty
filenames when executing ./release.sh on a development branch, so ignore
them.

Before:

    release-v2.5-install-release-17-gc57a0512b

After:

    release-v2.5-18-gc57a0512b

Refs Genymobile#4098 comment <Genymobile#4098 (comment)>
@adityatelange
Copy link

Hello @rom1v,

"This script is always intended to be executed on the master branch as a convenience to install the latest release"

please consider tagging it to releases or tags.
what we can do here is generate checksums.txt file along with the binary file.

kindly refer to releases here as an example. https://github.com/gohugoio/hugo/releases/tag/v0.131.0

This will ensure the checksums are available to everyone and are not hard-coded into the install script.

We can then modify the install script to verify the hash. Kindly refer this script as an example https://gist.github.com/adityatelange/98a6b8e8cf9551a5eeee129aead01823

Regards.

@pupitetris pupitetris reopened this Aug 3, 2024
@pupitetris
Copy link
Author

Reopening issue as the script's existence is still misleading users into using it, and there is now a proposed solution to improve the script and make it work not only for the master branch but for future releases as well.

@rom1v
Copy link
Collaborator

rom1v commented Aug 3, 2024

I totally agree that it is confusing.

In theory, install_release.sh should not be in the same repo, because then it creates a cyclic reference if we want the updated script to be included in the tag (the tag depends on updating the file, but updating the file depends on the release which depends on the tag). But it is still convenient to put it there.

Before that script, the commands to execute were just documented. The script just does it automatically.

what we can do here is generate checksums.txt file along with the binary file.

There is already such a file, but I don't want to use that for install_release.sh. Btw, if the file is at the very same place as the binary file, the checksum is useless to detect tampering.

It is important that the checksum is hardcoded in a commit, because that gives some level of trust (it cannot be maliciously modified without a high probability of being noticed by many git users). By contrast, someone with write access to the repo might change the checksum file in the release, and install_release.sh would accept a malicious replaced file+checksum.

Of course, the solution to this problem is known: public key cryptography (I also publish a signature SHA256SUMS.txt.asc for every release with my gpg key), but I don't want install_release.sh to depend on that. The script is so simple that everyone can read it and understand what it does.

For install_release.sh, it would be possible to cheat, because the scrcpy-server binary is not impacted by this script, so we could re-tag after the binaries are generated, but it feels wrong.

I might consider to move the tag just after the binaries are generated in the future, that would "solve" the problem. But I'm not sure yet.

@adityatelange
Copy link

I knew you'd come to this as from the way I mentioned, the binary will be safe from tampering but not checksusms.txt is not. And this is a never ending cycle. You have to trust Github here to provide the server package as it is without tampering.
With checksums what we are ensuring is users can verify the binary themselves.
IMHO we need not check the binary for checksum as a measure of tampering but as a measure that it is downloaded properly and is not corrupt/half finished.
In extreme cases where the system itself is compromised, that is not maintainer's job to protect the end user.
I hope that makes sense, would like to hear your thoughts.

@adityatelange
Copy link

adityatelange commented Aug 3, 2024

I am not sure how the release takes place, or maintainers directly drag and drop binaries as assets in a release.
Proposing following method, let me know if I can create a PR if you think this way is a good way to proceed.

  1. SHA256SUMS.txt
ca7ab50b2e25a0e5af7599c30383e365983fa5b808e65ce2e1c1bba5bfe8dc3b  scrcpy-server-v2.6.1
17a5d4d17230b4c90fad45af6395efda9aea287a03c04e6b4ecc9ceb8134ea04  scrcpy-win32-v2.6.1.zip
041fc3abf8578ddcead5a8c4a8be8960b7c4d45b21d3370ee2683605e86a728c  scrcpy-win64-v2.6.1.zip
  1. Updating the install instructions to clone the tag (this branch can be updated after each release by maintainer)

scrcpy/doc/linux.md

Lines 34 to 38 in 44b3fd8

```bash
git clone https://github.com/Genymobile/scrcpy
cd scrcpy
./install_release.sh
```

to

git clone https://github.com/Genymobile/scrcpy --branch v2.6.1
cd scrcpy
./install_release.sh
  1. install_release.sh
    We use the tag we cloned to download the binary and verify the checksum.
#!/usr/bin/env bash
set -e

GIT_TAG_VERSION=$(git describe --exact-match --tags)

BUILDDIR=build-auto
PREBUILT_SERVER_URL=https://github.com/Genymobile/scrcpy/releases/download/$GIT_TAG_VERSION/scrcpy-server-$GIT_TAG_VERSION
PREBUILT_SERVER_CHECKSUM=https://github.com/Genymobile/scrcpy/releases/download/$GIT_TAG_VERSION/SHA256SUMS.txt

rm -rf scrcpy-server*
rm -rf  SHA256SUMS.*
echo "[scrcpy] Downloading prebuilt server..."
wget "$PREBUILT_SERVER_URL"
echo "[scrcpy] Downloading prebuilt server checksum..."
wget "$PREBUILT_SERVER_CHECKSUM"
echo "[scrcpy] Verifying prebuilt server..."

PREBUILT_SERVER_SHA256=$(grep "scrcpy-server-$GIT_TAG_VERSION" SHA256SUMS.txt)
if echo $PREBUILT_SERVER_SHA256 | sha256sum --check ;then
	echo "[scrcpy] Building client..."
	rm -rf "$BUILDDIR"
	meson setup "$BUILDDIR" --buildtype=release --strip -Db_lto=true \
	    -Dprebuilt_server=scrcpy-server
	cd "$BUILDDIR"
	ninja
	echo "[scrcpy] Installing (sudo)..."
	sudo ninja install
else
	echo "[scrcpy] Checksum verification failed!"
fi

@rom1v
Copy link
Collaborator

rom1v commented Aug 3, 2024

Sorry, there is no way the script will download and accept a file that can be replaced on the remote server in the future.

@Coool
Copy link

Coool commented Aug 21, 2024

@adityatelange

#!/usr/bin/env bash
set -e

GIT_TAG_VERSION=$(git describe --exact-match --tags)

BUILDDIR=build-auto
PREBUILT_SERVER_URL=https://github.com/Genymobile/scrcpy/releases/download/$GIT_TAG_VERSION/scrcpy-server-$GIT_TAG_VERSION
PREBUILT_SERVER_CHECKSUM=https://github.com/Genymobile/scrcpy/releases/download/$GIT_TAG_VERSION/SHA256SUMS.txt

rm -rf scrcpy-server*
rm -rf  SHA256SUMS.*
echo "[scrcpy] Downloading prebuilt server..."
wget "$PREBUILT_SERVER_URL"
echo "[scrcpy] Downloading prebuilt server checksum..."
wget "$PREBUILT_SERVER_CHECKSUM"
echo "[scrcpy] Verifying prebuilt server..."

PREBUILT_SERVER_SHA256=$(grep "scrcpy-server-$GIT_CLONED_VERSION" SHA256SUMS.txt)
if echo $PREBUILT_SERVER_SHA256 | sha256sum --check ;then
	echo "[scrcpy] Building client..."
	rm -rf "$BUILDDIR"
	meson setup "$BUILDDIR" --buildtype=release --strip -Db_lto=true \
	    -Dprebuilt_server=scrcpy-server
	cd "$BUILDDIR"
	ninja
	echo "[scrcpy] Installing (sudo)..."
	sudo ninja install
else
	echo "[scrcpy] Checksum verification failed!"
fi

I'm blind. Can't see where $GIT_CLONED_VERSION variable defined!

@adityatelange
Copy link

I'm blind. Can't see where $GIT_CLONED_VERSION variable defined!

Try developing an ability to correlate that it is a typo for GIT_TAG_VERSION

@pupitetris
Copy link
Author

Sorry, there is no way the script will download and accept a file that can be replaced on the remote server in the future.

You can use the git hash then. Since that depends on the previous changes, it cannot be altered: https://stackoverflow.com/questions/460297/git-finding-the-sha1-of-an-individual-file-in-the-index#answer-460422

@Coool
Copy link

Coool commented Aug 22, 2024

@pupitetris, strange people are arguing if any of git used command returns valid hash compared to sha command. Haven't used and needed such git function. But if returns wrong hash it's usless.

@adityatelange
Copy link

Sorry, there is no way the script will download and accept a file that can be replaced on the remote server in the future.

So you have trust issues on GitHub or the future maintainer of this project. Alright, so if they can replace the remote server file they can ofcourse rewrite the git history as well.

What I can also see is the commits aren't signed by any key so they can do it without anyone even noticing it.

Thoughts?

@rom1v
Copy link
Collaborator

rom1v commented Sep 15, 2024

I might consider to move the tag just after the binaries are generated in the future, that would "solve" the problem. But I'm not sure yet.

Tag v2.7 points to the updated install_release.sh 🚀

@rom1v rom1v closed this as completed Sep 15, 2024
Gottox pushed a commit to Gottox/scrcpy that referenced this issue Sep 29, 2024
The install_release.sh script is updated one commit after the release
tag, which may be confusing.

For convenience, new lightweight tags have been added (for example
v2.5-install-release) to point to the commit where install_release.sh is
updated.

But these tags interfere with "git describe" to generate pretty
filenames when executing ./release.sh on a development branch, so ignore
them.

Before:

    release-v2.5-install-release-17-gc57a0512b

After:

    release-v2.5-18-gc57a0512b

Refs Genymobile#4098 comment <Genymobile#4098 (comment)>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants