fix(windows): patch_binary causing codesigning verification failure

[acx0]

fixes #13942

#13209 introduced changes causing codesigned binaries to fail verification

Fix has been verified by building the tauri CLI locally and using that to publish EV-signed binaries hosted on https://sentinowl.com

[github-actions]

Package Changes Through a2108dc

There are 7 changes which include tauri-cli with minor, @tauri-apps/cli with minor, tauri-utils with minor, tauri-bundler with patch, tauri with minor, @tauri-apps/api with minor, tauri-plugin with minor

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package	current	next
@tauri-apps/api	2.7.0	2.8.0
tauri-utils	2.6.0	2.7.0
tauri-bundler	2.5.2	2.5.3
tauri-runtime	2.7.1	2.7.2
tauri-runtime-wry	2.7.2	2.7.3
tauri-codegen	2.3.1	2.3.2
tauri-macros	2.3.2	2.3.3
tauri-plugin	2.3.1	2.4.0
tauri-build	2.3.1	2.3.2
tauri	2.7.0	2.8.0
@tauri-apps/cli	2.7.1	2.8.0
tauri-cli	2.7.1	2.8.0

Add another change file through the GitHub UI by following this link.

---

Read about change files or the docs at github.com/jbolda/covector

[Legend-Master]

Thanks! We'll need a change file and a cargo fmt and we can merge it

[acx0]

Thanks for the quick review @Legend-Master, I'll update the PR to include those.

What are your thoughts on making this error fatal?

    if let Err(e) = patch_binary(&main_binary_path, package_type) {
      log::warn!("Failed to add bundler type to the binary: {e}. Updater plugin may not be able to update this package. This shouldn't normally happen, please report it to https://github.com/tauri-apps/tauri/issues");
    }

I haven't had time to trace through how patching the package_type string into the binary affects client updates but based on the plugin may not be able to update this package log str, this seems like something that should be bubbled up immediately rather than being swallowed and left to hoping someone notices the warning amongst all the other build output.

Is there an RFC for this binary-patching feature that covers its scope and/or documents its failure modes?

[Legend-Master]

I haven't had time to trace through how patching the package_type string into the binary affects client updates but based on the plugin may not be able to update this package log str, this seems like something that should be bubbled up immediately rather than being swallowed and left to hoping someone notices the warning amongst all the other build output.

So this was changed in #13825 to basically prioritizing that we don't break the build and incrementally adding this feature (detecting installer type) at least in v2 (by the way this function hasn't landed yet, see tauri-apps/plugins-workspace#2624, everything related to updaters are kinda slow to make, they need to be backward compatible and they're hard to test, also the design will need to be made with no breaking changes in the future in mind)

Is there an RFC for this binary-patching feature that covers its scope and/or documents its failure modes?

Not really... And I wasn't involved much by the time this was merged, and we don't have any docs around this yet, mainly because tauri-apps/plugins-workspace#2624 is not merged yet

I would probably prefer we bundle a text file along side the executable but I said it a bit too late and the binary patching method didn't show much of a problem at the time (no body really tried the signing process during the time so that's why this leaked through)

[acx0]

Appreciate the context.

I would probably prefer we bundle a text file along side the executable but I said it a bit too late and the binary patching method didn't show much of a problem at the time (no body really tried the signing process during the time so that's why this leaked through)

Is it too late to back off the binary-patching approach? It seems like it's going to be prone to regressions/bugs if there's no straight forward way to unit-test/verify that future code changes don't result in signed binaries from failing verification.

We've already had users uninstall our app due to the "untrusted publisher" warnings that were showing up, before we realized what was going on, so I'm more than happy to spend any time/effort to contribute here in order to prevent something like that from happening again.

[Legend-Master]

It's quite bad we didn't find it earlier, I didn't even know you can sign a file twice to be honest

so I'm more than happy to spend any time/effort to contribute here.

That's awesome, but I currently don't have a better idea then the binary patching to be honest, the extra text file could probably work, but it would be also quite a lot of work, we need to make sure we place them at a good location, remember to sign them, remove them on uninstall, etc

Let me know if you have something in mind, also probably want to talk with @kandrelczyk here since he contributed that solution

And just by the way, I might get busy the next few weeks so I might be able to follow that closely

[kandrelczyk]

Hey,

Just to add what we currently have regarding unit-tests and see if anything can be added to cover issues like this one in the future.

In updater plugin we have test/app-updater tests that build, update and run the app on Linux and Windows. On Linux we use AppImage and run the app twice checking that on first run it's updated and on second run it's up to date. On Windows we do both Nsis and MSI but only run the app once to see that it is updated correctly.

Obviously the first issue here is that this functionality is split between tauri-cli and updater plugin. The tests are in updater plugin but we don't have any additional verification in tauri-cli. Maybe some test that runs 'sigcheck.exe` on each binary should be added? Would that detect this issue?

Do I understand it correctly that the current issue is caused by double signing the binary and only the second bundle is affected? In the unit test for updater plugin the "to be updated" app (v 1.0.0) is actually only bundled once. The updated version (v 1.0.1) is bundled twice but we never run it after updating. Would this issue have been detected by simply running the version with invalid signature? What exactly happens when we do this? Maybe the test should be modified to also run the app after update? I don't remember why it's not done on Windows.

As for changing the binary patching to some other method based on bundling files I could do the Linux part. If someone can take care of the Windows part I think this could be done in a manner transparent for the updater plugin so that we don't have to do more work there. If we are worried that binary patching might cause more issues in the future it could be worth doing.

[acx0]

Thanks for the additional context @kandrelczyk

1. and see if anything can be added to cover issues like this one in the future.

I'll take a look at the components you mentioned and see if I can add any tests that'll fail if the behaviour ever regresses to the 2.7.1 CLI functionality. I can follow up with these changes in a separate PR once I've had time to investigate/experiment.

2. some test that runs 'sigcheck.exe` on each binary

I think something along the lines of this will need to be done using some multiplatform signature verification tool (osslsigncode/jsign/etc) or rust library, I'm just not sure what the validation output will look like if the certificate file is self-signed - as I imagine it would be for certs created in unit tests. E.g. osslsigncode validates the entire certificate chain and outputs a "pass/fail" based on that. I imagine sigcheck does the same.

3. Do I understand it correctly that the current issue is caused by double signing the binary and only the second bundle is affected?

Can't say definitively because our build hosts are linux machines so bundle.rs::bundle_project() attempts to process both MSI and NSIS package types but only windows::nsis::bundle_project() is actually invoked since windows::msi::bundle_project() is gated behind #[cfg(target_os = "windows")] - but I imagine the first bundle would be processed correctly due to main_binary being in a clean/freshly-compiled state.

I believe if we'd been allowed to pass --bundles NSIS to our tauri build command we wouldn't have discovered this issue as settings.package_types() would only contained PackageType::Nsis. I was planning on submitting another diff to allow NSIS being supplied on linux builds of the tauri CLI.

But I imagine if the build is being done on a windows host the same bug would manifest as the signing tool we're using (jsign) is very popular for setups where the codesigning private key is hosted in a cloud-based KMS, and would end up being invoked in the same way we observed in linux.

4. Would this issue have been detected by simply running the version with invalid signature?

I don't think so- Windows SmartScreen only seems to pop up if the downloaded binary, i.e. the NSIS installer, is unsigned. Once you've run the installer and launch the extracted program binary, the "unknown publisher" SmartScreen warning never shows up again. The bug didn't affect the signing of the NSIS installer .exe otherwise we would have noticed during our CI/testing that the signature was invalid. SmartScreen would've also blocked the execution of the installer in this case.

We only noticed the invalid signature of the program binary because our program performs a one-time admin-escalated operation on new installations. This triggers a UAC prompt which verifies+renders the details of any codesign certificates attached to the binary, and in this case was showing as "unknown publisher" which is what drove us to look into the codesigning flow.

5. Maybe the test should be modified to also run the app after update?

We actually had the invalid-signature binaries deployed for about two weeks before we noticed the issue, and had also published a couple of automatic updates to users during that period without any errors. Our desktop agent emits client events/crash reports in the event of any update failures, which never occurred because of this behaviour mentioned above:

Once you've run the installer and launch the extracted program binary, the "unknown publisher" SmartScreen warning never shows up again

It was actually our website observability/analytics dashboard that showed us a spike in new users signing up and immediately deleting their accounts after downloading the binary, which prompted us to investigate.

[kandrelczyk]

Thanks @acx0 , now I understand what happened. I think the updater plugin tests are fine than and just some additional signature checks should be added to tauri-cli tests. There is already test present that bundles the patched binary so adding some signature verification shouldn't be hard.

[Legend-Master]

Thanks!

I couldn't test this since I don't have a Windows signing key (and not enough time to setup a self signed certificate), @FabianLars could you help testing it?

[FabianLars]

could you help testing it?

hmm idk how valuable i can be here. i only have it set up on windows and there the current live version works fine for me as well as far as i can tell. Edit: I assume this is specific to the cross-platform compilation or by the signtool you use but i can't test either rn.
This PR doesn't break that though so that's good enough for me.

[Legend-Master]

Thank you all

[kandrelczyk]

Hey, the work on the updater plugin stalled again. I understand everyone is busy but the work on tauri-cli will only be useful when we also merge the plugin PR so let's not forget about it :)

[acx0]

Thank you all for the quick feedback and reviews! @Legend-Master @kandrelczyk @FabianLars if you ever need me to test out any changes to the bundling/signing/autoupdate flows, please don't hesitate to @acx0 or email me:

• We use the NSIS bundler for our windows builds (via cargo-xwin on linux), and DMG bundler for macos builds (with signing+notarization enabled using our Apple Developer Program issued cert) so those should be quick/easy for me to test
• I imagine It shouldn't be too much work to provision a windows build host so I could probably assist with MSI bundler testing too
• We also have plans to distribute linux binaries soon and leverage the updater plugin there as well

Is there any other discussion forum/channel that tauri devs use for coordinating long-term changes? I'm not sure how much time I'll have to help out with new feature implementation at the moment but I'm definitely interested in providing assistance with testing/code review for upcoming bundling/updater changes, whether it's for the currrent binary-patching approach or considering/investigating any alternatives.

I've made some personal TODOs to look into the other possible work items that were discussed above, namely the --bundles parsing validation, but I'll reach out if I come up with any other ideas for testing the codesigning as well once I have some time.

[FabianLars]

Is there any other discussion forum/channel that tauri devs use for coordinating long-term changes?

hmm maybe the #contributing channel on our discord server (aside from github of course). we don't really have or discuss long-term plans atm though.