Skip to content

Commit dc92d86

Browse files
BryanJacobsBryanJacobs
committed
Error handling and correctness
1 parent a80b6a3 commit dc92d86

File tree

2 files changed

+69
-18
lines changed

2 files changed

+69
-18
lines changed

‎src/main/java/us/q3q/fido2/CannedCBOR.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ public abstract class CannedCBOR {
2121
};
2222
static final byte[] AUTH_INFO_RESPONSE = {
2323
FIDOConstants.CTAP2_OK,
24-
(byte) 0xA8, // Map - eight keys
24+
(byte) 0xA9, // Map - nine keys
2525
0x01, // map key: versions
2626
(byte) 0x82, // array - two items
2727
0x68, // string - eight bytes long

‎src/main/java/us/q3q/fido2/FIDO2Applet.java‎

Lines changed: 68 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,7 @@ private void makeCredential(APDU apdu, short lc, short readIdx) {
495495
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
496496
}
497497

498-
readIdx = consumeMapAndGetID(apdu, readIdx, lc);
498+
readIdx = consumeMapAndGetID(apdu, readIdx, lc, false, false);
499499
short rpIdIdx = transientStorage.getStoredIdx();
500500
short rpIdLen = transientStorage.getStoredLen();
501501

@@ -506,7 +506,7 @@ private void makeCredential(APDU apdu, short lc, short readIdx) {
506506
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
507507
}
508508

509-
readIdx = consumeMapAndGetID(apdu, readIdx, lc);
509+
readIdx = consumeMapAndGetID(apdu, readIdx, lc, true, false);
510510
short userIdIdx = transientStorage.getStoredIdx();
511511
short userIdLen = transientStorage.getStoredLen();
512512
if (userIdLen > MAX_USER_ID_LENGTH) {
@@ -685,13 +685,9 @@ private void makeCredential(APDU apdu, short lc, short readIdx) {
685685

686686
short excludeReadIdx = excludeListStartIdx;
687687
for (short excludeListIdx = 0; excludeListIdx < numExcludeListEntries; excludeListIdx++) {
688-
excludeReadIdx = consumeMapAndGetID(apdu, excludeReadIdx, lc);
688+
excludeReadIdx = consumeMapAndGetID(apdu, excludeReadIdx, lc, true, true);
689689
short credIdIdx = transientStorage.getStoredIdx();
690690
short credIdLen = transientStorage.getStoredLen();
691-
if (credIdIdx == 0) {
692-
// Not having an ID in here means it's invalid
693-
sendErrorByte(apdu, FIDOConstants.CTAP1_ERR_INVALID_PARAMETER);
694-
}
695691
if (credIdLen != CREDENTIAL_ID_LEN) {
696692
// ruh-roh, the exclude list has bogus stuff in it...
697693
// it could be a credential ID from some OTHER authenticator, so ignore it.
@@ -1110,9 +1106,14 @@ private short verifyPinAuth(APDU apdu, short readIdx, short clientDataHashIdx, b
11101106
* @return New read index into bufferMem after consuming public key block
11111107
*/
11121108
private short checkIfPubKeyBlockSupported(APDU apdu, short readIdx, short lc) {
1113-
if (bufferMem[readIdx++] != (byte) 0xA2) {
1109+
byte mapDef = bufferMem[readIdx++];
1110+
if ((mapDef & 0xF0) != 0xA0) {
11141111
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_CBOR_UNEXPECTED_TYPE);
11151112
}
1113+
short numMapEntries = (short)(ub(mapDef) - ub((byte) 0xA0));
1114+
if (numMapEntries < 2) {
1115+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_MISSING_PARAMETER);
1116+
}
11161117
if (readIdx >= lc) {
11171118
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
11181119
}
@@ -1123,7 +1124,7 @@ private short checkIfPubKeyBlockSupported(APDU apdu, short readIdx, short lc) {
11231124
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
11241125
}
11251126
if (bufferMem[readIdx++] != 'a' || bufferMem[readIdx++] != 'l' || bufferMem[readIdx++] != 'g') {
1126-
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_UNSUPPORTED_ALGORITHM);
1127+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_MISSING_PARAMETER);
11271128
}
11281129

11291130
byte algIntType = bufferMem[readIdx++];
@@ -1165,6 +1166,10 @@ private short checkIfPubKeyBlockSupported(APDU apdu, short readIdx, short lc) {
11651166
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
11661167
}
11671168

1169+
for (short i = 2; i < numMapEntries; i++) {
1170+
readIdx = consumeAnyEntity(apdu, readIdx, lc);
1171+
}
1172+
11681173
return readIdx;
11691174
}
11701175

@@ -1602,9 +1607,13 @@ private void getAssertion(APDU apdu, short lc, short readIdx, short firstCredIdx
16021607

16031608
for (short i = 0; i < allowListLength; i++) {
16041609
short beforeReadIdx = blockReadIdx;
1605-
blockReadIdx = consumeMapAndGetID(apdu, blockReadIdx, lc);
1610+
blockReadIdx = consumeMapAndGetID(apdu, blockReadIdx, lc, true, true);
16061611
short pubKeyIdx = transientStorage.getStoredIdx();
16071612
short pubKeyLen = transientStorage.getStoredLen();
1613+
if (pubKeyLen == -1) {
1614+
// Invalid allow list entry - ignore
1615+
continue;
1616+
}
16081617

16091618
if (i < firstCredIdx) {
16101619
// Don't check credentials that are before our iteration point, just skip them
@@ -2393,17 +2402,20 @@ private short consumeAnyEntity(APDU apdu, short readIdx, short lc) {
23932402

23942403
/**
23952404
* Consumes a CBOR map and locates the element having string key "id". After call, temp variables
2396-
* representing the index and length of the matched value are set, if a match was found. Values for the ID
2397-
* element are allowed to be string or byte arrays.
2405+
* representing the index and length of the matched value are set, if a match was found.
23982406
*
23992407
* @param apdu Request/response object
24002408
* @param readIdx Current index into the request buffer
24012409
* @param lc Length of the request buffer, as sent by the platform
2410+
* @param byteString If true, ID should be a byte string: if false, a UTF-8 string
2411+
* @param checkTypePublicKey If true, check there is a "type" key with value UTF-8 string "public-key"
24022412
*
24032413
* @return New index into the request buffer, after consuming one CBOR map element
24042414
*/
2405-
private short consumeMapAndGetID(APDU apdu, short readIdx, short lc) {
2415+
private short consumeMapAndGetID(APDU apdu, short readIdx, short lc, boolean byteString, boolean checkTypePublicKey) {
24062416
boolean foundId = false;
2417+
boolean foundType = false;
2418+
boolean correctType = false;
24072419
transientStorage.readyStoredVars();
24082420
short mapDef = bufferMem[readIdx++];
24092421
if (readIdx >= lc) {
@@ -2445,6 +2457,11 @@ private short consumeMapAndGetID(APDU apdu, short readIdx, short lc) {
24452457
if (isId && foundId) {
24462458
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
24472459
}
2460+
boolean isType = (keyLen == 4 && bufferMem[readIdx] == 't' && bufferMem[(short)(readIdx+1)] == 'y'
2461+
&& bufferMem[(short)(readIdx+2)] == 'p' && bufferMem[(short)(readIdx+3)] == 'e');
2462+
if (isType && foundType) {
2463+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
2464+
}
24482465
readIdx += keyLen;
24492466
if (readIdx >= lc) {
24502467
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
@@ -2458,6 +2475,17 @@ private short consumeMapAndGetID(APDU apdu, short readIdx, short lc) {
24582475

24592476
short valLen = 0;
24602477
if (valDef == 0x78 || valDef == 0x58) {
2478+
if (isId) {
2479+
if (valDef == 0x78 && byteString) {
2480+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_CBOR_UNEXPECTED_TYPE);
2481+
} else if (valDef == 0x58 && !byteString) {
2482+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_CBOR_UNEXPECTED_TYPE);
2483+
}
2484+
}
2485+
if (isType && valDef == 0x58) {
2486+
// heh, literally "unexpected type" here
2487+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_CBOR_UNEXPECTED_TYPE);
2488+
}
24612489
valLen = ub(bufferMem[readIdx++]);
24622490
if (isId) {
24632491
idPos++;
@@ -2466,8 +2494,14 @@ private short consumeMapAndGetID(APDU apdu, short readIdx, short lc) {
24662494
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
24672495
}
24682496
} else if (valDef >= 0x61 && valDef < 0x78) {
2497+
if (isId && byteString) {
2498+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_CBOR_UNEXPECTED_TYPE);
2499+
}
24692500
valLen = (short) (valDef - 0x60);
24702501
} else if (valDef >= 0x41 && valDef < 0x58) {
2502+
if ((isId && !byteString) || isType) {
2503+
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_CBOR_UNEXPECTED_TYPE);
2504+
}
24712505
valLen = (short) (valDef - 0x40);
24722506
} else {
24732507
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_CBOR_UNEXPECTED_TYPE);
@@ -2478,6 +2512,13 @@ private short consumeMapAndGetID(APDU apdu, short readIdx, short lc) {
24782512
transientStorage.setStoredVars(idPos, valLen);
24792513
}
24802514

2515+
if (isType && checkTypePublicKey) {
2516+
foundType = true;
2517+
correctType = valLen == (short) CannedCBOR.PUBLIC_KEY_TYPE.length
2518+
|| Util.arrayCompare(bufferMem, readIdx,
2519+
CannedCBOR.PUBLIC_KEY_TYPE, (short) 0, (short) CannedCBOR.PUBLIC_KEY_TYPE.length) == 0;
2520+
}
2521+
24812522
readIdx += valLen;
24822523
if (readIdx >= lc) {
24832524
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INVALID_CBOR);
@@ -2488,6 +2529,16 @@ private short consumeMapAndGetID(APDU apdu, short readIdx, short lc) {
24882529
sendErrorByte(apdu, FIDOConstants.CTAP1_ERR_INVALID_PARAMETER);
24892530
}
24902531

2532+
if (checkTypePublicKey && !foundType) {
2533+
// entirely missing "type" field
2534+
sendErrorByte(apdu, FIDOConstants.CTAP1_ERR_INVALID_PARAMETER);
2535+
}
2536+
2537+
if (checkTypePublicKey && foundType && !correctType) {
2538+
// We found an ID entry, but it's not of type "public-key" - treat it as if we found nothing
2539+
transientStorage.readyStoredVars();
2540+
}
2541+
24912542
return readIdx;
24922543
}
24932544

@@ -2817,7 +2868,7 @@ private void handleUserUpdate(APDU apdu, short readOffset, short lc) {
28172868
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_MISSING_PARAMETER);
28182869
}
28192870

2820-
readOffset = consumeMapAndGetID(apdu, readOffset, lc);
2871+
readOffset = consumeMapAndGetID(apdu, readOffset, lc, true, true);
28212872
short credIdIdx = transientStorage.getStoredIdx();
28222873
short credIdLen = transientStorage.getStoredLen();
28232874
if (credIdLen != CREDENTIAL_ID_LEN) {
@@ -2829,7 +2880,7 @@ private void handleUserUpdate(APDU apdu, short readOffset, short lc) {
28292880
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_MISSING_PARAMETER);
28302881
}
28312882

2832-
readOffset = consumeMapAndGetID(apdu, readOffset, lc);
2883+
readOffset = consumeMapAndGetID(apdu, readOffset, lc, true, false);
28332884
short userIdIdx = transientStorage.getStoredIdx();
28342885
short userIdLen = transientStorage.getStoredLen();
28352886
if (userIdLen > MAX_USER_ID_LENGTH) {
@@ -2908,7 +2959,7 @@ private void handleDeleteCred(APDU apdu, short readOffset, short lc) {
29082959
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_MISSING_PARAMETER);
29092960
}
29102961

2911-
readOffset = consumeMapAndGetID(apdu, readOffset, lc);
2962+
readOffset = consumeMapAndGetID(apdu, readOffset, lc, true, true);
29122963
short credIdIdx = transientStorage.getStoredIdx();
29132964
short credIdLen = transientStorage.getStoredLen();
29142965

@@ -3440,7 +3491,7 @@ private void sendAuthInfo(APDU apdu) {
34403491
buffer[offset++] = 0x01; // one
34413492

34423493
buffer[offset++] = 0x14; // map key: remainingDiscoverableCredentials
3443-
offset = encodeInt(offset, (byte)(NUM_RESIDENT_KEY_SLOTS - numResidentCredentials));
3494+
offset = encodeIntTo(buffer, offset, (byte)(NUM_RESIDENT_KEY_SLOTS - numResidentCredentials));
34443495

34453496
apdu.setOutgoingAndSend((short) 0, offset);
34463497
}

0 commit comments

Comments
 (0)