Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update NVDA's Authenticode code signing certificate #12398

Merged
merged 3 commits into from May 18, 2021

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented May 11, 2021

Link to issue number:

None.

Summary of the issue:

NVDA requires being signed with a trusted Authenticode code-signing certificate, so that it can access certain accessibility features on Windows such as UIAccess.
The current Authenticode certificate expires in July 2021.

Description of how this pull request fixes the issue:

NV access has purchased a new Authenticode certificate which expires in August 2024.
Due to updated Authenticode policies, this certificate uses a 3072 bit RSA, rather than 2048 bit.
This certificate has been securely encrypted by a secret (itself encrypted via our AppVeyor key). The encoded certificate replaces the older 2018 encoded certificate in this Git repository.
The encryption of the certificate is also significantly stronger
as it uses SHA256 instead of md5, a salt is now used, and a newer secret key derivation algorithm is now used, as recommended by openssl when decrypting the certificate in the past.
A more secure time stamping server is used, again upgraded to SHA256.

Testing strategy:

On Windows 10:

  • Built NVDA locally. Installed and run. Ensured that NVDA could access another app running as administrator.
  • Built NVDA on appVeyor as a try build. Installed and run. Ensured that NVDA could access another app running as administrator.

Known issues with pull request:

  • The try build requires testing on Windows 8.1 and Windows 7. As the certificate has a 3072 bit RSA (per new regulations) I'm not quite sure how well this will go in Windows 7, as new intermediate and root certificates may be required...
  • Currently on Windows 10 I need to disable Reputation-based protection in Windows Security Settings, as this is a new certificate and I'm the only person who has downloaded the file so far, thus Windows is hard-blocking it.

Change log entries:

New features
Changes
Bug fixes
For Developers

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

This certificate is now valid until August 2024, and is 3072 bit instead of 2048 bit.
The certificate as stored in this repository is also now encrypted much stronger, using sha256, pbkdf2 and a salt.
@michaelDCurran michaelDCurran requested a review from a team as a code owner May 11, 2021 08:52
@michaelDCurran michaelDCurran changed the title Update NVDA's Authentocode code signing certificate Update NVDA's Authenticode code signing certificate May 11, 2021
appveyor.yml Outdated
openssl enc -d -md sha256 -aes-256-cbc -pbkdf2 -salt -pass pass:$env:secure_authenticode_pass -in authenticode.pfx.enc -out authenticode.pfx
if($LastExitCode -ne 0) {
$errorCode=$LastExitCode
Add-AppveyorMessage "Enable to decrypt authenticode certificate"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enable -> Unable

feerrenrut
feerrenrut previously approved these changes May 11, 2021
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just fix the enable / unable typo.

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented May 11, 2021

@michaelDCurran I've tested this try build on a fresh Win 7 SP1 vm which has never been connected to the network and this build works correctly i.e can be installed and NVDA can access applications running as admin. I've also tested on Windows 8 and programs running with admin privileges are accessible as well.

@feerrenrut
Copy link
Contributor

Thanks for testing @lukaszgo1. Looks like we should hold off on this until these issues are resolved.

@feerrenrut feerrenrut dismissed their stale review May 12, 2021 05:10

Bugs found running on Windows 8, looks like further changes will be required.

@LeonarddeR
Copy link
Collaborator

this build crashes for me when it tries to exit after successful installation whereas latest Alpha does not.

I also experienced a hard crash of NVDA after an update lately. I wonder whether the exit logic changes could be the cause of the issues you describe, rather than the certificate. Honestly I don't see how a certificate change can cause this kind of changes.

@michaelDCurran
Copy link
Member Author

I tend to agree with Leonard here re the crash very likely being not related. However, I will delay merging this to master until after first beta of NVDA 2021.1 comes out, as there is no need for the certificate to be in the release, as long as it is made before July 26.

@AppVeyorBot
Copy link

See test results for failed build of commit 228bdbf2ad

@feerrenrut
Copy link
Contributor

certificate to be in the release, as long as it is made before July 26.

Will the signed release continue to be valid after July 26?

@michaelDCurran
Copy link
Member Author

michaelDCurran commented May 13, 2021 via email

@lukaszgo1

This comment has been minimized.

@LeonarddeR
Copy link
Collaborator

In that case, I think it makes sense to have a merge asap.

@michaelDCurran michaelDCurran merged commit 0a6806a into master May 18, 2021
@michaelDCurran michaelDCurran deleted the authenticode2021 branch May 18, 2021 02:36
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone May 18, 2021
@michaelDCurran michaelDCurran modified the milestones: 2021.1, 2021.2 May 18, 2021
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.

None yet

6 participants