littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
5.16k stars 793 forks source link

Information leaks through padding #67

Open rojer opened 6 years ago

rojer commented 6 years ago

I was examining on-device state left by LFS and was surprised to find some pieces of data that were not supposed to be on the FS at all. Turns out, these were pieces of heap that hitched a ride to the persistent storage as padding for writes. This is not supposed to happen and it's a pretty serious issue. I was able to put a quick bandaid on by adding memset right after the buffer mallocs, so at least non-filesystem data doesn't make it through. However, more careful approach is needed to ensure that there is no cross-contamination within the filesystem - e.g. parts of file_a data do not end up in padding for file_b or some directory structure and thus live long after file_a is gone.

geky commented 6 years ago

Ah yes, that is not good. Hmm, this should be fixable by zeroing the caches when we "drop".

geky commented 6 years ago

Here we go, how does this look to you? https://github.com/ARMmbed/littlefs/pull/69

geky commented 6 years ago

Actually, I think it may be flawed to assume removing file_a completely erases the data from the disk. As a copy-on-write filesystem, many copies of your data may end up on different places on the disk and it would be prohibitively expensive to erase these.

Can you think of another reason cross-contamination within the filesystem would be a problem? I'm having a hard time coming up with a situation that would actually be an issue.

Agree that leaking data from malloc is a problem.

rojer commented 6 years ago

As a copy-on-write filesystem, many copies of your data may end up on different places on the disk and it would be prohibitively expensive to erase these.

yeah, i understand that. however, that should be ok as long as these blocks are not reported as used. then scrubbing free space after deleting files that are supposed to be wiped would be ok, which is what i intend to do. it will only be done on factory reset, so it's ok if it takes some time. however, if the information ends up in a used block, e.g. as part of padding for a live file, then this won't work. so, LFS should ensure that used blocks are not cross-contaminated and scrubbing free blocks should be left to the user if they wish.

geky commented 6 years ago

Ah interesting. How does that work with the metadata stored in directory blocks? Right now this is only the file location and size, but with inline files this could be the entire file.

rojer commented 6 years ago

well, that'd be a problem then. basically, i would like to have a way of scrubbing the fs, even if it's expensive. perhaps, introduce a "gc" process for this purpose that would relocate and scrub entries that contain such garbage? as a utility function, perhaps, so if you don't use it, it doesn't increase code size.

rojer commented 6 years ago

as an example, SPIFFS has a SPIFFS_gc function that will erase a bunch of unused blocks. currently, to scrub the FS i just repeatedly invoke it until it makes no further progress.

geky commented 6 years ago

Hmm, I'll have to think about that. It would definitely be expensive and stress inducing for the block device.

Another option is we could add an optional LFS_O_NOINLINE flag that would let you force sensitive files into their own blocks.

rojer commented 6 years ago

this has the disadvantage that we'd have to know upfront which files are sensitive which is not necessarily the case...

On Fri, Jul 6, 2018, 22:08 Christopher Haster notifications@github.com wrote:

Hmm, I'll have to think about that. It would definitely be expensive and stress inducing for the block device.

Another option is we could add an optional LFS_O_NOINLINE flag that would let you force sensitive files into their own blocks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ARMmbed/littlefs/issues/67#issuecomment-403121934, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgbayYsGPB-NBLg_OQX_9iVo0IEX6PJks5uD7XEgaJpZM4VDiQ_ .

-- Deomid "rojer" Ryabkov, Software Engineer Cesanta Software Ltd.

geky commented 6 years ago

Sorry didn't mean to close this. I think there's still an interesting discussion to be had about a potential scrub function.