Skip to content
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

Allow deterministic diriv creation via cli flag #592

Closed
wants to merge 1 commit into from
Closed

Allow deterministic diriv creation via cli flag #592

wants to merge 1 commit into from

Conversation

Kuroneer
Copy link
Contributor

This PR introduces a new flag (-deterministicdirivcreation), which generates the diriv from the first bytes of the sha256 of the plaintext path.

I'm not very familiar with go and I was unable to run the tests (old ubuntu, I tested it by hand compiling it with docker run -u $(id -u):$(id -g) -v $PWD:/source -w /source -e HOME=/source golang ./build-without-openssl.bash). That's why I'm initially leaving this as a draft. I hope that the github actions will be able to test it.

@Kuroneer
Copy link
Contributor Author

I'll squash all commits eventually once everything is ok

@rfjakob
Copy link
Owner

rfjakob commented Aug 13, 2021

I kinda like this, with the exception that you leak the unencrypted sha256 of the plaintext path. That means looking at the gocryptfs.diriv files an attacker can know if you do or do not have the folder Secret_CIA_Files (for example) in the top directory.

Can be fixed, but, I wonder if having all diriv all-zero would be good enough for you usecase? That would also fix the problem with the dirivs not being recalculated on mv.

@Kuroneer
Copy link
Contributor Author

Kuroneer commented Aug 13, 2021

I kinda like this, with the exception that you leak the unencrypted sha256 of the plaintext path. That means looking at the gocryptfs.diriv files an attacker can know if you do or do not have the folder Secret_CIA_Files (for example) in the top directory.

That could be solved by salting the sha, either by reverting the change to the root diriv initialization and using the root diriv as salt everywhere or by allowing the cli flag to provide a salt. In this case I'd go with using a random root diriv as salt. I don't believe that having a deterministic diriv for the root is as critical (and what I'm trying to solve here -the issue mentioned in #108- won't be as relevant for the root dir because you usually clone that directory the first time and that's it)

Can be fixed, but, I wonder if having all diriv all-zero would be good enough for you usecase? That would also fix the problem with the dirivs not being recalculated on mv.

That being said, yes, any kind of deterministic diriv would be good enough. It's a simpler solution, cleaner and wider. Would you be more keen to that approach?

EDIT: Another possibility would be using the encrypted name of the directory instead of the plain name (the parent path would still be clear, though, because of needing fuse's node.Path()), by involving the key you fix the leak

@rfjakob
Copy link
Owner

rfjakob commented Aug 13, 2021

Yes salting the sha would fix the leak, multiple ways to do it, should be no problem.

But about the all-zero dirv:

Secret_CIA_Files would still be encrypted, so I don't see how we would leak if you do or do not have it.

@rfjakob
Copy link
Owner

rfjakob commented Aug 13, 2021

Some background info: i have been thinking about how to have something fully deterministc for a while, best i came up with is

  • Aes-siv encryption, all-zero block ivs
  • Filename encryption on but with all-zero dirivs

@Kuroneer
Copy link
Contributor Author

Secret_CIA_Files would still be encrypted, so I don't see how we would leak if you do or do not have it.

Yes, I realized that late, my bad

@Kuroneer
Copy link
Contributor Author

I'll change this PR to have all-zero diriv then, not an issue (but it'll be around next week, I'll be AFK for the weekend). And I'll do something to be able to run the tests locally so I don't need to rely on the github actions

@Kuroneer Kuroneer marked this pull request as ready for review August 19, 2021 12:53
@rfjakob
Copy link
Owner

rfjakob commented Aug 19, 2021

Oh, it's incompatible with earlier gocryptfs versions because of the allzero sanity check.

Change is good, I will merge this, but will extend it to not write the diriv to disk at all. Or do you want to give it a try?

@rfjakob
Copy link
Owner

rfjakob commented Aug 19, 2021

I'm already working on it.

@Kuroneer
Copy link
Contributor Author

I went with the easiest route, true. I also thought that it'd be better to have compatibility when mounting with or without the flag. I guess previous versions went above my head when I was required to remove the zero iv check.

It opens the following questions:
Should the flag only prevent diriv creation or reading the dirivs altogether? (my guess is that it should only prevent creation to allow the same system to work both with and without the flag)
In that case, missing dirivs should/would be treated as all-zero dirivs, in order to support the previous statement (and the flag would be irrelevant when reading a directory)

@rfjakob
Copy link
Owner

rfjakob commented Aug 19, 2021

My plan now is to require the flag on -init, and store it in gocryptfs.conf, because a user may forget specifying the flag sometimes, and then you'll end up with an unholy mix. Then, not read or write any gocryptfs.diriv files.

@rfjakob rfjakob closed this in f6be765 Aug 20, 2021
@rfjakob
Copy link
Owner

rfjakob commented Aug 20, 2021

Merged to master, thanks!

@Kuroneer Kuroneer deleted the deterministic-diriv-on-creation branch August 24, 2021 11:09
@dumblob
Copy link

dumblob commented Oct 16, 2021

My plan now is to require the flag on -init, and store it in gocryptfs.conf, because a user may forget specifying the flag sometimes, and then you'll end up with an unholy mix. Then, not read or write any gocryptfs.diriv files.

It seems the flag -deterministic-names is not meant to be the default. I'm new to this, but I'd like to ask why is it not the default?

@rfjakob
Copy link
Owner

rfjakob commented Oct 16, 2021

Because it is less secure than randomized names

@dumblob
Copy link

dumblob commented Oct 16, 2021

@rfjakob I see, thanks. Though I still fail to build any more tangible understanding of the measure "how much less secure it is". Would you have any pointers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants