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

Cryptkeeper sets the same password "p" for everything independently of user input #23

Open
dmoerner opened this Issue Jan 30, 2017 · 13 comments

Comments

Projects
None yet
9 participants
@dmoerner

dmoerner commented Jan 30, 2017

Hi, there is a serious security hole in cryptkeeper.

Details are in this Debian bugreport: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=852751

Here is a quote from Kirill Tkhai ktkhai@virtuozzo.com, who found this bug:

"I've looked into cryptkeeper code and found, it calls encfs
with -S option:

execlp ("encfs", "encfs", "-S", crypt_dir, mount_dir, NULL);
exit (0);

While the password is passed to encfs using pipe in this way:
// paranoid default setup mode
//write (fd[1], "y\n", 2);
//write (fd[1], "y\n", 2);
write (fd[1], "p\n", 2);
write (fd[1], password, strlen (password));
write (fd[1], "\n", 1);

But it seems it's wrong. When I'm executing encfs program
from console

$ encfs -S crypt_dir mount_dir

and I'm passing "p\n", encfs exits and doesn't wait for a password
itself."

This may be caused by a change in the underlying encfs interface.

@Artoria2e5

This comment has been minimized.

Artoria2e5 commented Jan 30, 2017

According to the simple echo usage at http://jc.coynel.net/2013/09/reading-a-encfs-volume-password-from-a-file/, the p\n step should just be removed.

Hmm, CLI change?

@mhogomchungu

This comment has been minimized.

mhogomchungu commented Jan 30, 2017

I do not think this is a bug in cryptkeeper and cryptkeeper is doing here what everybody else i know does.

The problem is in encfs somehow not behaving as expected in debian.

Below is output that shows how the latest version of encfs works and what cryptkeeper expects

[mtz@ink ~]$ encfs --version
encfs version 1.9.1
[mtz@ink ~]$ encfs -S ~/cipher ~/plain/
Creating new encrypted volume.
Please choose from one of the following options:
 enter "x" for expert configuration mode,
 enter "p" for pre-configured paranoia mode,
 anything else, or an empty line will select standard mode.
?> p

Paranoia configuration selected.

Configuration finished.  The filesystem to be created has
the following properties:
Filesystem cipher: "ssl/aes", version 3:0:2
Filename encoding: "nameio/block", version 4:0:2
Key Size: 256 bits
Block Size: 1024 bytes, including 8 byte MAC header
Each file contains 8 byte header with unique IV data.
Filenames encoded using IV chaining mode.
File data IV is chained to filename IV.
File holes passed through to ciphertext.

-------------------------- WARNING --------------------------
The external initialization-vector chaining option has been
enabled.  This option disables the use of hard links on the
filesystem. Without hard links, some programs may not work.
The programs 'mutt' and 'procmail' are known to fail.  For
more information, please see the encfs mailing list.
If you would like to choose another configuration setting,
please press CTRL-C now to abort and start over.

Now you will need to enter a password for your filesystem.
You will need to remember this password, as there is absolutely
no recovery mechanism.  However, the password can be changed
later using encfsctl.

xxx
[mtz@ink ~]$ 

Notice that encfs asks for two inputs,the first input is either an "x" or a "p" and the second one is the password. Not getting these two requests could only be caused by an internal error in encfs in whatever version of debian this problem is in or maybe a new build time configuration option or a debian specific patch or something else completely that changes the behavior of encfs.

@dmoerner

This comment has been minimized.

dmoerner commented Jan 31, 2017

Hi, this clearly is a bug in cryptkeeper.

Debian, as of release 1.9.1-3, has applied the following bugfix commit from upstream encfs: vgough/encfs@c3a7da5

Cryptkeeper assumed that encfs -S is a stable way to manipulate encfs, which it is not.

You are not seeing this behavior on your system because your distribution has not pulled this upstream bug fix. As soon as a new encfs release comes out, we will see cryptkeeper fail on all major distributions (e.g., Fedora has decided to wait for a new upstream release: https://bugzilla.redhat.com/show_bug.cgi?id=1390107)

@mhogomchungu

This comment has been minimized.

mhogomchungu commented Jan 31, 2017

This does not make me happy.

I have a project called SiriKali[1],its in debian[2] and it interfaces with encfs the exact same way cryptkeeper does it[3] and hence its broken the same exact way.

It is not cool to break interfaces that has been working FOR YEARS and then blame others who depend on those interface.

Cryptkeeper assumed that encfs -S is a stable way to
 manipulate encfs, which it is not.

What documentation made you think this way?

The documentation for "-S" here[4] creates no such impression.

The only thing i can do here is disabling encfs volume creation in SiriKali since i can no longer trust it. Atleast i have words from other projects that they care about backward compatibility and they will not change their API as carelessly as what encfs is doing here.

[1] https://mhogomchungu.github.io/sirikali/

[2] https://packages.debian.org/sid/sirikali

[3] https://github.com/mhogomchungu/sirikali/blob/1c354231a51c5a66c0921552d29221579aacfa75/src/siritask.cpp#L469

[4] https://linux.die.net/man/1/encfs

@rfjakob

This comment has been minimized.

rfjakob commented Jan 31, 2017

EncFS co-maintainer here. Looks like vgough/encfs@c3a7da5 should be reverted as it is an unintentional ABI change. No question about that.

However, please note that "-S" has never been meant to be used when creating a filesystem. From man 1 encfs:

-S, --stdinpass
Read password from standard input, without prompting. This may be useful for scripting encfs mounts.
Note that you should make sure the filesystem and mount points exist first. Otherwise encfs will prompt for the filesystem creation options, which may interfere with your script.

@ailin-nemui

This comment has been minimized.

ailin-nemui commented Jan 31, 2017

that Note just almost sounds like you could use -S also for creation, and just need to take a little extra care

@SammysHP

This comment has been minimized.

SammysHP commented Jan 31, 2017

Since when is piping something in stdin an API or ABI? This is a UI and was never meant to be used by other programs in this way.

But I must say that the different behavior depending on the existence of the filesystem is not that great (although there's a warning in the manual).

@davesteele

This comment has been minimized.

davesteele commented Feb 2, 2017

@Code7R

This comment has been minimized.

Code7R commented Feb 2, 2017

@rfjakob Here the Debian maintainer.

What's your proposal on vgough/encfs@c3a7da5 ? Simply reverting will probably reintroduce the issue that it was trying to fix. I cannot see any other suggestion or pending pull-request anywhere.

BTW, not sure I need to feel bad or sorry here because otherwise that issue would probably slip under the radar right into the next official release.

@rfjakob

This comment has been minimized.

rfjakob commented Feb 2, 2017

What I will do in the weekend is revert the patch and fix the crash in a second commit. Note that the crash is on invalid user input only and nobody noticed for years, so a straight revert is probably good enough.

@rfjakob

This comment has been minimized.

rfjakob commented Feb 5, 2017

@Code7R pull-request to fix this is at vgough/encfs#282

rfjakob added a commit to vgough/encfs that referenced this issue Feb 6, 2017

Merge pull request #282 from rfjakob/issue280
Revert "-S" ABI change

Revert c3a7da5 and enforce empty password ban.

Context:
tomm/cryptkeeper#23
#280
@v6

This comment has been minimized.

v6 commented Apr 2, 2017

// , Is there anything we can do about this issue?

Has anyone forked cryptkeeper?

https://www.theregister.co.uk/AMP/2017/01/31/cryptkeeper_cooked

@davesteele

This comment has been minimized.

davesteele commented Apr 2, 2017

The immediate problem with encfs has been addressed.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=853916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment