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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[q1]

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?

Comment 1

[q1]

BTW, similar code was in, e.g., the 48.0 branch.

Comment 2

[Daniel Veditz [:dveditz]]

JC: is this your code, or can you find the right owner? I believe this is not enabled in current builds, correct?

Comment 3

[J.C. Jones [:jcj] (he/him)]

Yes, it's my code - and it's behind a pref in all builds. Taking this.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[J.C. Jones [:jcj] (he/him)]

Attachment: 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".

Comment 6

[J.C. Jones [:jcj] (he/him)]

Try run looks alright: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f98728d35edf7cd707e7edd9b44a917799e3bff6&selectedJob=28840924

Comment 7

[J.C. Jones [:jcj] (he/him)]

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.

Comment 8

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

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

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

LGTM.

Comment 9

[J.C. Jones [:jcj] (he/him)]

Marking checkin-needed.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88be27f821aad089e816d470d4c5de3b111c484b

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88be27f821aa

Comment 12

[Daniel Veditz [:dveditz]]

Seems sec-low at best given comment 5.