Closed Bug 488350 Opened 12 years ago Closed 12 years ago

NSPR-free freebl interface need to do post tests only in fips mode.

Categories

(NSS :: Libraries, defect, P1)

3.12.4
x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: rrelyea, Assigned: rrelyea)

Details

Attachments

(1 file, 1 obsolete file)

The NSSLOW_ interface to freebl should only do POST when the OS is in FIPS mode.
Attached patch freebl patch (obsolete) — Splinter Review
Attachment #372679 - Attachment is obsolete: true
Attachment #372682 - Flags: superreview?
Attachment #372682 - Flags: review?(nelson)
Attachment #372682 - Flags: superreview? → superreview?(wtc)
Attachment #372682 - Flags: superreview?(wtc) → superreview+
Comment on attachment 372682 [details] [diff] [review]
Friendlier version of this patch.

r=wtc.

It's strange for nsslow_GetFIPSEnabled() to return 1 by default.
It seems that nsslow_GetFIPSEnabled() should return 0 by default.
In other words, the code at the end of the function should look
like this:

>+    if (d == '1')
>+        return 1;
>+#endif
>+    return 0;
>+}

Also, on Linux distributions that don't have the special file
/proc/sys/crypto/fips_enabled, nsslow_GetFIPSEnabled() returns 0.
Is that what you want?
re: default.

on non-linux, I would think that you would want the fips check if you don't have a way of turning it on. It's moot since only linux uses this mode right now. I'd be happy to change the default.

re missing /proc....

That's a better question. I thought about it to and I lean toward turning it on if the /proc doesn't exist, but I could go either way. Thanks for the review wan-Teh..
Yeah, the default return value doesn't matter much to me,
but the code would be more self-consistent if it treats
a non-Linux OS and a Linux distro without /proc/sys/crypto/fips_enabled
the same way.
This is obviously P1.
Priority: -- → P1
Comment on attachment 372682 [details] [diff] [review]
Friendlier version of this patch.

/proc seems like an odd place for this.  It's not a process...  but I don't really care.  If Linux wants it there...

Is there any sort of attack possible here?  
Might an attacker want to disable FIPS mode for some reason?  Or force it to be enabled for some reason?  
Might an attacker be able to effect the change he wants by playing some game with the directories in that path?  
If so, is there a way for this to "fail safe"?
In the event of an attack, should it fail by going into FIPS mode by default? or by not going into FIPS mode by default?

I don't have answers to these questions.  I'm just passing them on for you to think about.
Attachment #372682 - Attachment description: Friendlyer version of this patch. → Friendlier version of this patch.
Attachment #372682 - Flags: review?(nelson) → review+
Might an attacker be able to effect this change by doing a chroot 
in a parent process before running the code that uses NSS?
I know /proc is a mounted filesystem. It's used to present kernel variables (not just processes). On linux you can find out all sorts of things, and privileged users can even change kernel behavior on the fly.

chroot is an interesting issue. I think that says we should probably default to FIPS on as wtc suggests.

bob
Checking in nsslowhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/nsslowhash.c,v  <--  nsslowhash.c
new revision: 1.3; previous revision: 1.2
done

Checked in. default for failure to open changes to '1' (use fips mode).
fixed
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Your checkin looks good, Bob.  Nit: to go all the
way, you may want to do something like this so
that only a value of '0' in /proc/sys/crypto/fips_enabled
can make nsslow_GetFIPSEnabled return 0:

+    if (size != 1)
+        return 1;
+    if (d == '0')
+        return 0;
(In reply to comment #12)
> Your checkin looks good, Bob.  Nit: to go all the
> way, you may want to do something like this so
> that only a value of '0' in /proc/sys/crypto/fips_enabled
> can make nsslow_GetFIPSEnabled return 0:
> 
> +    if (size != 1)
> +        return 1;
> +    if (d == '0')
> +        return 0;


Looks like this comment was missed.

Wan-Teh, if you propose this to be done, would it make sense to file a new bug?
You need to log in before you can comment on or make changes to this bug.