ecryptfs-simple: KISS directory encryption

Xyne · 2016-11-14 08:24:00

mhogomchungu wrote:

My use case generates two keys,"ecryptfs_fnek_sig" and "ecryptfs_sig" and I have noticed that these keys are sometimes the same and other times different and i think you should check for both of them.
I think the best place to get these keys is from volume's mount options as they show up in "/proc/self/mountinfo". Your current way of finding them will not work if somebody used custom configuration path(opened the volume with --config option,SiriKali does this)

I grabbed the key from the config because it was quick and easy and I didn't have the time to mess around with processing the system's mount info. The custom configuration path is not an issue (for me) at the moment because the option states its assumptions in the help message. Obviously parsing the system info is the correct solution. I don't know when I will have the time so feel free to code it.

mhogomchungu wrote:

Why do you still use strcpy? Looking at below code for example
  if (arguments.mount_options != NULL)
  {
    strcpy(opts_str, arguments.mount_options);
  }
  else
  {
    opts_str[0] = '\0';
  }
That looks like its taking a string of arbitrary size from users and copying it to a buffer of fixed size,the buffer just begs to be abused. Where are you checking that the buffer is large enough to hold the "arguments.mount_options"?. If it is there then i did not see it with amount a minute of looking around and i shouldnt have to do that since "snprintf" would have carried all the checks right there with zero cost on your part.

Did you consider that I may have simply missed it when updating the code? You missed a few potential overflows in your own updates. It happens. I'll fix it when I have time. Incidentally, this is why I was hesitant to jump back into code that I haven't touched in 4 years. If I had to start over, I would write it quite differently.

I'm rediscovering the code as I look it it: the buffer size is set to the size of the const input string. As the comment above says, it's only to make that string mutable for the option cleaner function, which will only shorten it. Again, I see a better way to do it, but overflows don't seem to be an issue right now.

mhogomchungu · 2016-11-14 09:34:27

Your current code fail to build in ubuntu and debian and snprintf changes here[1] solves the problem,other changes silences warnings.

[1] https://github.com/mhogomchungu/ecryptf … ab6a1f259d

Did you consider that I may have simply missed it when updating the code?

No,i did not. I though you intentionally left the first one behind because it was used where the destination buffer was just as large as the source and hence its use is guaranteed to be safe. With this thinking,i went looking for where the same guarantee in the second use of it,did not see it and i commented on it.

There is a buffer overflow bug here.

  // The string needs to be mutable for cleaning, so allocate memory.
  if (arguments.excluded_options != NULL)
  {
    excluded_options = realloc(
      excluded_options,
      strlen(arguments.excluded_options) * sizeof(char)
    );
    if (excluded_options == NULL)
    {
      die("error: failed to allocate memory\n");
    }
    strcpy(excluded_options, arguments.excluded_options);
    clean_opts_str(excluded_options);
    debug_print("excluded options: \"%s\"\n", excluded_options);
  }

strlen will give a value that did not count the NULL character causing the allocated buffer to be one character short and strcpy will add the NULL character pass the end of the allocated buffer. snprintf will just truncate the last character giving less serious different bug.

The size of the destination buffer needs to be one character larger than the size of the source as reported by strlen().

Last edited by mhogomchungu (2016-11-14 10:37:56)

Xyne · 2016-11-15 06:48:50

I've implemented the suggested changes along with the following:

Added an inline "copy_string" function to check for string truncations while handling potential overflows.
Fixed some bugs in string handling.
Added support for unlinking keys based on mount options. In my last reply, I didn't realize that the necessary code was already there in the mount functions
Changed debug message formatting.
Probably more.

I searched a bit for best practices for ignoring unused parameters and return values (which I've honestly never bothered with before). My understanding is that casting to void was universally supported before but no longer is with GCC since <insert different versions here based on different posts>. Is there a standard? I noticed that you use a conditional for return values and used it previously for parameters. Is there a reason to prefer that for both?

Btw, if I missed any other overflows, just point me to the line. I already understand the concept of null-terminated strings and allocated space

mhogomchungu · 2016-11-15 15:00:36

Your latest changes does not work and i have made changes here[1] to make it work. Explanations for the changes are follows:

1. You were calling ecryptfs without having privileges, a solution i went with is to raise them,call ecryptfs and then drop them afterwards.

2. This is not a bug but it makes the code looks nice and doestnt raise questions about your C proficiency, a pointer of type "void *" can be assigned to a pointer of any other type making explicit casting unnecessary and i removed them.

3. Your code was unmounting a volume twice and it was checking for keys on the second unmount and failed to found them because the volume was already unmounted. I removed the second mount and moved up the code that looked for keys.

At some point i noticed that casting to void worked for variables but not to functions return values necessitating a different method. I do not remember how but i stumbled upon the conditional trick and i have been using it ever since because it works for everything. I do not know of any standard way of silencing both used variables and return values.

Some compilers from some distributions care about different return values and its not easy to know them in advance until you build your problem with a compiler that cares about them and i noticed compilers from debian are the ones that warns about them.

Its a person policy to build C/C++ projects in a way that does not generate any warnings and your project right has generates two only warnings as seen here in debian 7 build. Other bugs i reported where caused by a failed build on opensuse. Its always nice to build against different distro because different distro care about different things and they fine tune their compiler options according to what they care about.

I think a robust code base is the one that build without warnings using different compilers from different distributions.

With the above changes,there is still one more bug,i just created a volume and below is the content of the created config file

ecryptfs_sig=1b77138d91206e66,ecryptfs_fnek_sig=1b77138d91206e66,ecryptfs_cipher=aes,ecryptfs_key_bytes=32,ecryptfs_passthrough,ecryptfs_enable_filename_crypto=y

ecryptfs_passthrough should be either "ecryptfs_passthrough=y" or "ecryptfs_passthrough=n"

[1] https://github.com/mhogomchungu/ecryptf … 9084a128d2

[2] https://build.opensuse.org/package/live … n_7.0/i586

Last edited by mhogomchungu (2016-11-15 15:42:13)

Xyne · 2016-11-16 00:10:05

I have implemented most of the changes.

When/why is it necessary to have privileges when invoking ecryptfs_get_version or ecryptfs_process_decision_graph? The admittedly limited testing that I have done here indicates that it works without privileges. If it really is necessary then I will rearrange some lines in prompt_parameters to only enclose the necessary calls. For now I have omitted those changes.

The double unmounting was an unintended consequence of checking both sources and targets* when unmounting (so the command can be invoke with either). The duplication of the loop was clearly noobish. My new solution is to check for both in the same loop using mnt_match_source_or_target.

The inclusion of ecryptfs_passthrough by default was a mistake. According to the current man page, that option is off by default and does not accept a value. If previous versions require ecryptfs_passthrough=n then I will try to add a version check to the code for backwards compatibility.

I agree with your remarks about what constitutes a robust code base and I appreciate technical correctness and the opportunity to develop my own skills. When I have more time, I intend to improve multiple aspects of my projects (public Git repos, testing repo, unit tests, etc.).

* Which shouldn't result in a double-unmount as the target and source should always differ, no?

mhogomchungu · 2016-11-16 03:57:55

When/why is it necessary to have privileges when invoking ecryptfs_get_version or ecryptfs_process_decision_graph? The admittedly limited testing that I have done here indicates that it works without privileges. If it really is necessary then I will rearrange some lines in prompt_parameters to only enclose the necessary calls. For now I have omitted those changes.

It fails here with below error:

[mtz@ink ecryptfs-simple]$ ecryptfs-simple -a -c 1/zzz 1 2
Mounting /home/mtz/build/ecryptfs-simple/1 on /home/mtz/build/ecryptfs-simple/2
Passphrase: error: option prompt failed
No such file or directory
[mtz@ink ecryptfs-simple]$

system log reports the following error

[mtz@ink ~]$ cat /var/log/syslog
Nov 16 06:48:31 ink ecryptfs-simple: Could not open library handle
Nov 16 06:48:31 ink ecryptfs-simple: Could not open library handle
Nov 16 06:48:34 ink ecryptfs-simple: do_hash: PK11_HashBuf() error; SECFailure = [-1]; PORT_GetError() = [-8128]
Nov 16 06:48:34 ink ecryptfs-simple: Error generating passphrase signature; rc = [-22]
Nov 16 06:48:34 ink ecryptfs-simple: ecryptfs_add_passphrase_key_to_keyring: Error attempting to generate the passphrase auth tok payload; rc = [-22]
[mtz@ink ~]$

The problem also affects arch users(not on arch at the moment) as reported in the first comment here: https://aur.archlinux.org/packages/ecryptfs-simple/

"ecryptfs_process_decision_graph" function needs to run elevated.

If previous versions require ecryptfs_passthrough=n then I will try to add a version check to the code for backwards compatibility.

No,i did not know that the option does not take options,i just assumed that it does,i should have checked before commenting about this.

Your newest code(2016.11.15.3) introduced a bug,"-k" does not work if not accompanied with "-u"

  if (arguments.unmount)
  {
    unmount_simple(source_path, arguments.unlink, &unlinked_keys);
    if (arguments.unlink && !unlinked_keys)
    {

should be:

if (arguments.unmount || arguments.unlink) <------------------------ required change is here
{
unmount_simple(source_path, arguments.unlink, &unlinked_keys);
if (arguments.unlink && !unlinked_keys)
{

Last edited by mhogomchungu (2016-11-16 04:27:20)

Xyne · 2016-11-16 05:03:02

Fixed and fixed.

mhogomchungu · 2016-11-22 05:05:04

Found a problem with the latest version.

[mtz@ink ~]$ ecryptfs-simple -a -c 1/xxx 1 2
Mounting /home/mtz/1 on /home/mtz/2
Select key type to use for newly created files:
1) openssl
2) passphrase
Selection: 2
Passphrase: Enable plaintext passthrough (y/n) [n]:
Saving to 1/xxx
[mtz@ink ~]$

I do not think the question of whether plaintext passthrough should be enabled or not should be asked when unlocking a volume that already has a configuration file.

I do not think "Saving to XYZ" line should be there when unlocking a volume that already has a configuration file.

My fork currently has two additions.
1. The config file is created with a new line at the end.This addition makes it easy to read the config file using "cat" command[1].
2. Its now possible to unlock a volume in read only mode using "--readonly"[2].

[1] https://github.com/mhogomchungu/ecryptf … 7e05c27678
[2] https://github.com/mhogomchungu/ecryptf … 651498f49f

Last edited by mhogomchungu (2016-11-28 09:24:31)

Arch Linux

#26 2016-11-14 08:24:00

Re: ecryptfs-simple: KISS directory encryption

#27 2016-11-14 09:34:27

Re: ecryptfs-simple: KISS directory encryption

#28 2016-11-15 06:48:50

Re: ecryptfs-simple: KISS directory encryption

#29 2016-11-15 15:00:36

Re: ecryptfs-simple: KISS directory encryption

#30 2016-11-16 00:10:05

Re: ecryptfs-simple: KISS directory encryption

#31 2016-11-16 03:57:55

Re: ecryptfs-simple: KISS directory encryption

#32 2016-11-16 05:03:02

Re: ecryptfs-simple: KISS directory encryption

#33 2016-11-22 05:05:04

Re: ecryptfs-simple: KISS directory encryption

Board footer