Sign the phar with SHA-1 instead of SHA-512 - #6632

2 things come to mind

• my understanding is that sha1 is considered insecure and a attacker might be able to spoof a wrong signature with sha1, but couldn't with sha256 (I am no security expert, but thats my understand atm)
• AFAIK is sha256 hardware accelerated on newer php versions.. this might not include phar signing though. maybe @TimWolla can shed some light on this

Thanks, both worth checking. The first one does not apply to this signature. You are right about the second one on x86.

On security: the phar signature has no key. Anyone who can change phpstan.phar can compute a new valid SHA-512 signature as easily as a SHA-1 one. So the signature only detects a damaged file. A collision attack matters when someone trusts a hash that someone else made. For the phar that is phpstan.phar.asc, which is RSA over SHA-512 and does not change here.

On speed, I timed Phar::loadPhar() on the 31 MB phar on GitHub runners, median of 7 (run):

On x86 runners that both report SHA-NI, PHP 8.5 hashes SHA-256 about four times faster than 8.3, which fits the hardware acceleration you mention. I did not test 8.4.

On both arm64 runners (one is an Apple M1), SHA-256 is the slowest of the three, and slower than today's SHA-512. SHA-1 is faster than SHA-512 on every platform here. Against SHA-512, SHA-256 would save about 60-70 ms on x86 with PHP 8.5, and cost about 65-95 ms on arm64. I would keep SHA-1, but SHA-256 is a one-token change if you prefer it.

• AFAIK is sha256 hardware accelerated on newer php versions.. this might not include phar signing though. maybe @TimWolla can shed some light on this

Yes, since PHP 8.4: https://tideways.com/profiler/blog/whats-new-in-php-8-4-in-terms-of-performance-debugging-and-operations. This also applies to Phar signatures if I read the code correctly.

my understanding is that sha1 is considered insecure and a attacker might be able to spoof a wrong signature with sha1, but couldn't with sha256 (I am no security expert, but thats my understand atm)

SHA-1 is only broken with regard to collisions, which are not applicable here. But if what Sander says is indeed true and it's just a hash check rather than an actual signature, then it's totally meaningless.

SHA-256 is the slowest of the three, and slower than today's SHA-512.

This is expected without SHA-NI, because SHA-256 uses 32 bit arithmetic, whereas SHA-512 uses 64 bit arithmetic.

Either way, the correct solution for security is a Sigstore attestation, not a PGP signature and not a Phar signature.

And FWIW: SHA-256 is only accelerated on x64. ARM also includes native instructions, but these are not included yet, because I'm unable to test correct functionality. The upstream library we used for SHA-NI also includes an ARM variant, so if anyone wants to add native SHA-256 for ARM for PHP 8.7 that should be easy enough to do. You would just need to test it.

Thank you Tim for the insights.

And FWIW: SHA-256 is only accelerated on x64. ARM also includes native instructions, but these are not included yet, because I'm unable to test correct functionality. The upstream library we used for SHA-NI also includes an ARM variant, so if anyone wants to add native SHA-256 for ARM for PHP 8.7 that should be easy enough to do. You would just need to test it.

@SanderMuller would be great you could try to work on such a php-src addition.

Either way, the correct solution for security is a Sigstore attestation, not a PGP signature and not a Phar signature.

yes, we are aware that we need additional hardening for that

@SanderMuller how is this resign.php file included in regular phpstan bootstrap?
since its part of compiler it looks like a build-time only file and might not affect running phpstan.phar (the release artifact)

Deciding SHA algorithm is a build time thing, but my question is rather whether this needs to be just in resign.php, or also somewhere in box.json settings for example too.

Inspect the built artifacts (phar-file), whether they use the expected algorithm or not.

resign.php is build-time only, yes. It runs in phar.yml right after box compile, on both builds, and Timestamps::save($file, Phar::SHA1) writes the signature again. So Box's default (SHA-512, box.json sets no algorithm) only reaches the intermediate file.

I checked the artifacts of this PR's Compile PHAR run (36694375075). phar-file and phar-file-checksum both have a SHA-1 signature, and none of their 7167 members has mtime 0. The commit job downloads phar-file, moves it into phpstan/phpstan and signs it with GPG into phpstan.phar.asc, which does not touch the phar's own signature. So the released phar is the one I checked.

"algorithm": "SHA1" in box.json would only change the intermediate file. I can add it if you want both to agree.

The "Download base SHA PHAR" red here was a race. The job ran at 09:13, and the Compile PHAR run of the base (#6633's merge) finished at 09:19.

Thanks @TimWolla, that explains the timings. On x86, 8.5 hashed SHA-256 in 32 ms against 125 ms on 8.3, and SHA-256 stayed the slowest on both arm64 runners.