Verify final code signature for issue #441 #523
Conversation
Tests whether a particular app is code signed or not. It does not verify the signature.
…o +[SUCodeSigningVerifier codeSignatureMatchesHostAndIsValidAtPath:error:] Explains more clearly what the method does.
Let's you check the validity of an arbitrary code signed app.
…thPath() The API docs say "For bundles, pass a URL to the root directory of the bundle… If you pass a URL to the main executable of a bundle, the bundle as a whole is generally recognized."
There's a zipped up sample app in the unit test target. This gets unzipped to a temporary folder and there are three versions of it: the original which is not code signed at all, another with a valid code signature, and a third which has had one of its resources removed after code signing to make it invalid.
This should help prevent situations like issue sparkle-project#441 where Sparkle creates broken app bundles. In this case, the update will just report as a failure instead of succeeding.
|
It's worth investigating if you have to do an OS check for a deep signature test. See: [edit]: also, we'd hope this doesn't break in future if another breaking code signing change were to happen. |
|
Just to let you know — it looks like a very important improvement. I'm currently very busy, but I'm going to give it a thorough review as soon as I can. |
|
I may back out of using the SecCSCheckNestedCode flag. Thanks for pointing out its potential issue. I also plan on doing more testing of this today. |
|
What if the app isn't code signed at all? AFAIK, Sparkle doesn't require apps to be code signed. |
|
That should still be handled but I think Sparkle is too lenient on providing an invalid DSA or code sig as long as one of them works, but not both have to be (if both are provided). |
This is less secure, but should enable the code to work with older code signatures.
|
To elaborate on what I was saying above. If we look at the current code in this pull request at https://github.com/iwankerl/Sparkle/blob/8bbe487c86886d27136054f0dc9923599baeec3f/Sparkle/SUBasicUpdateDriver.m#L222 We can see the logic allows a broken code signature to be specified, as long as the DSA is valid. Or vise versa: the logic allows the DSA to be invalid, as long as the code signature is valid. I think the logic should change to:
DSA is still useful and shouldn't be treated as an "alternative" to code signing IMO. It's for validating the download. Plus, archives can preserve important attributes that Apple's code signing doesn't bother to check for (file permissions!), and we know from the past that the code signing is not perfect. If both DSA and code signing is provided, we should make sure both are correct. |
|
zorgiepoo has a valid point. The DSA of the downloaded file should probably be checked every time if it is provided. On another note, in practice my pull request is not testing very well and I don't recommend merging it. It is proving difficult to replace the old app if a corrupt one is being or has been installed. What I would like to do in the mean time is add another updater delegate method so the host app can do some verification of the extracted item before the autoupdate install tool is launched. Something like:
We could still do a final code signature validation from the autoupdate tool and log unfavorable results but perhaps not cause an install failure. It's still not clear to me if the root cause of this bug is the in the extraction phase or the replacement of the old app phase. |
|
If the issue lies on Sparkle crashing during copying the app to its final destination, then no amount of verification is going to improve this issue. Which begs the question of why a copy operation of the app is being done in the last steps after the application has been unarchived/patched? Usually with copying files, file moves are often done in the final steps since they're atomic operations. If the destination is on a different volume from the source, you can copy it to a temporary path in the same volume, then do a move as a final step. On the other hand, if they're on the same volume and permissions are already sufficient or what not, a copy just takes extra time. Or maybe it already works like this? If so, odd that users would end up with missing resources (also see issue #498). |
|
This is very well written. Thanks for refactorings and checking the docs. Still, the root cause of breakage needs to be fixed, but this check won't hurt. |
Verify final code signature for issue #441
This should help prevent situations like issue #441 where Sparkle
creates broken app bundles.
Once the app has been installed, we check to make sure the final
copy has a valid code signature (if it's required to be signed).