428421 - FIPS mode: Master password dialog truncates security device name

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Joe Vornehm Jr.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/20080311 Firefox/2.0.0.13
Build Identifier: 2008041005

When FIPS mode is enabled in Firefox or Thunderbird and the user is prompted for the master password, the security device name is truncated: "Please enter the master password for the FIPS 140 Cryptographic, Key and."  I think I saw the same truncation in the Security Devices dialog (where the Enable FIPS button lives).

I might have a fix.  WARNING: I know nothing about NSS; this is just what I found by poking around LXR.

mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties line 71 seems to indicate that Fips140TokenDescription gets 32 bytes and Fips140SlotDescription gets 64.  The slot description is what's used in the master password dialog box, but it's getting truncated at 32 characters (or possibly 31).

The method nsNSSComponent::ConfigureInternalPKCS11Token() (mozilla/security/manager/ssl/src/nsNSSComponent.cpp line 857) loads those two strings and passes them to PK11_ConfigurePKCS11() as arguments 7 and 8 (fslotdes and fpslotdes), respectively.

PK11_ConfigurePKCS11() then transposes them (mozilla/security/nss/lib/nss/nssinit.c line 202), setting "FIPSSlotDescription='<fslotdes>' FIPSTokenDescription='<fpslotdes>'" in pk11_config_strings.  I think that's where the problem occurs.

So it would seem to the untrained eye (i.e. my eye) that fixing the problem is as easy as swapping lines 203 and 211 in mozilla/security/nss/lib/nss/nssinit.c.  However, I'm not sure I've understood the code correctly, and there's always the chance that the fix would have unintended consequences.


Reproducible: Always

Steps to Reproduce:
Using Firefox:
1. Enable FIPS mode (Tools > Options > Advanced > Encryption > Security Devices > Enable FIPS), ensure a master password is set (Tools > Options > Security > Use a master password), and restart Firefox.
2. Do something to require Firefox to ask for the master password.

Actual Results:  
Prompt reads, "Please enter the master password for the FIPS 140 Cryptographic, Key and."

Expected Results:  
Prompt should read, "Please enter the master password for the FIPS 140 Cryptographic, Key and Certificate Services."

Happens in Firefox 3b5 (build ID 2008031114) and also in the latest nightly I downloaded (2008041005) with a fresh profile.  Similar behavior occurs in Thunderbird 2.0.0.12 (build ID 20080213).

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

PKCS11 defines numerous string types.  In each case, the strings are 
fixed length arrays of bytes, not NUL terminated, blank padded.  
That is, all the bytes in the array contain characters.

NSS's APIs allow callers to pass NUL-terminated strings.  NSS then 
converts them to the PKCS#11 standard form by adding blank padding, 
or by truncating, as necessary.  

Among the strings of these forms are:
module:
    libraryDescription[32]
Slot:
    slotDescription[64]        This is (obviously) the slot description
    manufacturerID[32]
Token:
    label[32]                  This is the token name or token description
    manufacturerID[32]
    model[16]
    serialNumber[16]

NSS provides function PK11_ConfigurePKCS11 by which applications can set 
these names for the pure-software virtual slots and virtual tokens 
implemented in NSS.  

Sadly there is no documentation for that function, and the names of the 
function parameters ("fslotdes" and "fpslotdes") offer no meaningful 
clues about which which PKCS#11 strings are intended to correspond to them.
The code at lines 203 and 211 (cited above) make it pretty clear that 
 fslotdes is intended to be the slot description and 
fpslotdes is intended to be the token description.
Elsewhere, these string variables are named fslotdes and ftokdes, which is 
less ambiguous.

The cited code hasn't changed since it was written in 2001.  
But we see that the caller of this function in PSM was changed in February 
2007, and that change switched the order of those two arguments.  See 
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSComponent.cpp&rev=1.164#884>
and 
<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/manager/ssl/src&command=DIFF_FRAMESET&file=nsNSSComponent.cpp&rev2=1.146&rev1=1.145>

So, I conclude that the truncation problem being reported here is not the 
fault of NSS, but of its caller.  Consequently, I am changing this to be a 
PSM bug.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The PSM change in February 2007, to which I referred in comment 1, was 
made on the trunk, but this bug is about the 1.8 branch.  So, maybe the
fix is to backport that trunk change to the branch?  

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Joe, Let me suggest that you try the latest Firefox 3 Beta build (or release
candidate build) and see if the problem persists there.  If not, then this
gives us a very good idea about the fix.  

Comment 4

[Joe Vornehm Jr.]

Thanks for your investigation and explanation!  It still happens in Firefox 3b5 and in the latest nightly I tried (2008041005).  I agree with your assessment and that it sounds like a PSM issue (though my opinion shouldn't count for much).

This may be a duplicate of bug 317630, which is closed, but applies to the trunk, not 1.8.  I'll let someone else judge whether this should be marked as a duplicate bug.

(I think "fslotdes" is "FIPS slot description", and "fpslotdes" is "FIPS private slot description," but I don't know what either of those is.)

Comment 5

[Lars P M]

Cp. bug 426051 for a FIPS label/description related regression on trunk.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Lars.  Bug 426051 says:
> This regressed between the builds from
> 2008-03-16-05 and 2008-03-17-05.
> <http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2008-03-16+04&maxdate=2008-03-17+06>

Among the checkins shown on that page are:

Bug 420151, FF3Beta5 should use updated NSS tag NSS_3_12_BETA3
Also updating NSPR tag to NSPR_4_7_1_BETA2
r=rrelyea, r=wtc, blocking1.9=dsicore

Bug 406755, EV certs not recognized as EV with some cross-certification scenarios
r=rrelyea, blocking1.9=dsicore 

Comment 8

[u60234]

I believe this is a trunk-only regression. The similar behaviour the reporter saw in Thunderbird 2.0.0.12 is most likely bug 317630, which was never fixed on 1.8 branch. So it show the truncated label "PSM Internal FIPS-140-1 Cryptogr".

This new bug on trunk is that the description string "FIPS 140 Cryptographic, Key and Certificate Services" is used as a truncated label, and the label "Software Security Device (FIPS)" is used as description.

I see this also on Linux, so changing Hardware/OS to All/All.

Comment 9

[u60234]

Sorry, I didn't mean to change the severity or status. Setting back to "Normal" and "New".

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Bob Relyea, Can you think of any change made to NSS between the tags of 
NSS_3_12_BETA2 and NSS_3_12_BETA3 that could have affected this behavior?

Comment 11

[Robert Relyea]

Not that I can think of. If there was something that let a longer label through, that would have been a bug.

Regression or not, PSM needs to have Token Descriptions of 31 or less bytes.

bob

Comment 12

[Martin Arndt]

So, we all know that description and label of the FIPS-security device got, well, "interchanged" - or at least the label is cut because of a 32-char-limit..

IMHO this is _very_ trivial to fix, so why doesn't a developer does it? :(

Comment 13

[u60234]

Attachment: patch

From my limited understanding of this code, I would guess that this was caused by the main patch in bug 391296. The "fpslotdes" variable was renamed to "ftokdes" and then switched place with "fslotdes" at http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Fsecurity%2Fnss%2Flib%2Fsoftoken&file=sftkpars.c&rev1=1.3&rev2=1.4&whitespace_mode=show&diff_mode=context#7

That looks like a reasonable change, so I guess Nelson's idea of changing the caller in nsNSSComponent.cpp is a good way to fix this bug.

Comment 14

[Martin Arndt]

Thanks a lot for providing a patch! :)

Since I'm not that deep into C(++) and CVS/SVN in general, I as a Nightly user would like to know when this appears within the builds.. ;)

Comment 15

[Stefan [:stefanh]]

(In reply to comment #14)
> Thanks a lot for providing a patch! :)
> 
> Since I'm not that deep into C(++) and CVS/SVN in general, I as a Nightly user
> would like to know when this appears within the builds.. ;)
> 

It will appear in the nightlies once the patch has been reviewed and landed (in other words, when this bug have been resolved as fixed).

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 331986 [details] [diff] [review]
patch

>+  nsAutoString fips140SlotDescription;
>   nsAutoString fips140TokenDescription;
>-  nsAutoString fips140SlotDescription;

This code has no effect, you are simply changing the order.


>-  rv = GetPIPNSSBundleString("Fips140TokenDescription", fips140TokenDescription);
>+  rv = GetPIPNSSBundleString("Fips140SlotDescription", fips140SlotDescription);
>   if (NS_FAILED(rv)) return rv;
> 
>-  rv = GetPIPNSSBundleString("Fips140SlotDescription", fips140SlotDescription);
>+  rv = GetPIPNSSBundleString("Fips140TokenDescription", fips140TokenDescription);
>   if (NS_FAILED(rv)) return rv;

This code has no effect, you are simply changing the order.


when calling function PK11_ConfigurePKCS11
>                        NS_ConvertUTF16toUTF8(privateSlotDescription).get(),
>+                       NS_ConvertUTF16toUTF8(fips140SlotDescription).get(),
>                        NS_ConvertUTF16toUTF8(fips140TokenDescription).get(),
>-                       NS_ConvertUTF16toUTF8(fips140SlotDescription).get(),
>                        0, 0);

Clearly Bob Relyea should not have made a change that requires a change of parameters to this stable interface. If this change helps to fix the regression, then I believe Bob's change in bug 391296 had an undesired side effect.
I have sent email to Bob asking him to double check his change.

Comment 17

[David Ward]

It's been a year since this bug has been touched, but it's still present in Firefox 3.5.2 and Thunderbird 3.0b3.  What needs to be done to get this fixed?  Thanks.

Comment 18

[Bob Lord]

Attachment: bizarre password dialog text

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7 GTB6

I'm attaching a screenshot of the dialog containing the strange wording.

Comment 19

[Kai Engert (:KaiE:)]

We tried to fix this in the past (bug 317630), but it seems it wasn't sufficient.

Comment 20

[Martin Arndt]

Any progress on this bug?

Seemed to not require a lot of code and should definitely get fixed before 4.0 is released, because such a small UI-glitch isn't a good example for an otherwise very good product.

Comment 21

[bugzilla]

Is anyone looking at this? Yes, it's a minor irritation, but it's been 5 years now!

Comment 22

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

Attachment: 428421-fips-token-name.diff

Hasse's patch is correct. The order in which PSM supplied the parameters in question to PK11_ConfigurePKCS11 was switched in bug 317630. I've rebased it (the location of nsNSSComponent.cpp changed) and I'm going to check it in.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd10c66b9a2c

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd10c66b9a2c