pkcs11-tool --write-object may corrupt flash

[al-heisner]

I'm using this shell script to test (erases all data on the hsm!):

#!/bin/bash

stty -F /dev/ttyUSB0 ispeed 115200 ospeed 115200 raw
cat /dev/ttyUSB0 > /tmp/hsm.log &
pid=$!

echo "" | python3 ../tools/pico-hsm-tool.py initialize

dd if=/dev/urandom of=/tmp/testfile bs=3000 count=1
for x in {01..06}; do
   pkcs11-tool --private --pin 'env:pass' --type data --write-object /tmp/testfile --label test.$x
done

echo "" | python3 ../tools/pico-hsm-tool.py initialize

dd if=/dev/urandom of=/tmp/testfile bs=2000 count=1
for x in {01..06}; do
   pkcs11-tool --private --pin 'env:pass' --type data --write-object /tmp/testfile --label test.$x
done

ps -ef | grep $pid
kill $pid
cat /tmp/hsm.log

This produces a state where the HSM is unable to read or delete the last file written, and debug messages seem to show error followed by corrupted file id and size for last file written:

ERROR: ALL FLASH PAGES CACHED
ERROR: ALL FLASH PAGES CACHED
ERROR: ALL FLASH PAGES CACHED
SCAN
[101fffb4] scan fid 100a, len 52
[101fff74] scan fid 100b, len 52
[101fff47] scan fid cc00, len 33
[101ffd4a] scan fid ce00, len 497
[101ff992] scan fid c400, len 58
[101ff5f1] scan fid 2f02, len 917
[101fbfbf] scan fid 1081, len 33
[101fbf92] scan fid 1088, len 33
[101fbf85] scan fid 1083, len 1
[101fbf78] scan fid 108a, len 1
[101fbf6b] scan fid 1082, len 1
[101fbf5e] scan fid 1089, len 1
[101fbf50] scan fid 100e, len 2
[101fbf42] scan fid 10a0, len 2
[101fb766] scan fid cd00, len 2000
[101fb733] scan fid c900, len 39
[101fb700] scan fid c901, len 39
[101fb6cd] scan fid c902, len 39
[101fb69a] scan fid c903, len 39
[101fb667] scan fid c904, len 39
[101fb634] scan fid c905, len 39
[101fa824] scan fid cd01, len 2000
[101fa048] scan fid cd02, len 2000
[101f9824] scan fid cd03, len 2000
[101f9048] scan fid cd04, len 2000
[101f8824] scan fid 9531, len 61168

Through some trial and error testing, I've been able to get this to work by changing TOTAL_FLASH_PAGES to 5 in low_flash.c, then I'm able to write far more files with no corruption. However, I now get to 23 objects written and retrieved successfully, but the 24th, while it appears to write successfully it cannot be retrieved or deleted. I haven't figured out why it now fails at 24 objects, object size doesn't appear to matter.

[al-heisner]

I was able to track down why it was failing at 24 objects. cmd_list_keys returns 2 bytes per key in the list. Once the list reached 52, the return apdu was split because of the fix in apdu_finish for mutiples of 64. This seemed to break SCSH3, pkcs11-tool, and pkcs15-tool. I got around this by null padding the return apdu when the list would return a multiple of 64. My git diff's:

diff --git a/src/hsm/cmd_list_keys.c b/src/hsm/cmd_list_keys.c
index ef92c6f..d3bc6af 100644
--- a/src/hsm/cmd_list_keys.c
+++ b/src/hsm/cmd_list_keys.c
@@ -60,5 +60,9 @@ int cmd_list_keys() {
             res_APDU[res_APDU_size++] = f->fid & 0xff;
         }
     }
+    if ((apdu.rlen + 2 + 10) % 64 == 0) {     // FIX for strange behaviour with PSCS and multiple of 64
+        res_APDU[res_APDU_size++] = 0;
+       res_APDU[res_APDU_size++] = 0;
+    }
     return SW_OK();
 }

diff --git a/src/fs/low_flash.c b/src/fs/low_flash.c
index a70a216..26dc777 100644
--- a/src/fs/low_flash.c
+++ b/src/fs/low_flash.c
@@ -39,7 +39,7 @@ uint8_t *map = NULL;
 #include "pico_keys.h"
 #include 

-#define TOTAL_FLASH_PAGES 4
+#define TOTAL_FLASH_PAGES 6

 extern const uintptr_t start_data_pool;
 extern const uintptr_t end_rom_pool;

[polhenarejos]

I am aware of this bug (really strange, though). It is handled at https://github.com/polhenarejos/pico-keys-sdk/blob/f0687c1ef392c2bcb293ea554f1dd8b784484922/src/apdu.c#L209

Perhaps there's some buggy in it. I'll take a look.

[al-heisner]

Yes, that blob referenced is what splits the APDU whenever the end length would have been exactly 64 bytes. Splitting the APDU is what appears to break the tools on the host side when listing keys. So instead of having the return APDU split when listing keys, I added null padding (in my git diff above) when the same conditions are met to avoid having it split the APDU. I've tested with over 60 written objects with the patch I posted above and this appears to work.

[polhenarejos]

Seems reasonable, but to me it is clearly a bug of OpenSC, which does not handle properly APDU codes. It may happen with other commands too BTW.

Can you send a PR?

[al-heisner]

I agree, this is workaround for bugs in OpenSC, SCSH3, etc where APDU return code for more data available is not handled. I submitted PRs for this change and one for piko-keys-sdk for the change in TOTAL_FLASH_PAGES

[polhenarejos]

Merged.