Add securer mode to totp_face_lfs (secrets only in memory) #421
Conversation
|
|
||
| for (i = 0; i < num_totp_records; ++i) { | ||
| totp_face_lfs_get_file_secret(&totp_records[i]); | ||
| totp_records[i].secret = malloc(totp_records[i].secret_size); | ||
| if (totp_records[i].secret == NULL) { | ||
| num_totp_records = i - 1; | ||
| // We can't easily undo at this point, because we destroyed | ||
| // the offset info (secret is in a union). So let's just junk | ||
| // the remaining records. | ||
| break; | ||
| } | ||
| memcpy(totp_records[i].secret, current_secret, totp_records[i].secret_size); | ||
| } |
There was a problem hiding this comment.
How does this impact memory usage? Copying every secret into memory has caused resets in the past, and the fix was to read them into memory one at a time.
There was a problem hiding this comment.
Yes - there is a warning about this in the doco (i.e. that using secure mode is more likely to make you run out of memory, and whether it works well is a bit config dependent).
Personally, I've run 10 codes on my device without issue, but it obviously depends what other faces are loaded.
| memcpy(totp_records[i].secret, current_secret, totp_records[i].secret_size); | ||
| } | ||
|
|
||
| filesystem_rm(TOTP_FILE); |
There was a problem hiding this comment.
This is unlikely to be a secure erase operation. For performance reasons, file systems typically don't overwrite file contents when erasing, they just remove the links to the file from the file system data structures, causing the file to become unreachable and the memory to become available for new files. The data will not be overwritten until new data is written to the same physical location.
For this reason, it may be wise to overwrite the entire file with zeros before issuing the rm command to the file system.
This is adequate for spinning disks but may not be completely effective for flash storage: copies of the file may still linger on the storage medium if wear leveling is employed.
I do not know if the hardware supports a secure erase operation but it's probably a good idea to also issue those commands if it does. Even if it does, it is unknown how effective it is, hardware manufacturers are known for screwing up features like this. The data might therefore still be recoverable despite all possible precautions.
There was a problem hiding this comment.
👍
Yes, as I mentioned at the top, don't know enough about LFS to understand exactly what will happen here. I wouldn't feel confident in zero-ing it out as a method because I haven't looked at LFS's internal structure (I mean, if it was the same length... but as you mention, LFS could have some built-in wear levelling that means it will write it in a separate block). At least there's no hardware level magic here we have to worry about!
I'll change the top of the PR to make these caveats clearer.
wryun
changed the title
Since the totp_lfs keeps things on flash unencrypted, if you're particularly paranoid this loads them from LFS then removes them from flash.
WARNING: this probably doesn't remove them entirely from the flash; I haven't looked into how LFS does deletes, and if it's like most FS's this will just remove an entry rather than the data. I'm leaving this here because:
Split out work from #393