Add test for "View Saved Passwords" hang when any line of password-related entries is absent

NEW
Unassigned

Status

()

Core
Security: PSM
P5
critical
4 years ago
4 months ago

People

(Reporter: User Dderss, Unassigned)

Tracking

({64bit, crash})

29 Branch
x86_64
Windows 7
64bit, crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [psm-backlog], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 8351370 [details]
FF29a1 View Saved Passwords freeze.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20131224030203

Steps to reproduce:

At any point from anywhere open "Page Info", then switch to "Security" tab, then click on "View Saved Passwords".


Actual results:

FF 29a1 has freezed/hung, and Windows killed FF ("Debug or close" standard dialogue after solution of this hung was not found) -- without Mozilla Crash Reporter registering anything. I have tried it at least thrice always with the same result.


Expected results:

Window with saved passwords should have been opened.
(Reporter)

Updated

4 years ago
Hardware: x86 → x86_64
(Reporter)

Comment 1

4 years ago
How can I contact FF 29a1 (64-bit) developers to fix the issue?

There is no way to view password list right now (see the attached screenshot of freeze), it has to be fixed somehow.
Also WFM.
(Reporter)

Comment 3

4 years ago
What does WFM mean?
Works for me, can't reproduce.
(Reporter)

Comment 5

4 years ago
I now was able to catch the crash with Mozilla Crash Reporter:

http://crash-stats.mozilla.com/report/index/e5f751d6-0b70-43e6-a3c1-096b82131228
Crash Signature: @ nsSecretDecoderRing::decode(char const*, unsigned char**, int*)
(Reporter)

Comment 6

4 years ago
The fact that you can not reproduce this crash probably means that it depends on my concrete exemplar of "signons.sqlite". There might be a value that, when processes, exposes a coding error. Can we detect from the dump in the crash report at least the index number of the string in this database file where FF faces such value?
Have you been able to reproduce in safe mode? Also, how do we know this is a Win64 issue specifically?
Component: Untriaged → Security: PSM
Keywords: crash
Product: Firefox → Core
(Reporter)

Comment 8

4 years ago
The crash was captures in safe mode (see the report, it shows no add-ons or extensions). FF 28 32-bit (aurora) and other versions do not hung this dialogue. But, to be sure, I will install FF 29v1 32-bit to see if this bug is FF 29-specific or 64-bit specific. Thanks for the hint, Ryan.
(Reporter)

Comment 9

4 years ago
I checked it: fully updated FF 29a1 32-bit works with password list dialogue without issue. This means that this bug is 64-bit only, indeed.
Given that this is difficult to reproduce, you are probably in the best position to track down when this regressed. Can you please try the nightly builds from the other bug and see if you can figure it when it broke?
(Reporter)

Comment 11

4 years ago
I browsed back testing to when version 27 had 64-bit build, and this freeze/hung was still there. I could not find earlier 64-bit builds.

Could you please check if something could be derived from the dump file/crash report as I suggest in comment #6?
That requires knowledge and expertise that I don't have. Getting this in the right component should help get it noticed by someone who does, however.
(Reporter)

Updated

4 years ago
Blocks: 953056
(Reporter)

Comment 13

4 years ago
https://crash-analysis.mozilla.com/hang-reports/2013/12-28/hr-20131228-1157d713-a5f0-4203-ac5c-ef031aa871ca.html

Signature: hang | RtlpWaitOnCriticalSection | RtlInitializeCriticalSectionAndSpinCount | CreateMutexExW
Can't reproduce on Nightly x64 29.0a1 (2014-01-08), Win 7 x64.
Try on a new, empty profile:
http://support.mozilla.org/en-US/kb/Managing-profiles#w_starting-the-profile-manager
Flags: needinfo?(zxspectrum3579)
(Reporter)

Comment 15

4 years ago
Yes, I can reproduce the crash; here is crash from today:
http://crash-stats.mozilla.com/report/index/5531ec66-83c2-426e-ae88-cf3f82140108

As I wrote, this crash depends on specific exemplar of "signons.sqlite" file I have. Apparently no one can reproduce the crash without having my logon database, nor there is crash if the DB is empty (as in a clean profile), nor there is crash in 32-bit build.

We need someone with expertise to look at memory dump in crash report and find exact point/function in which crash happens.

Could you please ask someone to look at crash report and memory dump?
Flags: needinfo?(zxspectrum3579)
(In reply to User Dderss from comment #15)
> Could you please ask someone to look at crash report and memory dump?
Ryan, any idea who's best to take a look here ?
Flags: needinfo?(ryanvm)
Maybe Brian Smith?
Flags: needinfo?(ryanvm)
(Reporter)

Comment 18

4 years ago
From what I see, in both bugs (that I filed and) that are in essence the same thing, the signature leads to SSL decryption module that attempts to call a decode function from either in "nsSecretDecoderRing" (http://crash-stats.mozilla.com/report/index/5531ec66-83c2-426e-ae88-cf3f82140108) or "nsCMSSecureMessage" (http://crash-stats.mozilla.com/report/index/9d14b989-f946-4b08-9e31-c04822131224) -- "decode(char const *,unsigned char * *,int *)". But what exactly and why goes wrong in 64-bit build during this decode, I do not understand.
Brian, can you take a look please ?
Flags: needinfo?(brian)
(In reply to User Dderss from comment #18)
> From what I see, in both bugs (that I filed and) that are in essence the
> same thing, the signature leads to SSL decryption module that attempts to
> call a decode function from either in "nsSecretDecoderRing"
> (http://crash-stats.mozilla.com/report/index/5531ec66-83c2-426e-ae88-
> cf3f82140108) or "nsCMSSecureMessage"
> (http://crash-stats.mozilla.com/report/index/9d14b989-f946-4b08-9e31-
> c04822131224) -- "decode(char const *,unsigned char * *,int *)". But what
> exactly and why goes wrong in 64-bit build during this decode, I do not
> understand.

64-bit Windows? I know of at least one other bug where we don't do the correct thing in PSM and/or NSS on 64-bit Windows. Are 64-bit Windows builds supported now? If not, this is one reason why.
Flags: needinfo?(brian)
(Reporter)

Comment 21

4 years ago
Could anyone please confirm if at any point "decode(char const *,unsigned char * *,int *)" uses any platform/OS-specific system calls rather than direct platform-independent program code?

If it is the former case, then this is Win-64 specific issue, but if it is the latter case, then it does not matter if Win-64 is supported or not, because my crashes would mean that any 64-bit build, such as Linux or OX X, would also result in crash with my concrete rare exemplar of "signons.sqlite".

Any SSL programmers here?
(Reporter)

Comment 22

4 years ago
Any updates?
(Reporter)

Comment 23

4 years ago
FireFox 30a1 still absolutely crashes on SSL decode operations. This is critical error, please someone check Comment #21.
(In reply to User Dderss from comment #21)
> This is critical error

I can understand the frustration that this bug may be causing; however, keep in mind though that at this point, it's still entirely possible (and perhaps likely) that this only happens with specially-corrupted "signons.sqlite" files - e.g. due to filesystem corruption, or a sudden shutdown, or a crash at exactly the wrong time.

Ideally Firefox would still be robust in the face of such corruption, but it's not inconceivable that a corrupted sqlite database (formatted in just the "right" way) might cause trouble in programs that are attempting to read it.

Without the file to work with (given the sensitive contents), it may be difficult for folks here to quickly figure out what might be going on, in light of possible file-corruption.

BUT: since you've got access to the file in question, you're in a particularly good position to dig into this -- it might be worth trying to inspect it using the sqlite3 standalone program. (i.e. see if it opens correctly, in particular).

https://www.sqlite.org/sqlite.html has more information about the program. On Ubuntu, I can dump out the contents of my signons.sqlite as ASCII text using the following commands:

  sudo apt-get install sqlite3
  sqlite3 signons.sqlite .dump

...and if I edit the file to intentionally corrupt it, I get "ROLLBACK; -- due to errors" at the end of the output. It'd be useful to know if your file produces any errors like that.
(Reporter)

Comment 25

4 years ago
Thanks for reply.

The possible corruption of "signon.sqlite" files was under consideration, but:
1) the file is fully "openable" and browsable (through any SQL browsers I have tried, including "SQL Manager" extension);
2) the file passes full DB Pragma integrity check, as well as DB analysis without issue;
3) the file works fine under any 32-bit version of FireFox (however, all 64-bit versions of Windows FireFox crash);
4) the error seems to happen at the point where FireFox engine already gets the information from the file just fine, but then tries to decrypt the "encryptedUsername" field. This procedure works without issue in 32-bit builds, but crashed in 64-builds -- all in exact places.

There are two crash reports, both of which lead to crash at the same "decode(char const *,unsigned char * *,int *)" step. There are dumps, so full context is available:

http://crash-stats.mozilla.com/report/index/5531ec66-83c2-426e-ae88-cf3f82140108

http://crash-stats.mozilla.com/report/index/9d14b989-f946-4b08-9e31-c04822131224

Those dumps should be enough to see not only the function at which FF crashes -- which is detected as "signature" -- but also immediate arguments/variables that were processed. This could help a programmer to run a test at hand with the same context arguments/variables to emulate the conditions of this crash and find out why it happens.

Is there anyone who has speciality in dump files?
(In reply to User Dderss from comment #25)
> The possible corruption of "signon.sqlite" files was under consideration,
> but:

OK, those are very useful pieces of information - thanks for that.

> Is there anyone who has speciality in dump files?

I'd suspect briansmith or keeler would be the most likely suspects, for this area of code [regardless of specialty in minidump files]. Tagging Brian as needinfo to make sure it's on someone's radar.

Brian, see comment 25 (and as you likely know, some of the folks in security-group can get the actual minidumps for you, which may be helpful for seeing the actual variables that are causing problems here).
Flags: needinfo?(brian)
Flags: needinfo?(brian)
Whiteboard: [briansmith ETA: 2014-02-24]
Flags: needinfo?(brian)
> DecryptString(const char *crypt, char **_retval)
> {
>   nsNSSShutDownPreventionLock locker;
>
>   [...]
>
>   rv = decode(crypt, &decoded, &decodedLen);
>   if (rv != NS_OK) goto loser;
>
>  [...]

The use of nsNSSShutDownPreventionLock will prevent NSS from shutting down in the middle of the function. However, how does the function know that NSS hasn't already shut down? It needs to call isAlreadyShutDown which means something here needs to implement the nsNSSShutDownObject interface. And/or use the CryptoTask interface, which abstracts a bunch of this away.
Flags: needinfo?(brian)
(In reply to User Dderss from comment #9)
> I checked it: fully updated FF 29a1 32-bit works with password list dialogue
> without issue. This means that this bug is 64-bit only, indeed.

64-bit Windows is not a supported platform now. I believe there are bugs in NSS on 64-bit Windows.

Anyway, looking at it more, it isn't anything to do with NSS shutdown, because this ins't happening at shutdown. Also, this isn't happening during any NSS call--it's simply an XPCOM call to a function DecryptString with a string input parameter, and then this decode function tries to base-64-decode it. No crypto at all, up to this point, if the stack traces are correct.

Anyway, I can't spend too much time on this since 64-bit Windows is not a supported platform, AFAICT.
Whiteboard: [briansmith ETA: 2014-02-24]
(Reporter)

Comment 29

4 years ago
All right, Brian. You do not have to spend much time any more because I found what causes the crashes. Signons database can be in perfect integrity and everything, but if one of the encrypted fields (encryptedUserName or encryptedPassword) in any line of password-related entries is absent, somehow FF engine can not survive it. It tries to decode zero-length value and it kills it. Strangely enough, this never happens in 32-bit FireFox, it can perfectly survive that one of the encrypted fields could be empty. Just insert a line or few to check if the field is empty or not, and everything is going to be fine. Thanks in advance.
Duplicate of this bug: 953056

Comment 31

4 years ago
Dderss, fyi crash signature field in bugzilla needs format of [@ signature ]
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: @ nsSecretDecoderRing::decode(char const*, unsigned char**, int*) → [@ nsSecretDecoderRing::decode(char const*, unsigned char**, int*) ]
Ever confirmed: true
Keywords: 64bit
Summary: "View Saved Passwords" freezes/hungs FF 29a1 → "View Saved Passwords" freezes/hungs FF 29a1 if any line of password-related entries is absent
This difference in negative array index handling on 64 bit looks like to be the cause why we're crashing in x64 and not in x32:

http://www.devx.com/tips/Tip/41349

Anyway, as dders has said, we should just make input validation and not even try to access a negative index.

Updated

2 years ago
Crash Signature: [@ nsSecretDecoderRing::decode(char const*, unsigned char**, int*) ] → [@ nsSecretDecoderRing::decode(char const*, unsigned char**, int*) ] [@ nsSecretDecoderRing::decode ]
Whiteboard: [psm-backlog]
David, you marked this bug for psm-backlog. Is this bug still an issue? Should this bug block making 64-bit Firefox the default version for Win64 OS users?
Flags: needinfo?(dkeeler)
I believe this was fixed by bug 1275309, but we could add a test to verify. I don't think this should block making 64-bit Firefox the default on Windows (but maybe bug 1275309 should, depending on what version we're hoping to make the switch in).
Flags: needinfo?(dkeeler)
No longer blocks: 558448
Depends on: 1275309
Summary: "View Saved Passwords" freezes/hungs FF 29a1 if any line of password-related entries is absent → Add test for "View Saved Passwords" hang when any line of password-related entries is absent
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.