Failure to check return code in u2f.cpp can cause security breaches

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
a year ago
26 days ago

People

(Reporter: q1, Assigned: jcj)

Tracking

({sec-low})

Trunk
mozilla52
sec-low
Points:
---
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [post-critsmash-triage][domsecurity-active][adv-main52+])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Two  functions in u2f.cpp (dom\u2f\u2f.cpp) (which implements support for u2f security tokens) fail to check the return status from CryptoBuffer.Assign(), which is fallible [1]. This omission can cause these functions to, for example, register a user with a website using a null signature. Since an attacker easily can guess such a user's signature, the token does not protect her as it should. (In mitigation, presumably the website checks for and rejects a null signature?)

The bug is in U2FRegisterTask::Run() line 249, and U2FSignTask::Run() line 427 (these lines are from trunk as of the filing of this bug):

116: NS_IMETHODIMP
117: U2FRegisterTask::Run()
118: {
...
224:     // Get the registration data from the token
225:     CryptoBuffer regData;
226:     bool registerSuccess = false;
227:     bool isCompatible = false;
228: 
229:     for (auto token : mAuthenticators) {
230:       rv = token->IsCompatibleVersion(request.mVersion.Value(), &isCompatible);
231:       if (NS_FAILED(rv)) {
232:         ReturnError(ErrorCode::OTHER_ERROR);
233:         return NS_ERROR_FAILURE;
234:       }
235: 
236:       if (isCompatible) {
237:         uint8_t* buffer;
238:         uint32_t bufferlen;
239:         nsresult rv;
240:         rv = token->Register(appParam.Elements(), appParam.Length(),
241:                              challengeParam.Elements(), challengeParam.Length(),
242:                              &buffer, &bufferlen);
243:         if (NS_FAILED(rv)) {
244:           ReturnError(ErrorCode::OTHER_ERROR);
245:           return NS_ERROR_FAILURE;
246:         }
247: 
248:         MOZ_ASSERT(buffer);
249:         regData.Assign(buffer, bufferlen);
250:         free(buffer);
251:         registerSuccess = true;
252:         break;
253:       }
254:     }
255: 
256:     if (!registerSuccess) {
257:       // Try another request
258:       continue;
259:     }
260: 
261:     // Assemble a response object to return
262:     nsString clientDataBase64, registrationDataBase64;
263:     nsresult rvClientData =
264:       clientData.ToJwkBase64(clientDataBase64);
265:     nsresult rvRegistrationData =
266:       regData.ToJwkBase64(registrationDataBase64);
267:     if (NS_WARN_IF(NS_FAILED(rvClientData)) ||
268:         NS_WARN_IF(NS_FAILED(rvRegistrationData))) {
269:       ReturnError(ErrorCode::OTHER_ERROR);
270:       return NS_ERROR_FAILURE;
271:     }
272: 
273:     RegisterResponse response;
274:     response.mClientData.Construct(clientDataBase64);
275:     response.mRegistrationData.Construct(registrationDataBase64);
276:     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OK));
277: 
278:     ErrorResult result;
279:     mCallback->Call(response, result);
280:     NS_WARNING_ASSERTION(!result.Failed(), "callback failed");
281:     // Useful exceptions already got reported.
282:     result.SuppressException();
283:     return NS_OK;
284:   }
...



319: NS_IMETHODIMP
320: U2FSignTask::Run()
321: {
...
386:     // Get the signature from the token
387:     CryptoBuffer signatureData;
388:     bool signSuccess = false;
389: 
390:     // We ignore mTransports, as it is intended to be used for sorting the
391:     // available devices by preference, but is not an exclusion factor.
392: 
393:     for (size_t a = 0; a < mAuthenticators.Length() && !signSuccess; ++a) {
394:       Authenticator token(mAuthenticators[a]);
395:       bool isCompatible = false;
396:       bool isRegistered = false;
397: 
398:       rv = token->IsCompatibleVersion(request.mVersion.Value(), &isCompatible);
399:       if (NS_FAILED(rv)) {
400:         ReturnError(ErrorCode::OTHER_ERROR);
401:         return NS_ERROR_FAILURE;
402:       }
403:       if (!isCompatible) {
404:         continue;
405:       }
406: 
407:       rv = token->IsRegistered(keyHandle.Elements(), keyHandle.Length(),
408:                                &isRegistered);
409:       if (NS_FAILED(rv)) {
410:         ReturnError(ErrorCode::OTHER_ERROR);
411:         return NS_ERROR_FAILURE;
412:       }
413: 
414:       if (isCompatible && isRegistered) {
415:         uint8_t* buffer;
416:         uint32_t bufferlen;
417:         nsresult rv = token->Sign(appParam.Elements(), appParam.Length(),
418:                                   challengeParam.Elements(), challengeParam.Length(),
419:                                   keyHandle.Elements(), keyHandle.Length(),
420:                                   &buffer, &bufferlen);
421:         if (NS_FAILED(rv)) {
422:           ReturnError(ErrorCode::OTHER_ERROR);
423:           return NS_ERROR_FAILURE;
424:         }
425: 
426:         MOZ_ASSERT(buffer);
427:         signatureData.Assign(buffer, bufferlen);
428:         free(buffer);
429:         signSuccess = true;
430:       }
431:     }
432: 
433:     if (!signSuccess) {
434:       // Try another request
435:       continue;
436:     }
437: 
438:     // Assemble a response object to return
439:     nsString clientDataBase64, signatureDataBase64;
440:     nsresult rvClientData =
441:       clientData.ToJwkBase64(clientDataBase64);
442:     nsresult rvSignatureData =
443:       signatureData.ToJwkBase64(signatureDataBase64);
444:     if (NS_WARN_IF(NS_FAILED(rvClientData)) ||
445:         NS_WARN_IF(NS_FAILED(rvSignatureData))) {
446:       ReturnError(ErrorCode::OTHER_ERROR);
447:       return NS_ERROR_FAILURE;
448:     }
449:     SignResponse response;
450:     response.mKeyHandle.Construct(request.mKeyHandle.Value());
451:     response.mClientData.Construct(clientDataBase64);
452:     response.mSignatureData.Construct(signatureDataBase64);
453:     response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OK));
454: 
455:     ErrorResult result;
456:     mCallback->Call(response, result);
457:     NS_WARNING_ASSERTION(!result.Failed(), "callback failed");
458:     // Useful exceptions already got reported.
459:     result.SuppressException();
460:     return NS_OK;
461:   }
462: 
463:   // Nothing could satisfy
464:   ReturnError(ErrorCode::DEVICE_INELIGIBLE);
465:   return NS_ERROR_FAILURE;
466: }

[1] Maybe it would be better for this function to be infallible?
(Reporter)

Comment 1

a year ago
BTW, similar code was in, e.g., the 48.0 branch.
JC: is this your code, or can you find the right owner? I believe this is not enabled in current builds, correct?
Group: core-security → dom-core-security
Flags: sec-bounty?
Flags: needinfo?(jjones)
(Assignee)

Comment 3

a year ago
Yes, it's my code - and it's behind a pref in all builds. Taking this.
Assignee: nobody → jjones
Status: NEW → ASSIGNED
Flags: needinfo?(jjones)
This seems to be a mis-implementation of U2F that will return/use bad results, but not a way to hack Firefox directly. Capping severity to sec-moderate.
Keywords: sec-moderate
(Assignee)

Comment 5

a year ago
Created attachment 8799081 [details] [diff] [review]
1306142-u2f-unchecked-assign.patch

The U2F.cpp code fails to test all returns from CryptoBuffer.Assign(),
leading (when OOM) to potentially empty registration keys (during Register),
or empty attestations (during Sign).

This is a protocol violation, and forced testing at Dropbox,
u2fdemo.appspot.com, and u2f.bin.coffee show that those Relying Parties'
implementations properly error if the registration or attestation is empty,
as would happen in this instance.

As this is only on an OOM condition, it's not really feasible to add an
automated test.

Also catches one other Assign() that isn't properly returning
"NS_ERROR_OUT_OF_MEMORY".
Attachment #8799081 - Flags: review?(dveditz)
(Assignee)

Comment 6

a year ago
Try run looks alright: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f98728d35edf7cd707e7edd9b44a917799e3bff6&selectedJob=28840924
Priority: -- → P1
Whiteboard: [domsecurity-active]
(Assignee)

Comment 7

a year ago
Comment on attachment 8799081 [details] [diff] [review]
1306142-u2f-unchecked-assign.patch

Changing reviewer to :keeler; this is pretty simple, and it'd be good to land soon.
Attachment #8799081 - Flags: review?(dveditz) → review?(dkeeler)
Comment on attachment 8799081 [details] [diff] [review]
1306142-u2f-unchecked-assign.patch

Review of attachment 8799081 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8799081 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 9

a year ago
Marking checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/88be27f821aad089e816d470d4c5de3b111c484b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88be27f821aa
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Seems sec-low at best given comment 5.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-moderate → sec-low
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [domsecurity-active] → [post-critsmash-triage][domsecurity-active]
Whiteboard: [post-critsmash-triage][domsecurity-active] → [post-critsmash-triage][domsecurity-active][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.