sparkle-project / Sparkle

A software update framework for macOS
https://sparkle-project.org
Other
7.34k stars 1.04k forks source link

Improve signing error message to developers if the wrong file is being served #2468

Closed kornelski closed 9 months ago

kornelski commented 9 months ago

Summary

Signing errors are a massive pain for developers. Currently, if the signature doesn't match, we can only say that much. Why it doesn't match is a mystery to be solved by the developer. Because it involves unintelligible signatures, private keys, and cryptography, it's hard.

Possible Fix

Sparkle can catch a common mistake — the file that has been downloaded isn't the same file that has been signed — and report it explicitly. If Sparkle explicitly said "that's a wrong file!", then this eliminates all of the signing issues from the debugging, and makes the problem much easier to diagnose. The developer will know to check all the URLs and uploads, and/or sign a new file version.

On a technical side, this could be solved by including an explicit hash/digest of the archive file in the appcast. Even though dsa and eddsa signatures hash the archive as part of the verification process, unfortunately they don't do it in a way that is separable from the signature verification, so their hashing can't be reused. It would have to be a separate hash. It could be checked only if signing failed, to avoid extra work. Because it's not security-sensitive, it could be some fast crc algorithm or a truncated hash.

zorgiepoo commented 9 months ago

I believe this the error message we currently log to developers is:

@"EdDSA signature does not match. Data of the update file being checked is different than data that has been signed, or the public key and the private key are not from the same set."

We show a different error to the user that is a bit less technical / more user-friendly but I don't remember offhand what this message is.

I'm hesitant about adding an optional checksum attribute and doing additional cryptographic checks. Despite providing documentation I feel like we'll get questions about it and people will get the wrong idea that it's not a security check. This also seems like an attribute (further bloating the appcast) that solely helps developers diagnose issues, not really help users. Sparkle actually did have a MD5 checksum attribute a long time ago.

The enclosure does already typically have the length of the download item developers provide though. If you run into a signing error because the the file served is different than the one you signed, it is probably pretty unlikely the length of the downloaded archive will be the same as the length of the archive you signed. It is not as "good" as a crc but it's likely good enough. We could potentially leverage that info to create a better error message, if the lengths happen to differ. There is also a log warning emitted by the downloader today (in 2.x) if the HTTP content length differs from the expected content length but that's more subtle and only if we get a HTTP expected content length:

SULog(SULogLevelError, @"Warning: Downloader's expected content length (%llu) != Appcast item's length (%llu)", self->_expectedContentLength, self->_updateItem.contentLength);