-
Notifications
You must be signed in to change notification settings - Fork 258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Badname file content access #568
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks! Before I review the code in detail, the git situation is a little complicated because you have merges inside your pull request. Can you:
-
Merge with latest master (I checked and it merges without conflicts, then there is one small change needed (build error) because
EncryptName
now returns an error value). -
Squash all changes to a single commit ( here is how to do it without re-doing all the merges: https://stackoverflow.com/a/67041865/1380267 )
One code comment already:
- In TestBadname, enable the "Case 7" test by setting wpanic=0 like done here:
gocryptfs/tests/cli/cli_test.go
Line 402 in 98ab59c
func TestMountPasswordIncorrect(t *testing.T) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add
EncryptAndHashBadName
to the NameTransformer interface so you can remove allrn.nameTransform.(*nametransform.NameTransform)
type assertions
Yeah sorry about that. I messed up my master branch since I accidentally committed on that branch.
I'm not quite sure if I get what you mean.
I edited the CLI test. Thanks for the hint - I was searching for that. |
This proposal is the counterpart of the modifications from the `-badname` parameter. It modifies the plain -> cipher mapping for filenames when using `-badname` parameter. The new function `EncryptAndHashBadName` tries to find a cipher filename for the given plain name with the following steps: 1. If `badname` is disabled or direct mapping is successful: Map directly (default and current behaviour) 2. If a file with badname flag has a valid cipher file, this is returned (=File just ends with the badname flag) 3. If a file with a badname flag exists where only the badname flag was added, this is returned (=File cipher name could not be decrypted by function `DecryptName` and just the badname flag was added) 4. Search for all files which cipher file name extists when cropping more and more characters from the end. If only 1 file is found, return this 5. Return an error otherwise This allows file access in the file browsers but most important it allows that you rename files with undecryptable cipher names in the plain directories. Renaming those files will then generate a proper cipher filename One backdraft: When mounting the cipher dir with -badname parameter, you can never create (or rename to) files whose file name ends with the badname file flag (at the moment this is " GOCRYPTFS_BAD_NAME"). This will cause an error. I modified the CLI test function to cover additional test cases. Test [Case 7](https://github.com/DerDonut/gocryptfs/blob/badnamecontent/tests/cli/cli_test.go#L712) cannot be performed since the cli tests are executed in panic mode. The testing is stopped on error. Since the function`DecryptName` produces internal errors when hitting non-decryptable file names, this test was omitted. This implementation is a proposal where I tried to change the minimum amount of existing code. Another possibility would be instead of creating the new function `EncryptAndHashBadName` to modify the signature of the existing function `EncryptAndHashName(name string, iv []byte)` to `EncryptAndHashName(name string, iv []byte, dirfd int)` and integrate the functionality into this function directly. You may allow calling with dirfd=-1 or other invalid values an then performing the current functionality.
Hmm, the merged commit doesn't look good: I did this:
and the result is here: https://github.com/rfjakob/gocryptfs/tree/pull568-squashed Anyway, no need to redo this, you can just continue on pull568-squashed. You can then create a new pull request, or (preferred), force-push into your |
About the |
Phew! I think that's it. Can you check the current |
Merged as a611810 , thanks! |
This proposal is the counterpart of the modifications from the
-badname
parameter. It modifies the plain -> cipher mapping for filenames when using-badname
parameter. The new functionEncryptAndHashBadName
tries to find a cipher filename for the given plain name with the following steps:badname
is disabled or direct mapping is successful: Map directly (default and current behaviour)DecryptName
and just the badname flag was added)This allows file access in the file browsers but most important it allows that you rename files with undecryptable cipher names in the plain directories. Renaming those files will then generate a proper cipher filename
One backdraft: When mounting the cipher dir with -badname parameter, you can never create (or rename to) files whose file name ends with the badname file flag (at the moment this is " GOCRYPTFS_BAD_NAME"). This will cause an error.
I modified the CLI test function to cover additional test cases. Test Case 7 cannot be performed since the cli tests are executed in panic mode. The testing is stopped on error. Since the function
DecryptName
produces internal errors when hitting non-decryptable file names, this test was omitted.This implementation is a proposal where I tried to change the minimum amount of existing code. Another possibility would be instead of creating the new function
EncryptAndHashBadName
to modify the signature of the existing functionEncryptAndHashName(name string, iv []byte)
toEncryptAndHashName(name string, iv []byte, dirfd int)
and integrate the functionality into this function directly. You may allow calling with dirfd=-1 or other invalid values an then performing the current functionality.