cryptomator / cryptofs

Java Filesystem Provider with integrated encryption
GNU Affero General Public License v3.0
93 stars 35 forks source link

Ciphertext filechannels close() might change lastModified timestamp #205

Closed infeo closed 7 months ago

infeo commented 8 months ago

When opening a filechannel to a not previously opened file in cryptofs, an OpenCryptoFile for this file (path) is instantiated and OpenCryptoFile::newFileChannel-method is called on it.

There the ciphertext filechannel (which is the one actually writing to the underlying filesystem) is created and linked to the returned cleartext file channel. When closing the cleartext file channel via channel.close(), the "close chain" runs over JDKs AbstractInteruptibleChannel.close() and the implementations of the implCloseChannel() methods in AbstractFileChannel and last and most importantly, the CleartextFileChannel: https://github.com/cryptomator/cryptofs/blob/6f04f2a2677ca4104d2d34e867aebaa35d877048/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java#L322-L337

It basically deregisters the channel from some pools/queues and closes the ciphertext file channel.

Even though we explicitly write the last modifed time to storage, it might be changed afterwards because we never flush the ciphertextchannel explicitly before setting the last modified time. Hence, if there are unwritten changes in the channel, it is only written on close, which is after correcting the last modified time, effectively nullifying the correction.

The flush call only regards the chunk cache, which might use other (ciphertext) file channels.

infeo commented 7 months ago

For a better understanding, what happens during close, I created a (simplified) sequence diagram: cryptofs_bug_205_flow-Problem drawio

infeo commented 7 months ago

I discussed this with @overheadhunter today. Since changes to cryptofs have a higher bug/regression risk, we decided to go for the minimal change to resolve the issue: Flush the channel before persisting lastModified. There will be still a small gap between persistLastModified() and ciphertextChannel.close(), but with this fix it is always ensured, that the last closing channel persists the lastModified date correctly. And the lib consumer recieves the in-filesystem lastModified object, e.g. only reads from the encrypted file, if no file channel is open.