Skip to content

Conversation

@mossyhub
Copy link

No description provided.

@mossyhub mossyhub closed this Sep 14, 2024
@mossyhub mossyhub reopened this Sep 14, 2024
@caesay
Copy link
Member

caesay commented Sep 14, 2024

Hi, thanks for the contribution, but can you add some more context to this PR?

What does "SignatureState.SignedAndTrusted" mean in this context, does that mean it has a valid authenticode signature? Or that it has been signed by a trusted root in the OS store? How do you go about signing that package? Is that a manual step after vpk pack? what steps need to be taken? etc. Does reconstructing a package from deltas break the package signature? Is this change cross platform? can signing be performed and verified on mac / linux?

As far as the code itself, the signature check should probably be combined with the current checksum method to deduplicate some code and avoid needing to do the fast fails (like file length check) more than once.

@mossyhub
Copy link
Author

mossyhub commented Sep 14, 2024

Hi, thanks for the contribution, but can you add some more context to this PR?

What does "SignatureState.SignedAndTrusted" mean in this context, does that mean it has a valid authenticode signature? Or that it has been signed by a trusted root in the OS store? How do you go about signing that package? Is that a manual step after vpk pack? what steps need to be taken? etc. Does reconstructing a package from deltas break the package signature? Is this change cross platform? can signing be performed and verified on mac / linux?

As far as the code itself, the signature check should probably be combined with the current checksum method to deduplicate some code and avoid needing to do the fast fails (like file length check) more than once.

1st, I need to hold off on submitting this as I work for MSFT and should run my contribution through official channels first since this might be work related. Otherwise I would just have to ask for the feature and hope it gets added :-)

The SignatureState.SignedAndTrusted status in the context of Microsoft.Security.Extensions indicates that the file has a valid Authenticode signature and that the signature is trusted. This means the signature is verified against a trusted root certificate in the OS store as well (which all major software signing certs would be). It is Windows specific, so I made it optional. I am not presonally aware of any universal way (Win/OSX/Linux) to validate a files signing cert. Perhaps, I could change the name of the option to make it indicate it's for Windows only? This is why I don't think adding it to the hash check methods would be best since its OS specific and not everyone will choose to sign their packages.

As far as signing it, yes, for me the package files from vpk commands are signed downstream in pipelines (by the same cert used to sign the application binaries). An extra security level in case someone was to create MITM attack replacing downloaded update files cleverly. I am told MSIX has similar logic to validate update package files before being willing to extract them. You could choose to modify your vpk code as well to optionally sign the update packages.

"Does reconstructing a package from deltas break the package signature?" yes, but my hope was that my code checks only after the update manager downloads either a full package file or a delta package(s). It should not check the signature of a local full package after it has been reconstructed, nor should it need to if it already checked the individual delta files before allowing them to be rebuilt into a new full package.

@mossyhub
Copy link
Author

mossyhub commented Sep 16, 2024

Actually, looking at your AuthenticodeTools IsTrusted method, My method is basically a 1:1 replacement for that as far as what it accomplishes.

@caesay
Copy link
Member

caesay commented Sep 16, 2024

In that case we may want to be consistent. Either use authenticodetools in both places (and avoid an additional dependency) or take the library and deprecate our existing code.

As far as this approach in general goes, since this is Windows only (and I'm still unclear about how you would actually go about signing a package) I'm slightly hesitant to merge it.

If we could make it much more clear that this is a windows only authenticode check, eg with supported os annotations and better naming. And also document how to go about creating signed packages, it might be good enough to go in! In general though we would still need to look for similar solutions on Mac and Linux.

@mossyhub
Copy link
Author

mossyhub commented Sep 16, 2024

In that case we may want to be consistent. Either use authenticodetools in both places (and avoid an additional dependency) or take the library and deprecate our existing code.

As far as this approach in general goes, since this is Windows only (and I'm still unclear about how you would actually go about signing a package) I'm slightly hesitant to merge it.

If we could make it much more clear that this is a windows only authenticode check, eg with supported os annotations and better naming. And also document how to go about creating signed packages, it might be good enough to go in! In general though we would still need to look for similar solutions on Mac and Linux.

Agree 100%. I will play around with it a bit more to address this. It certainly simplifies the code to take the dependency, Where is the repo for your documentation?

@caesay
Copy link
Member

caesay commented Sep 16, 2024

@caesay
Copy link
Member

caesay commented Sep 22, 2024

As one additional point of reference, powershell is the only open source thing I can find which depends on Microsoft.Security.Extensions, and it also has a fallback to WinVerifyTrust.

https://github.com/PowerShell/PowerShell/blob/389f9dba8879bfacfef76337eb661eb640bb6218/src/System.Management.Automation/security/Authenticode.cs#L278

@mossyhub
Copy link
Author

unfortunately I have had to go in a different direction so will close this pull request.

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

Successfully merging this pull request may close these issues.

2 participants