Closed Bug 629090 Opened 13 years ago Closed 13 years ago

NSS's J-PAKE Implementation Allows Session Key Retrieval Without Knowing The Shared Secret

Categories

(NSS :: Libraries, defect)

defect
Not set
major

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mcoates, Assigned: briansmith)

Details

(Whiteboard: [MOZ2-8][sg:high][infrasec:crypto][ws:high][hardblocker][has patch])

Attachments

(2 files, 2 obsolete files)

Attached file Security Event Report
This issue was reported by our third party security reviewers. The full
security issue is attached and includes steps to reproduce this issue.

Summary:
 
The J-PAKE implementation in Firefox browser's Sync client doesn't seem to check if the other party has provided g^x2 equal to 1 (which would mean that x2 is 0). Additionally, neither Firefox Sync nor Firefox Home seem to set the upper bound for g^x2 (or perform an initial modulo p operation on it) to prevent the other party from providing number p+1 which is in effect identical to 1.
This allows an active attacker (rogue server) to masquerade as a legitimate peer without knowing the shared secret. As a result, a rogue server can first extract the Sync credentials from the "sender", and then immediately pass them on to the "receiver", thus avoiding the legitimate J-PAKE parties from noticing anything suspect.


Recommendations:

We recommend that the Firefox Sync client, like the Firefox Home, checks the received g^x2 (or g^x4) to be not equal to 1. In order to avoid the vulnerability described in http://seb.dbzteam.org/crypto/jpake-session-key-retrieval.pdf, the received g^x2 (or g^x4) must either be checked not to exceed p or be reduced modulo p immediately upon receipt (since p+1, 2p+1, etc. are also identical to 1 in the context of this calculation). Finally, the received A (or B) must also be not equal to 1.
This issue as reported is in the fx-sync and Firefox Home products, not the server product.  Moving to client component.
Component: Server: Key Exchange → Firefox Sync: Backend
QA Contact: key-exchange-server → sync-backend
Brian, please take a look here.
Assignee: nobody → bsmith
blocking2.0: --- → betaN+
Component: Firefox Sync: Backend → Firefox Sync: Crypto
QA Contact: sync-backend → sync-crypto
Whiteboard: [MOZ2-8][infrasec:crypto][ws:high] → [MOZ2-8][infrasec:crypto][ws:high][hardblocker]
Interesting. We did actually fix the code based on

http://seb.dbzteam.org/crypto/jpake-session-key-retrieval.pdf

I will take a thorough look to see if we missed a case.

(The changes that I made were reviewed by the author of the above paper, Ben Laurie (who put the same fixes in OpenSSL) and the original author or JPAKE.

It is of course very possible that we missed something.
(In reply to comment #3)
> Interesting. We did actually fix the code based on
> 
> http://seb.dbzteam.org/crypto/jpake-session-key-retrieval.pdf
> 
> I will take a thorough look to see if we missed a case.

Note that Firefox Home has a separate bug 629133. Clarifying that this bug is about the implementation in NSS.
Summary: J-PAKE Implementation Allows Session Key Retrieval Without Knowing The Shared Secret → NSS's J-PAKE Implementation Allows Session Key Retrieval Without Knowing The Shared Secret
(In reply to comment #0)
> We recommend that the Firefox Sync client, like the Firefox Home, checks the
> received g^x2 (or g^x4) to be not equal to 1.

IIRC, this check is subsumed by one of the other checks, but I can't remember which one. But, maybe I remember incorrectly. This is something I will investigate tonight. 

> In order to avoid the vulnerability described in
> http://seb.dbzteam.org/crypto/jpake-session-key-retrieval.pdf, the received
> g^x2 (or g^x4) must either be checked not to exceed p or be reduced modulo p
> immediately upon receipt (since p+1, 2p+1, etc. are also identical to 1 in the
> context of this calculation). 

NSS's J-PAKE does this check in JPAKE_Verify in jpake.c:

/* Check g^x is in [1, p-2], R is in [0, q-1], and (g^x)^q mod p == 1 */
292     if (!(mp_cmp_z(&GX) > 0 && 
293           mp_cmp(&GX, &p_minus_1) < 0 && 
294           mp_cmp(&R, &q) < 0 &&
295           mp_cmp_d(&one, 1) == 0)) {
296         goto badSig;
297     }
If this is an NSS issue, shouldn't this be moved over to the NSS product since that's where the code lives and where the NSS team can put any fixes through their QA process?
(In reply to comment #6)
> If this is an NSS issue, shouldn't this be moved over to the NSS product since
> that's where the code lives and where the NSS team can put any fixes through
> their QA process?

Quite likely. It feels like this kind of check should live in the NSS bits and pieces (if it doesn't already) and not in the glue code in services/crypto/. I'll leave it to bsmith to make that call.
To minimize scheduling risk, I am add the necessary check that gx^4 != 0 to the services/crypto code. This way, we do not end up waiting for a new NSS release and it will also protect Firefox from the user/administrator configuring Firefox to use an older version of FreeBL. I am building+testing this patch now.

In addition, I will file a new bug in NSS to track the addition of the same check to FreeBL so that the check is done where it should be.
(In reply to comment #8)
> To minimize scheduling risk, I am add the necessary check that gx^4 != 0

I meant g^x4 > 1.
Brian, ETA on the fix?
This afternoon.
Assignee: bsmith → nobody
Group: client-services-security → core-security
Component: Firefox Sync: Crypto → Libraries
Product: Mozilla Services → NSS
QA Contact: sync-crypto → libraries
Assignee: nobody → bsmith
ETA: today. The actual patch has been written, but I am working on getting the testcase to actually reproduce the issue.
Whiteboard: [MOZ2-8][infrasec:crypto][ws:high][hardblocker] → [MOZ2-8][sg:high][infrasec:crypto][ws:high][hardblocker]
Brian, where are we here?  If we can't effectively test this yet, can we at least get the patch in place?
Verification procedure:

1. Apply this patch
2. Run:
   make -C services/crypto/component/tests/ xpcshell-tests

Before the bug fix patch (coming next) is applied, the first test (g^x == p + 1) is successfully blocked but the second one (g^x == 1) is not blocked. After the bug fix, the g^x==1 case is successfully blocked.
Attachment #509874 - Flags: review?(philipp)
Attached patch Wallpaper fix to nsSyncJPAKE (obsolete) — Splinter Review
The upcoming changes to FreeBL are being tracked in Bug 631654. This patch will to Firefox's nsSyncJPAKE component will allow us to safely ship Firefox 4 without waiting for an updated NSS release with the check added.
Attachment #509878 - Flags: review?(mconnor)
Whiteboard: [MOZ2-8][sg:high][infrasec:crypto][ws:high][hardblocker] → [MOZ2-8][sg:high][infrasec:crypto][ws:high][hardblocker][has patch]
Attachment #509878 - Flags: review?(philipp)
Attachment #509878 - Flags: review?(mconnor)
Attachment #509878 - Flags: review+
Attachment #509878 - Flags: review?(philipp) → review+
Comment on attachment 509874 [details] [diff] [review]
Testcase demonstrating g^x==1 is not rejected but g^x==p+1 is rejected

>+function test_gx4_zero() {

That name might be a bit misleading, I suggest test_x4_zero() or test_gx4_one().

>+  // x is NIST 3072's p + 1, (p + 1) mod p == 1, g^1 == 0

I think you meant to say here:

  // gx is NIST 3072's p + 1, (p + 1) mod p == 1, x == 0

Looks good otherwise!
Attachment #509874 - Flags: review?(philipp) → review+
Updated testcase patch with philiKON's comments addressed combined with the wallpaper fix patch. r+ carried over.
Attachment #509874 - Attachment is obsolete: true
Attachment #509878 - Attachment is obsolete: true
Attachment #510938 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/2e453fece8c9
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This isn't checkin-needed anymore, is it?
Nope, it landed.
Keywords: checkin-needed
Status: RESOLVED → VERIFIED
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.