Closed
Bug 428421
Opened 17 years ago
Closed 8 years ago
FIPS mode: Master password dialog truncates security device name
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: joejr, Assigned: u60234)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [psm-assigned])
Attachments
(2 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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.
Assignee: nobody → kengert
Status: UNCONFIRMED → NEW
Component: Libraries → Security: PSM
Ever confirmed: true
Product: NSS → Core
QA Contact: libraries → psm
Version: unspecified → 1.8 Branch
Comment 2•17 years ago
|
||
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•17 years ago
|
||
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.
Reporter | ||
Comment 4•17 years ago
|
||
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.)
Cp. bug 426051 for a FIPS label/description related regression on trunk.
Comment 6•17 years ago
|
||
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
Severity: trivial → normal
Keywords: regression
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.
Severity: normal → trivial
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: PC → All
Version: 1.8 Branch → Trunk
Sorry, I didn't mean to change the severity or status. Setting back to "Normal" and "New".
Severity: trivial → normal
Status: ASSIGNED → NEW
Comment 10•17 years ago
|
||
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•17 years ago
|
||
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•16 years ago
|
||
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? :(
Assignee | ||
Comment 13•16 years ago
|
||
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.
Attachment #331986 -
Flags: review?(kaie)
Comment 14•16 years ago
|
||
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•16 years ago
|
||
(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).
Updated•16 years ago
|
Attachment #331986 -
Flags: review?(kaie) → review-
Comment 16•16 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Updated•14 years ago
|
Assignee: kaie → nobody
Blocks: masterpassword
Comment 19•14 years ago
|
||
We tried to fix this in the past (bug 317630), but it seems it wasn't sufficient.
Depends on: 317630
Comment 20•14 years ago
|
||
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•11 years ago
|
||
Is anyone looking at this? Yes, it's a minor irritation, but it's been 5 years now!
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.
Attachment #331986 -
Attachment is obsolete: true
Attachment #8751968 -
Flags: review+
Assignee: nobody → hasse
Whiteboard: [psm-assigned]
Comment 23•8 years ago
|
||
Comment 24•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•