507937 - pwdecrypt program problems

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch ignoring whitespace

NSS's utility command program pwdecrypt is a diagnostic tool occasionally 
used by NSS developers.  It is not built as part of normal NSS builds. 
It is supposed to read in a file of encrypted web site passwords, in the 
format produced by Mozilla's "wallet" component, and output the same file
but with all encrypted lines decrypted, thereby revealing encrypted passwords 
and user names.  Of course, to do this, it needs the user's key3.db file,
for which the user must have the "master password".  

There are several problems with this program.
1. It's parsing of Base64 encoded encrypted data is faulty.  If it finds 
the letter 'M' anywhere in any line, it assumes that is the first character
of a base64 encoded SDR encrypted site password.  It should only make that
check at the beginning of a line of input.  There are also other problems
with the Base64 decoding.

2. When it outputs the line containing the decrypted site passwords, it does
not put a trailing \n on that line.  This causes the following line to be
appended to the decrypted password.  The following line is invariably a single
"dot" ('.') character.  

3. The file's indentation is very inconsistent, using variously an indentation quantum of 2, 3, or 4.  

I've fixed all these things.  I will attach a patch containing the fix,
made ignoring whitespace.  The indentation will look terrible in this patch,
but the patch will show no whitespace-only changes.

Since this program is not built in normal NSS builds, I will not seek review.

After completing these bug fixes, I will work on an enhancement to this 
program.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Checking in pwdecrypt.c; new revision: 1.6; previous revision: 1.5

Comment 2

[Robert Relyea]

Arg, why didn't I review this....

I just went to use pwdecrypt and found it no longer functioned as designed. Many apps (including mozilla) imbed the Base64 data in the middle of a text file. Issue 1 is really an important feature, and now it's broken. It should still be turned on at least with an option. 

The output is based on the input file. It's supposed to take a file full of mixed plaintext and encrypted passwords and convert them into a file of plaintext and decrypted passwords.

It no longer does this.

I don't mind having an option that makes it more friendly for the current wallet, but it should still be able to handle it's old stuff as well.

bob

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Bob, there has never been any written specification for this program.
The current behavior is what you describe.  Output based on input. 
input mixed plaintext and encrypted passwords, 
output mixed plaintext and decrypted passwords.
The difference is that the new code only interprets a string as an 
encrypted password if it has an 'M' in the first column on the line.
The old behavior, where it interpreted any 'M' (capital M) character 
appearing anywhere in any line as being the beginning of an encrypted 
password caused all sorts of problems for parsing wallet files.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

... because the 'M' character sometimes appears in host names.

Comment 5

[Robert Relyea]

The old wallet format included atob strings that could start in the middle of the line.

The new program also seems to add 'decrypted='.

I can see the need to 'allow the option' to force atob strings to start in the first line, but we still need the ability to have pwdecrypt find atob strings in the middle.

Also, we only output decrypted data not only if the result looks like valid atob data, but also if that atob data is a valid SDR value, and decrypts correctly... is all three occuring in these cases?

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)
> The old wallet format included atob strings that could start in the middle of
> the line.

Perhaps, but I never saw such a file.

> The new program also seems to add 'decrypted='.

Feel free to strike that.  

> I can see the need to 'allow the option' to force atob strings to start in the
> first line, but we still need the ability to have pwdecrypt find atob strings
> in the middle.

I may be wrong, but I believe that this program's primary objective was always
decrypting wallet files.  In the last year, for some reason, many users began
to ask for a program to do just that, and I sent them this program, and they
reported that it failed.  To debug it, some of them sent me their wallet files
(but not their key DBs) and I began to debug it.  Immediately, I saw that it 
was having trouble because it was stumbling over 'M' characters in the middle
of lines that did not contain encrypted passwords.  It appeared to me that 
the wallet format always put encrypted passwords at the beginning of a line.  So, I changed the program to expect that.  Then the program was able to parse all those wallet files correctly.  I'd like to see a wallet file that doesn't
follow that format, and know wht program produces it.

> Also, we only output decrypted data not only if the result looks like valid
> atob data, but also if that atob data is a valid SDR value, and decrypts
> correctly... is all three occuring in these cases?

I think you're saying that you expect this program to output decrypted data
that are not SDR data.  AFAIK, wallet files do not contain any such lines.
I have never seen a wallet file that did.  That is, in all the wallet files
I have ever seen, the only encrypted data was SDR datga.  
So, I concluded that you are using this for purposes other than wallet files.  

As I said, I've never seen any written specification for this program, and 
the only use for it I've ever known is for wallet files, whose format is as 
I described above.  If it has another purpose, there should be some specification for it somewhere.  

I have no objection to the program have multiple options, and being for multiple purposes, as long as they're specified and documented.  
Also, we generally do not require review for changes to files that are not 
regularly built in nightly builds (IOW, built by default). So, if you'd 
like to review changes to this file, then either we should build it by 
default, or else put a note in the file somewhere asking programmers to 
ask you to review patches.

> bob

Comment 7

[Robert Relyea]

Wallet programs tend to mix plain text with SDR data, sometimes on different lines, sometimes on the same line. The original mozilla wallet format had lines like 'url.password=Maaaaakfdjsalkfjasdjf" stored in a .js. The latest Firefox wallet stores things in sqlite databases, which can be dumped with sqlite3 signons.sqlite | pwdecrypt -d . -p {passwd}
.dump
.quit
The old version of pwdecrypt handles this correctly, the new version does not.

Mozilla, alone, changes it's format periodically (yet alone any other users. The program also worked on old Photo stored passwords). That's why I went with a simple replace in place. Basically the program finds SDR data, and if it can successfully decrypt it, replaces the SDR data with the decrypted results, it it was unsuccessful, is simply outputted the string. This allows it to work with a variety of wallet formats. 

Evidently you tripped over some cases where the random 'M' data was successfully interpreted as a valid SDR key and decrypted normally (this seems quite surprising to me, how did we get a valid Key ID?) I would like to see the wallet files that were causing problems.

> So, if you'd 
> like to review changes to this file, then either we should build it by 
> default, or else put a note in the file somewhere asking programmers to 
> ask you to review patches.

OK, I'll do that. I originally checked it in so it wouldn't disappear. I recently had a use for it today, and was surprised when I found the semantic had totally changed.

I'd like to revert to the original semantic (the original program worked very hard to implement that semantic, I'm surprised you thought it was accidental rather than design), but I also don't want to loose any other fixes you had made. I can add an option to restrict attempts to parse SDR blocks unless they begin the line.

I did find one of my files that outputted garbage from the SDR data, (it's actually valid SDR data, the key is just no longer available, but it decrypts to a meaningful pad, so the decrypt succeeds). Adding a check for valid utf8 strings successfully removed this issue (though, that too should probably be turned off on the fly in case mozilla records code page data).

Sorry about the mix-up, I was quite surprised that a feature I had carefully crafted suddenly disappeared;).

bob

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Bob, I still run a version of SM that uses the original wallet format files.
Here's a sample of the file contents (SDR data lines are truncated):

intranet.mozilla.org:443 (Mozilla Corporation - LDAP Login)
\=username=\
MEIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcN
*\=password=\
MEIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcN
.
https://www.linkedin.com
session_key
MEIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcN
*session_password
MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcN
.

The format seems to be 
line 1: URL basics, and for basic auth, some basic auth text
line 2: field name in form
line 3: SDR data for field in form (lines 2 and 3 are a pair)
Additional pairs of lines for a field may be repeated for as many fields
as need to be recorded.
line 4+2n: "."

This pattern is repeated for each host. So, what I see is, each line is 
either entirely unencrypted, or else entirely SDR data.
Does the data extracted from the sqlite3 file have that format?

I don't remember most of the specifics of the original input file that caused the problem now.  I only remember that the file in question had an 'M' in a 
line that was obviously not a line of encrypted SDR data, and the data following the M was obviously not SDR data, and the program just failed at 
that point.  I didn't think it right to attach the file to this bug, even 
though the passwords were encrypted.  I'm pretty sure i did not keep any 
copy of that file, but I'll double check.

Comment 9

[Robert Relyea]

The format was before that. The wallet was stored in a .js program.

I really only need the one line that failed. Also, what do you mean by 'failed', did the program crash? (That might actually turn up a bug in the SDR code). The decode is expected to fail once in a while, so it just outputs the 'encrypted block' (which often is not SDR data). I knew the program would sweep up non-SDR blocks based on the algorithm, but it shouldn't have mattered as those blocks should fail to decrypt and just be outputted transparently.

bob

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Bob, 
If you want to back out the fix for this bug, I won't be offended.
I did this work over a year ago.  I then carried the fix in my tree for 
about a year, and finally checked it in so that I would not have to keep
carrying it in my work area.  I cannot reproduce the problem with my own 
personal signons.txt files.

I have long ago deleted the files with which the problem was originally reproduced.  I no longer recall the details of the original problem.  
I am pretty sure that I did not perceive it to be a flaw in SDR, but rather 
in the pwdecrypt program. 

You reported that the output of the command 
  sqlite3 signons.sqlite | pwdecrypt -d . -p {passwd}
could not be parsed by the current program.  This implies that the output 
of that sqlite3 program is not in the format I described in comment 8.  
Please provide a description of the output of that command (perhaps attach a 
file of sample output), so that I can know how the format has changed yet 
again.

You observed that the current program outputs "decrypted=".  That is true.
More completely, it outputs "decrypted= "%s"\n".
The important thing there is not the "decrypted=" part but rather is the 
quotes around the decrypted part.  Without those quotes, one cannot determine
that the output is 
- an empty string
- a string with one or more leading and/or trailing blanks.
The quotes help a lot with that.
An option to delimit the decrypted output, to facilitate the recognition of 
the boundaries of the decrypted string(s), seems like a very good idea, but
could/should be an option.