bitwarden / desktop

The desktop vault (Windows, macOS, & Linux).
https://bitwarden.com
GNU General Public License v3.0
3.55k stars 401 forks source link

Bitwarden v1.20.0 or later crashes on Windows 7 #507

Closed bchavez closed 4 years ago

bchavez commented 4 years ago

Describe the Bug

A recent update hosed my installation of Bitwarden desktop for windows.

image

Attaching an instance of the VS 2019 debugger, slightly more details about the error:

Unhandled exception at 0x00000000773E889D (ntdll.dll) in Bitwarden.exe: 0xC0000005: Access violation writing location 0x0000000000090C58. occurred

Steps To Reproduce

  1. On Windows 7, go to start menu.
  2. Click on "Bitwarden" image

Expected Result

The program should start with the login screen.

Actual Result

image

Screenshots or Videos

Environment

Additional Context

Took a memory dump, ran !analyze -v in WinDbg: (trying to be careful not to share PII)

0:000> !analyze -v
*******************************************************************************
*                                                                             *
*                        Exception Analysis                                   *
*                                                                             *
*******************************************************************************
.....
.....
FAULTING_THREAD:  000024cc
EXCEPTION_CODE: (HRESULT) 0x80000003 (2147483651) - One or more arguments are invalid
FAILURE_EXCEPTION_CODE:  80000003

FAILURE_IMAGE_NAME:  ntdll.dll

BUCKET_ID_IMAGE_STR:  ntdll.dll

FAILURE_MODULE_NAME:  ntdll

BUCKET_ID_MODULE_STR:  ntdll

FAILURE_FUNCTION_NAME:  RtlRaiseStatus

BUCKET_ID_FUNCTION_STR:  RtlRaiseStatus

I'll try to debug this some more after work tomorrow.

cscharf commented 4 years ago

@bchavez , are you running in the AMD64 x86 space or purely 32-bit x86? Also, seeing as that looks like a pointer exception any additional information you're able to find through the debugger would be helpful (thank you for that).

If you would feel more comfortable sharing information privately with me directly my email is cscharf at bitwarden dot com.

bchavez commented 4 years ago

Hi Chad,

Thanks for the details. I'll try working on a memory dump I can share. I did some tests and narrowed it down:

I believe I'm using AMD64, 64-bit processes:

procexp64_3966

taskmgr_3967

bchavez commented 4 years ago

So, I did a diff between 1.19.0 and 1.20.0...

The changes in main.ts stood out to me and seemed suspect are:

https://github.com/bitwarden/desktop/blob/c31de46b9e879b065be39e11ba32ae6ddeebd26a/src/main.ts#L112-L113

It looks like this change was part of #470 - Biometric support.

To test this theory,

  1. Extracted the Bitwarden\resources\app.asar file to Bitwarden\resources\app
  2. Renamed app.asar to app.asarx to avoid electron from picking up the file.
  3. Commented out the following lines in the main.js web packed file in Bitwarden\resources\app:

image

  1. Saved main.js file.
  2. Started Bitwarden and now :heavy_check_mark: 1.20.1 works.

image


Conclusion

I suspect the native library import for biometric support from #470: "@nodert-win10-rs4/windows.security.credentials.ui" is making a native system call to a Windows 10 API that is not available on Windows 7. You should be able to reproduce this crash if you try running Bitwarden v1.20.0 or later on any Windows 7 machine.

Thanks, Brian Chavez

cscharf commented 4 years ago

Thanks @bchavez , you are correct, this module's addition has caused a few issues and crashes, however the issue was resolved for those users with Windows 7 (all) and Windows 10 (who did not have the appropriate VC++ dependencies installed). I suspect perhaps the loading of the module itself is successful on your machine as you're obviously set up for VC++ development, but then the API call itself under the covers is then failing.

If you note the line you commented out simply loads our wrapper/bootstrapper around that nodeRT module. This crash is not reproducible on any other Windows 7 machines we've tried, see: #495. Which leads me to believe it's because on your machine it is successfully loading the module and libraries necessary (which under normal circumstances would be missing) however the API calls are failing.

Is the crash itself when loading the module or when it's attempting to be used/called?

see: https://github.com/bitwarden/jslib/blob/master/src/electron/biometric.windows.main.ts#L52 (initialized/loaded) see: https://github.com/bitwarden/jslib/blob/master/src/electron/biometric.windows.main.ts#L65 (called right after initialization/loading of the module to determine whether it will work on the given machine)

bchavez commented 4 years ago

Hi Chad,

I reverted my changes and tested the lines you indicated above.

Reverting all changes, the target locations were found in the web packed file:

Notepad2_3974


Commenting out either line as you've pointed out will fix the problem too:

:heavy_check_mark: Works. No crash. Removing getWindowsSecurityCredentialsUiModule

Notepad2_3972


:heavy_check_mark: Works. No crash. Removing checkAvailabilityAsync

Notepad2_3973


Keeping them both methods intact will cause a crash; however. This test was on:

Version 1.20.1
Shell 6.1.7
Renderer 76.0.3809.146
Node 12.4.0
Architecture x64

Thanks, Brian

cscharf commented 4 years ago

Perfect, the PR submitted attached to this issue should resolve this for you once released.

bchavez commented 4 years ago

@cscharf Got a notification for a Bitwarden 1.21 update.

Updated Bitwarden and Bitwarden still crashes in 1.21:

image

Extracted the ASAR file, commented out the following lines in main.js:

image

And it works again:

image

This is still an issue.

cscharf commented 4 years ago

@bchavez , if you wrap those 2 lines in a try {} catch {} instead, does that also prevent the crash?

try {
    const BiometricWindowsMain = __webpack_require__(17).default;
    this.biometricMain = new BiometricWindowsMain(this.storageService, this.i18nService);
} catch { }
bchavez commented 4 years ago

Hi @cscharf, the try{} catch{} doesn't prevent the crash. I think the fact that a thread makes any attempt at an undefined native API causes Bitwarden to crash:

image

I suspect a proper fix would look something like:

image

It's almost as if the native exception preempts the JS runtime.

cscharf commented 4 years ago

@bchavez , I've merged into jslib and desktop what I believe will resolve this, thanks for the working models and psuedo-code on suggested fix. I believe now, unless you're working with Windows 10, we'll not even attempt to load the biometrics support library (nodeRT) which should prevent the crash. Until that gets released with the next version you should be able to keep your modified version with those 2 lines commented out.

bchavez commented 4 years ago

Great news, recently updated to:

Version 1.22.2
Shell 6.1.7
Renderer 76.0.3809.146
Node 12.4.0
Architecture x64

and the issue appears to be resolved on win7 now. Thanks a bunch!