Extend Authenticode signature verification #296
Conversation
The previous implementation only verified Authenticode signatures of
update binaries in rare cases, namely code red checks and app commands.
This commit adds a new registry DWORD value:
UpdateDev\AlwaysVerifyAuthenticodeSignatures
When set to a non-zero value, then update binaries are also verified in
normall install / update cycles.
|
I am going to look at this sometimes next week, I am currently AFK. |
|
Great, thank you |
|
So how would I deposit this to the market
…On Fri, Jul 9, 2021, 12:34 AM Michael Herrmann ***@***.***> wrote:
Great, thank you
|
|
Thank you very much for your review. I am afk this week and will hopefully be able to get back to you next week. |
AlwaysVerifyAuthenticodeSignatures -> VerifyPayloadAuthenticodeSignature
|
I pushed more commits and commented on your review. Would be fine for me to merge as it is now; But I would also be very happy to hear any further comments you might have. Thanks! |
Previous commits aliased them with hash verification failures.
|
I pushed two commits that (I hope) address your actionable comments. |
|
From my limited perspective, I suspect the decision whether to use a runtime check or a build flag will depend on whether Google wants to use the feature. If not, then the build flag limits Google's risk. If yes, then I personally would use the runtime check to limit the risk of a roll-out. But of course, as an outsider I can only guess at the right approach for Google. If you want me to rewrite the implementation to use a build flag, let me know. |
Right. Ganesh and I discussed the rollout scenario: who'd change the
As a compromise, this is a suggest we do. Keep the |
To clarify, I suggested a name change to be explicit in mentioning |
|
Thank you for your thoughts. I'll get started on the build flag implementation you suggested. However, in this case I will make the registry flag opt-out instead of opt-in. |
Previously, it was disabled and could be enabled by a registry key. Now, it is the other way around.
That's true, actually. I changed the method name to |
|
I've now pushed further commits to guard the new implementation behind a build flag (248bdc2) and to make the registry key opt-out instead of opt-in (f64c29a). Could you let me know your thoughts @sorinj @GitHubGanesh? |
|
We could probably merge the change like it is. I wonder what you think about making one more change, as suggested below. We are not fond of the churn introduced by conditional compilation in so many places. Would it make sense to just use We don't mind the extra increase in code size due to the code that we don't use right now. Thank you! |
Build flag VERIFY_PAYLOAD_AUTHENTICODE_SIGNATURE was checked in too many places. Reduce its usage to the few critical points.
|
No problem. I applied your suggestion. What do you think @sorinj? |
|
Thank you! |
lgtm |
|
Thank you! This will help our customers. |
* Extend Authenticode signature verification
The previous implementation only verified Authenticode signatures of
update binaries in rare cases, namely code red checks and app commands.
This commit adds a new registry DWORD value:
UpdateDev\AlwaysVerifyAuthenticodeSignatures
When set to a non-zero value, then update binaries are also verified in
normal install / update cycles.
|
Fixes #272.
The current implementation only verifies the Authenticode signatures of update binaries in rare cases, namely code red checks and app commands. This commit adds a new registry DWORD value:
When set to a non-zero value, then update binaries are also verified in normall install / update cycles.
Any feedback would be highly appreciated.