failing on undecryptable password entries

[rma-x]

Today I used firefox_decrypt on a profile that I have carried on over many years and Firefox versions and that has grown a long list of stored passwords. Apparently for one of my entries both username and password are corrupted for whatever reason, and that makes firefox_decrypt fail completely:

2023-07-26 16:21:00,168 - ERROR - Username/Password decryption failed. Credentials damaged or cert/key file mismatch.

Given that the rest of the passwords can be decrypted just fine (as I was able to see with -vv), I think that the decrypt method of the NSSProxy class should only print a warning (or the number of failures in a summary line), but not error out on entries that fail to decrypt.

[rma-x]

Or how about doing it this way?

        try:
            if err_status:  # -1 means password failed, other status are unknown
                res = "*** decryption failed ***"
            else:
                res = out.decode_data()

resulting in

Website:   http://example.com
Username: '*** decryption failed ***'
Password: '*** decryption failed ***'

[unode]

@all-contributors please add @rma-x for ideas

[allcontributors]

@unode

We had trouble processing your request. Please try again later.

[unode]

The newly released 1.1.0 should now cover this. See also --non-fatal-decryption

[rma-x]

Thanks for the quick response and fix!

I confirm that --non-fatal-decryption produces the expected output on stdout, but it also produces a traceback on stderr, which I think shouldn't occur when passing an option that is meant to tolerate exactly this kind of error.

IOW, I would expect to see all the WARNING lines and maybe the ERROR line (possibly reduced to a WARNING when --non-fatal-decryption is used), but not the traceback, because that makes it look like a bug was triggered rather than an expected failure situation being handled.

$ ./firefox_decrypt.py --non-fatal-decryption . >/dev/null 
2023-07-27 08:38:06,809 - WARNING - profile.ini not found in .
2023-07-27 08:38:06,811 - WARNING - Continuing and assuming '.' is a profile location

Primary Password for profile .: 
2023-07-27 08:38:10,879 - WARNING - Failed to decode username or password for entry from URL [...]
2023-07-27 08:38:10,880 - ERROR - Username/Password decryption failed. Credentials damaged or cert/key file mismatch.
Traceback (most recent call last):
  File "/[...]/firefox_decrypt.py", line 578, in decrypt_passwords
    user = self.proxy.decrypt(user)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/[...]/firefox_decrypt.py", line 524, in decrypt
    raise ValueError(error_msg)
ValueError: Username/Password decryption failed. Credentials damaged or cert/key file mismatch.

[unode]

The logged exception was kept to indicate which failure occurred (username or password).
I agree that its not the ideal user interface but the stderr redirect section was added precisely to help with this.

[rma-x]

The logged exception was kept to indicate which failure occurred (username or password).

This could be dealt with by (error)handling both values individually which would have the additional benefit of not invalidating both when only one of them is corrupted.

--- firefox_decrypt.py.orig
+++ firefox_decrypt.py
@@ -576,15 +576,20 @@
                 try:
                     LOG.debug("Decrypting username data '%s'", user)
                     user = self.proxy.decrypt(user)
+                except (TypeError, ValueError) as e:
+                    LOG.warning(
+                        "Failed to decode username for %s",
+                        url,
+                    )                    
+                    user = "*** decryption failed ***"
+                try:
                     LOG.debug("Decrypting password data '%s'", passw)
                     passw = self.proxy.decrypt(passw)
                 except (TypeError, ValueError) as e:
                     LOG.warning(
-                        "Failed to decode username or password for entry from URL %s",
+                        "Failed to decode password for %s",
                         url,
                     )
-                    LOG.exception(e)
-                    user = "*** decryption failed ***"
                     passw = "*** decryption failed ***"
 
             LOG.debug(

resulting in a (IMO) much nicer output with no backtraces:

$ ./firefox_decrypt.py --non-fatal-decryption . > /dev/null 
[...]
2023-07-27 17:22:12,874 - WARNING - Failed to decode username for http://example.com
2023-07-27 17:22:12,879 - WARNING - Failed to decode password for http://example.com

$ ./firefox_decrypt.py . > /dev/null 
[...]
2023-07-27 17:27:16,623 - ERROR - Username/Password decryption failed. Credentials damaged or cert/key file mismatch.

$ ./firefox_decrypt.py -vv . > /dev/null
[...]
2023-07-27 17:28:04,179 - DEBUG - Decrypting username data '[...]'
2023-07-27 17:28:04,183 - DEBUG - Decryption of data returned -1
2023-07-27 17:28:04,183 - ERROR - Username/Password decryption failed. Credentials damaged or cert/key file mismatch.
2023-07-27 17:28:04,184 - DEBUG - PR_LOAD_LIBRARY_ERROR: Failure to load dynamic library

[unode]

Would you be so kind to submit a pull request?

Also, please keep the LOG.exception behavior but replace that line by LOG.debug(e, exc_info=True) so the traceback is only shown when verbosity is increased to DEBUG level, and add a line describing the change to the CHANGELOG file.
I will likely make a minor release to include this afterwards.

[unode]

Actually, I think the change I propose addresses most of your suggestion, which is to not show the traceback.
I'll include that in a second.

[unode]

Done, let me know if this is good enough.

[rma-x]

Thanks, works for me.

It still invalidates both fields even when only one is corrupted, but that doesn't matter for my case at hand where both are corrupted anyway.

[unode]

It still invalidates both fields even when only one is corrupted, but that doesn't matter for my case at hand where both are corrupted anyway.

Indeed, but considering that a feature as one can't login without both pieces of information 😇

[rma-x]

Well, if only the username was corrupted, that I would have remembered anyway, I'd still be interested to get the password out. ;)