Skip to content

Commit 19962c9

Browse files
BryanJacobsBryanJacobs
committed
Safer largeBlob buffer swapping
1 parent 01024a6 commit 19962c9

File tree

2 files changed

+81
-22
lines changed

2 files changed

+81
-22
lines changed

‎python_tests/ctap/test_largeblobs.py‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ def test_get_empty_largeblob_arr(self):
160160
@parameterized.expand([
161161
("empty", 0),
162162
("short", 10),
163+
("short2", 78),
163164
("onepacket", 200),
164165
("onepacket2", 240),
165166
("medium", 100),
@@ -176,8 +177,33 @@ def test_set_and_get_large_blobs(self, _, num_bytes):
176177
res = LargeBlobs(self.ctap2).read_blob_array()
177178
self.assertEqual(blob_array, res)
178179

180+
def test_set_fragmented_low_level(self):
181+
blob_array = secrets.token_bytes(170)
182+
h = hashes.Hash(hashes.SHA256())
183+
h.update(blob_array)
184+
blob_array += h.finalize()[:16]
185+
186+
self.ctap2.large_blobs(offset=0, set=blob_array[:10], length=len(blob_array))
187+
self.ctap2.large_blobs(offset=10, set=blob_array[10:20])
188+
self.ctap2.large_blobs(offset=20, set=blob_array[20:])
189+
res = self.ctap2.large_blobs(offset=0, get=200)
190+
191+
self.assertEqual(blob_array, res[1])
192+
193+
def test_set_fragmented_incomplete_low_level(self):
194+
blob_array = secrets.token_bytes(170)
195+
h = hashes.Hash(hashes.SHA256())
196+
h.update(blob_array)
197+
blob_array += h.finalize()[:16]
198+
199+
self.ctap2.large_blobs(offset=0, set=blob_array[:10], length=len(blob_array))
200+
self.ctap2.large_blobs(offset=10, set=blob_array[10:20])
201+
res = self.ctap2.large_blobs(offset=0, get=200)
202+
203+
self.assertEqual(17, len(res[1]))
204+
179205
def test_get_beyond_end(self):
180-
blob_array = secrets.token_bytes(54)
206+
blob_array = secrets.token_bytes(62)
181207
h = hashes.Hash(hashes.SHA256())
182208
h.update(blob_array)
183209
blob_array += h.finalize()[:16]
@@ -188,6 +214,18 @@ def test_get_beyond_end(self):
188214

189215
self.assertEqual(blob_array, res[1])
190216

217+
def test_rejects_with_invalid_hash(self):
218+
blob_array = secrets.token_bytes(62)
219+
h = hashes.Hash(hashes.SHA256())
220+
h.update(blob_array)
221+
blob_array += h.finalize()[:16]
222+
blob_array = blob_array[:-1] + bytes([blob_array[-1] + 1])
223+
224+
with self.assertRaises(CtapError) as e:
225+
self.ctap2.large_blobs(offset=0, set=blob_array, length=len(blob_array))
226+
227+
self.assertEqual(CtapError.ERR.INTEGRITY_FAILURE, e.exception.code)
228+
191229
def test_set_and_get_large_blobs_high_level(self):
192230
cred = self.make_large_blob_key()
193231

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

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -348,11 +348,11 @@ public final class FIDO2Applet extends Applet implements ExtendedLength {
348348
/**
349349
* Storage for the largeBlobs extension
350350
*/
351-
private byte[] largeBlobStore;
351+
private final byte[][] largeBlobStores;
352352
/**
353-
* Double buffer for the large blob store
353+
* Which large blob store is in use
354354
*/
355-
private byte[] pendingLargeBlobStore;
355+
private byte largeBlobStoreIndex = 0;
356356
/**
357357
* Length of the currently stored large-blob array
358358
*/
@@ -3773,7 +3773,7 @@ private void handleLargeBlobs(APDU apdu, byte[] reqBuffer, short lc) {
37733773
sendErrorByte(apdu, FIDOConstants.CTAP1_ERR_INVALID_PARAMETER);
37743774
}
37753775

3776-
if (offset < 0 || offset > (short) largeBlobStore.length) {
3776+
if (offset < 0 || offset > (short) largeBlobStores[largeBlobStoreIndex].length) {
37773777
// Offset is mandatory and must be reasonable
37783778
sendErrorByte(apdu, FIDOConstants.CTAP1_ERR_INVALID_PARAMETER);
37793779
}
@@ -3804,7 +3804,7 @@ private void handleLargeBlobs(APDU apdu, byte[] reqBuffer, short lc) {
38043804
if (setTotalLength < 17) {
38053805
sendErrorByte(apdu, FIDOConstants.CTAP1_ERR_INVALID_PARAMETER);
38063806
}
3807-
if (setTotalLength > (short) largeBlobStore.length) {
3807+
if (setTotalLength > (short) largeBlobStores[largeBlobStoreIndex].length) {
38083808
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_LARGE_BLOB_STORAGE_FULL);
38093809
}
38103810
} else {
@@ -3875,8 +3875,9 @@ private void handleLargeBlobs(APDU apdu, byte[] reqBuffer, short lc) {
38753875
*/
38763876
private void handleLargeBlobSet(APDU apdu, byte[] buffer, short incomingDataOffset,
38773877
short blobWriteOffset, short incomingDataLength, short totalLength) {
3878+
final byte tempBlobStoreIndex = (byte)(largeBlobStoreIndex == 0 ? 1 : 0);
38783879
Util.arrayCopyNonAtomic(buffer, incomingDataOffset,
3879-
pendingLargeBlobStore, blobWriteOffset, incomingDataLength);
3880+
largeBlobStores[tempBlobStoreIndex], blobWriteOffset, incomingDataLength);
38803881
// Done with incoming request now
38813882
bufferManager.informAPDUBufferAvailability(apdu, (short) 0xFF);
38823883

@@ -3889,11 +3890,11 @@ private void handleLargeBlobSet(APDU apdu, byte[] buffer, short incomingDataOffs
38893890
byte[] scratch = bufferManager.getBufferForHandle(apdu, scratchHandle);
38903891
short scratchOffset = bufferManager.getOffsetForHandle(scratchHandle);
38913892

3892-
sha256.doFinal(pendingLargeBlobStore, (short) 0, (short)(totalLength - 16),
3893+
sha256.doFinal(largeBlobStores[tempBlobStoreIndex], (short) 0, (short)(totalLength - 16),
38933894
scratch, scratchOffset);
38943895

38953896
if (Util.arrayCompare(scratch, scratchOffset,
3896-
pendingLargeBlobStore, (short)(totalLength - 16), (short) 16) != 0) {
3897+
largeBlobStores[tempBlobStoreIndex], (short)(totalLength - 16), (short) 16) != 0) {
38973898
// hash mismatch
38983899
sendErrorByte(apdu, FIDOConstants.CTAP2_ERR_INTEGRITY_FAILURE);
38993900
}
@@ -3904,9 +3905,7 @@ private void handleLargeBlobSet(APDU apdu, byte[] buffer, short incomingDataOffs
39043905
JCSystem.beginTransaction();
39053906
boolean ok = false;
39063907
try {
3907-
byte[] temp = pendingLargeBlobStore;
3908-
pendingLargeBlobStore = largeBlobStore;
3909-
largeBlobStore = temp;
3908+
largeBlobStoreIndex = tempBlobStoreIndex;
39103909
largeBlobStoreFill = totalLength;
39113910
} finally {
39123911
if (ok) {
@@ -3953,7 +3952,7 @@ private void handleLargeBlobGet(APDU apdu, short numBytes, short offset) {
39533952
outBuffer[writeIdx++] = (byte) 0xA1; // map - one key
39543953
outBuffer[writeIdx++] = (byte) 0x01; // map key: config
39553954
writeIdx = encodeIntLenTo(outBuffer, writeIdx, bytesToRetrieve, true);
3956-
writeIdx = Util.arrayCopyNonAtomic(largeBlobStore, offset,
3955+
writeIdx = Util.arrayCopyNonAtomic(largeBlobStores[largeBlobStoreIndex], offset,
39573956
outBuffer, writeIdx, bytesToRetrieve);
39583957

39593958
if (outBuffer == bufferMem) {
@@ -5370,10 +5369,35 @@ private void authenticatorReset(APDU apdu) {
53705369
}
53715370
}
53725371

5373-
short pinIdx = pinRetryCounter.prepareIndex();
5372+
final short pinIdx = pinRetryCounter.prepareIndex();
5373+
final byte tempBlobStoreIndex = (byte)(largeBlobStoreIndex == 0 ? 1 : 0);
5374+
final byte realBlobStoreIndex = largeBlobStoreIndex;
53745375

5376+
// Empty the large blob store OUTside the main transaction, since it's non-precious and double buffered
5377+
Util.arrayFillNonAtomic(largeBlobStores[tempBlobStoreIndex], (short) 0,
5378+
(short) largeBlobStores[tempBlobStoreIndex].length, (byte) 0x00);
5379+
Util.arrayCopyNonAtomic(CannedCBOR.INITIAL_LARGE_BLOB_ARRAY, (short) 0,
5380+
largeBlobStores[tempBlobStoreIndex], (short) 0, (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length);
53755381
JCSystem.beginTransaction();
53765382
boolean ok = false;
5383+
try {
5384+
largeBlobStoreIndex = tempBlobStoreIndex;
5385+
largeBlobStoreFill = (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length;
5386+
ok = true;
5387+
} finally {
5388+
if (ok) {
5389+
JCSystem.commitTransaction();
5390+
} else {
5391+
JCSystem.abortTransaction();
5392+
}
5393+
}
5394+
Util.arrayFillNonAtomic(largeBlobStores[realBlobStoreIndex], (short) 0,
5395+
(short) largeBlobStores[realBlobStoreIndex].length, (byte) 0x00);
5396+
Util.arrayCopyNonAtomic(CannedCBOR.INITIAL_LARGE_BLOB_ARRAY, (short) 0,
5397+
largeBlobStores[realBlobStoreIndex], (short) 0, (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length);
5398+
5399+
JCSystem.beginTransaction();
5400+
ok = false;
53775401
try {
53785402
random.generateData(hmacWrapperBytesUV, (short) 0, (short) hmacWrapperBytesUV.length);
53795403
random.generateData(hmacWrapperBytesNoUV, (short) 0, (short) hmacWrapperBytesNoUV.length);
@@ -5388,10 +5412,6 @@ private void authenticatorReset(APDU apdu) {
53885412
alwaysUv = FORCE_ALWAYS_UV;
53895413
enterpriseAttestation = false;
53905414
pinRetryCounter.reset(pinIdx);
5391-
Util.arrayFillNonAtomic(largeBlobStore, (short) 0, (short) largeBlobStore.length, (byte) 0x00);
5392-
Util.arrayCopyNonAtomic(CannedCBOR.INITIAL_LARGE_BLOB_ARRAY, (short) 0,
5393-
largeBlobStore, (short) 0, (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length);
5394-
largeBlobStoreFill = (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length;
53955415
Util.arrayFillNonAtomic(minPinRPIDs, (short) 0, (short) (MAX_RP_IDS_MIN_PIN_LENGTH * RP_HASH_LEN), (byte) 0x00);
53965416

53975417
random.generateData(pinKDFSalt, (short) 0, (short) pinKDFSalt.length);
@@ -5574,7 +5594,7 @@ private void sendAuthInfo(APDU apdu) {
55745594

55755595
buffer[offset++] = 0x0B; // map key: maxSerializedLargeBlobArray: 1 byte = 5
55765596
buffer[offset++] = 0x19; // two-byte integer: 1 byte = 6
5577-
offset = Util.setShort(buffer, offset, (short) largeBlobStore.length); // 2 bytes = 8
5597+
offset = Util.setShort(buffer, offset, (short) largeBlobStores[largeBlobStoreIndex].length); // 2 bytes = 8
55785598

55795599
buffer[offset++] = 0x0C; // map key: forcePinChange: 1 byte = 9
55805600
buffer[offset++] = (byte)(forcePinChange ? 0xF5 : 0xF4); // 1 byte = 10
@@ -6798,10 +6818,11 @@ private FIDO2Applet(byte[] array, short offset, byte length) {
67986818
highSecurityWrappingIV = new byte[IV_LEN];
67996819
lowSecurityWrappingIV = new byte[IV_LEN];
68006820
externalCredentialIV = new byte[IV_LEN];
6801-
largeBlobStore = new byte[largeBlobStoreSize];
6802-
pendingLargeBlobStore = new byte[largeBlobStoreSize];
6821+
largeBlobStores = new byte[2][largeBlobStoreSize];
6822+
Util.arrayCopyNonAtomic(CannedCBOR.INITIAL_LARGE_BLOB_ARRAY, (short) 0,
6823+
largeBlobStores[0], (short) 0, (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length);
68036824
Util.arrayCopyNonAtomic(CannedCBOR.INITIAL_LARGE_BLOB_ARRAY, (short) 0,
6804-
largeBlobStore, (short) 0, (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length);
6825+
largeBlobStores[1], (short) 0, (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length);
68056826
largeBlobStoreFill = (short) CannedCBOR.INITIAL_LARGE_BLOB_ARRAY.length;
68066827
highSecurityWrappingKey = getTransientAESKey(); // Our most important treasure, from which all other crypto is born...
68076828
lowSecurityWrappingKey = getPersistentAESKey(); // Not really a treasure

0 commit comments

Comments
 (0)