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

Status: RESOLVED WONTFIX

(Core :: Security: PSM, defect, P5)

Description

[User Dderss]

Attachment: 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.

Comment 1

[User Dderss]

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.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Also WFM.

Comment 3

[User Dderss]

What does WFM mean?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Works for me, can't reproduce.

Comment 5

[User Dderss]

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

http://crash-stats.mozilla.com/report/index/e5f751d6-0b70-43e6-a3c1-096b82131228

Comment 6

[User Dderss]

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?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Have you been able to reproduce in safe mode? Also, how do we know this is a Win64 issue specifically?

Comment 8

[User Dderss]

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.

Comment 9

[User Dderss]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 11

[User Dderss]

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?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 13

[User Dderss]

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

Signature: hang | RtlpWaitOnCriticalSection | RtlInitializeCriticalSectionAndSpinCount | CreateMutexExW

Comment 14

[Paul Silaghi, QA [:pauly]]

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

Comment 15

[User Dderss]

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?

Comment 16

[Paul Silaghi, QA [:pauly]]

(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 ?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Maybe Brian Smith?

Comment 18

[User Dderss]

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.

Comment 19

[Paul Silaghi, QA [:pauly]]

Brian, can you take a look please ?

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 21

[User Dderss]

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?

Comment 22

[User Dderss]

Any updates?

Comment 23

[User Dderss]

FireFox 30a1 still absolutely crashes on SSL decode operations. This is critical error, please someone check Comment #21.

Comment 24

[Daniel Holbert [:dholbert]]

(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.

Comment 25

[User Dderss]

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?

Comment 26

[Daniel Holbert [:dholbert]]

(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).

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

> 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.

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 29

[User Dderss]

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.

Comment 31

[Wayne Mery (:wsmwk)]

Dderss, fyi crash signature field in bugzilla needs format of [@ signature ]

Comment 32

[Johannes Pfrang [:johnp]]

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.

Comment 33

[Chris Peterson [:cpeterson]]

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?

Comment 34

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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).

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.