128 comments

[ 3.7 ms ] story [ 240 ms ] thread
> But later, when I mounted it once again, cryptkeeper said the password is wrong (though, I 100% know, it's true!).

Is it a vulnerability if the product wasn't even working in the first place?

What if you create a folder, encrypt it, and then never bother to mount it again? You'd think it's safe when it isn't.
Also if you create a folder, encrypt it, and then are completely unable to re-mount it because it says the password is wrong.

He figured out from the source code that the password was "p", but for many people it'd just mean important data lost.

Then you're screwed fucked because you don't know how to decrypt your own data.
(comment deleted)
(comment deleted)
How can you ever know if encryption is working properly?
Steal very important data from the NSA, encrypt it, wait.
But this is certainly not a halting program.... or is it ?
Rubber hose cryptanalysis, if your adversary has not quarrels using it, make sure that your encryption is not strong enough to mandate employing it.
Finding out whether your encryption is strong does require some determination.
Start by trying to decrypt it :o)
Technically, you'll have to formalize and assert the correctness of (at least) the following components:

- All ICs having direct (unlimited) access to memory and/or computation (CPU/DMA/GPU/BIOS)

- Boot loader

- Operating system

- Encryption algorithm

- Encryption implementation

- User-program used to interface with encryption implementation

- Input/output peripherals used for encryption and display

- Probably a dozen things I'm forgetting here

But that's only the system itself. Obviously, you will need to formalize the relationship of the system with the environment. This means: side-channel attacks, such as timing, electro-magnetic radiation, sound and temperature.

Finally, you need to formalize the relationship of the user and whatever is encrypted. The user needs to have an understanding of the limitations of the encrypted channel, the used keys and the algorithms involved: what is the availability, the loss of entropy to attackers and the limitations to what can safely be encrypted?

Short answer is, you don't. That's why people say to not implement your own, but use a widely vetted library.

For some trivial tests against a library or application, you use test vectors calculated by hand. Every crypto algorithm has some published vectors out there. That would discover this flaw, but is not enough by a huge margin. In practice, you don't test crypto software beyond the trivial, you review it and proof correctness against the attacks you know.

Write your own decryption software, see if that software can undo the encryption.

For this to fail one of the following must hold: 1 you made a mistake in your decryption that matches a mistake in the encryption.

2 The encryption scheme allows incorrect cyphertext to decrypt to correct plaintext

3 The encryption software writes more than the supposed cyphertext.

1 requires a mistake on your end, so is hard for malicious entities to abuse. 2 is a feature of the cypher, as such can be defended against in design. 3 can be defended against by observing the encryption software (look for all disk modification and network activity).

the problem with testing encryption in an integration test is that the same input should encrypt to different values each time. this makes testing without some kind of stubbing harder.

in terms of automated tests that could have caught this error:

1). you can create a second implementation and check that it properly decrypts the output from the first implementation and check the first implementation will properly decrypt the output from the second implementation.

2) you can create output from what you believe is a known good program. then write an automated test that would decrypt this output. then write an automated test that would perform an encryption and then a decryption. this would catch a problem where the encryption and decryption routines switch to using a hardcoded string because of a bug.

warning: 2) only works if the bug doesn't exist when you first write the test. :)

3). you allow the random values that making testing encryption difficult to be stubbed out then you can just encrypt plaintext and check it for equality with the ciphertext.

---->

of course there is a lot more issues you should be testing for and google recently released a framework that checks for common implementation errors:

https://github.com/google/wycheproof

This issue highlights another common problem on FOSS:

If I create a small utility "just for fun" and someone includes it in a major distro and then nobody (including myself) touches it for 10 years, who is to blame if there are security (or any other) issues with the software?

Whoever included it in a major distro is to blame.
Yes, but is there any kind of vetting process in any of the major distros?

Does any distro (outside OpenBSD) have a continuous process of re-evaluating included packages?

Since quite some time I'm more than annoyed by the unnecessary complexity and difficulty of traditional distributions caused by inconsistent system tools and backwards compatibility with antique things that block modernization and simplification.

Do you know whether OpenBSD strives for consistency and throws away stuff when there is a better alternative?

They replaced OpenSSL with their own LibreSSL and sudo with doas, among others.
The whole BSD family of operating systems provide far better consistency than the Linux distro universe, as they are the ones to directly maintain the base system's codebase (POSIX utilites, OS-specific utilites, the kernel, the drivers, init system). Linux distros, on the other hand, are just a juxtaposition of software from a plethora of sources.
OpenBSD does not "evaluate" packages (only the base system).
It's a new regression, only found in Debian stretch (not yet released) and caused by an incompatible change in another package. As far as Debian is concerned, this is just the process working as intended.
Unless programmers are treated by the law in a similar way as engineers (and there are reasons for an against that), blaming someone is not a very valuable activity.

A better question is who will fix it and/or whether the maintainer should remove the package. I think such a package should be removed ASAP unless someone fixes the problem within a short amount of time.

I think you are missing my point. What I am saying is "how could this be included in Debian in the first place?"
It wasn't, did you read the article? No stable release ever included this. It was caught while in the testing repository, which is for, you know, testing stuff and finding issues.
Why do we need to blame anyone? Software has a lifecycle too, what was well maintained 10 year ago isn't necessarily maintained anymore because people have moved on. Ten year ago adding this to Debian in its current state might have been the right thing to do. Now, as other components have evolved, it's introduced a problem. If you are going to not include software because ten years down the line it might have a bug and you want to have someone to blame for it you're going to end up with a very empty package repository.

Once the bug is found a ticket gets filed for it, the security teams handle it and life moves on. If the software instead isn't deemed necessary a recommendation is made to remove it from the distribution instead.

Because we need some kind of quality assurance or else we are left with cowboy developers moving fast and breaking things on other peoples machines.
Quality assurance doesn't have to have anything to do with blaming people.
Change the question to "who owned maintenance of the code" and it becomes valid. The kernel of the question is in fact interesting. Distros have far too much software for any reasonable person to assume they maintain all of it.
The whole point of the major distributions - particularly those with a release cycle - is that they do integration testing.

I think two Debian policies interact poorly: heavily modifying upstream sofware, and not doing their own security review. To my mind the Debian SSH key vulnerability was a predictable result of this policy combination (which Debian did not see fit to change AIUI) and similar issues are to be expected from Debian going forward.

The bug here is probably with encfs and most frontend tools to enc are doing exactly what cryptkeeper is doing.

For those who want alternative to cryptkeeper,there is SiriKali[1], next version will be released on Feb 1st and it will have support for OSX.

    execlp ("encfs", "encfs", "-S", crypt_dir, mount_dir, NULL);
That line of code also has a bug as its wrong to pass NULL in C++ to a variadic function. IMHO,usage of NULL in C++ should be strongly discouraged in all cases.

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

Incorrect. execlp requires a NULL to terminate the argument list. How else would the called function know when to stop processing arguments?

To quote the linux man page, "The list of arguments must be terminated by a NULL pointer...".

(comment deleted)
The original paster didn't say that a null pointer was incorrect, merely that using NULL in C++ was. Which, in recent versions of C++ it certainly is. The correct way to specify a null pointer in C++ is 'nullptr', NULL has been deprecated because it's definition '(void*)0' leads to typing issues for templates.
In C++, `NULL` is defined as a simple 0 without the `void*` cast.
No, that's exactly wrong (in this case).

For variadic C functions, you MUST use 0 (or NULL, which is the same as 0). Passing nullptr will case abort() to be called (as is the case whenever you pass a C++ class into a variadic function).

Reply (to myself, based on a correction)

Apologises, I mis-remembered the rule -- it is only C++ types which have a non-trivial destructor which cause an abort when you pass them to a variadic funciton.

HOWEVER. This is still undefined behaviour. nullptr is an object of type std::nullptr_t. When you pass it to execlp, it just gets pushed through as a bit-pattern (as with any type passed to a variadic C function), and there is no guarantee what comes out looks like a "true" (void*)0 C-style null pointer.

There isn't a guarantee that NULL has a bit pattern that's the same as a true (void)0 C-style null pointer either.

I didn't look it up directly but apparently NULL is allowed to be nullptr instead of 0, and nullptr converts to void in varargs arguments.

I think C/C++ also allow actual null pointers to have different bit patterns than the integer zero, so in C++03 passing NULL to terminate a varargs argument list would technically be incorrect, or maybe implementation-defined.

Damn, there is a special exception in the definition of vararg functions, just for nullptr! throws things in the air I give up having discussions about C++.
I give up too, because the HN formatting is actively fighting me.
(comment deleted)
nullptr, I believe, is compatible with C interfaces (as is simply passing 0) while not suffering from the issues described here.
What are the semantic/grammatical/"magical" differences between 0 and NULL?

I understand their similarity in terms of NULL's equivalence to memory address 0 and boolean false, but I'm increasingly aware that there's some hidden behavior.

Does NULL strictly resolve to '(void * )0', or can it be influenced by anything else?

Also, does it behave differently in C vs. C++? I suspect it might.

EDIT: Someone else just replied close to this thread chain just said it's defined in C++ as 0 without '(void * )'. If anyone can expand on this that would be awesome.

sizeof(NULL) in 4 in C++ in a 64 bit system.

sizeof(NULL) is 8 in C in a 64 bit system.

The difference is because NULL in C is a variable of a void pointer type with a value zero. In C++,NULL is a variable with a value zero that gets deduced to an int type.

That's not true. sizeof(NULL) is the same in C++ and C:

https://goo.gl/WL1akj

I looked into this.

#include <iostream>

int main() { std::cout << "sizeof(NULL) = " << sizeof(NULL) << '\n'; }

Compiling with both g++ 6.2.1 and clang++ 3.9.1 with -std=c++11 in both cases gives: sizeof(NULL) = 8

AFAICT, NULL should not even be defined at all in C++ 11; certainly if you include neither <stddef.h> nor <stdlib.h> nor <cstddef> nor <cstdlib>.

I could never get it to compile on x86_64 linux without saying 8.

Bottom line: NULL works in the execlp call, whether from C or from C++. nullptr would be cleaner.

fwiw, in C++11 onwards, apparently NULL is allowed to be nullptr and not an integral constant.
No, that's wrong!

You can't pass nullptr to a variadic function. It causes a run-time call to abort. You can pass 0, or NULL (which is just a macro defined to 0).

That would be pretty surprising if true, but it's not:

#include <unistd.h>

int main() { execlp("ls", "ls", nullptr); }

g++ -std=c++11 -o x x.cpp

"cpp cpp.cpp cpp.cpp~ cryptkeeper q q.c q.c~ q.cpp q.cpp~ x x.cpp x.cpp~"

Apologises, I mis-remembered the rule -- it is only C++ types which have a non-trivial destructor which cause an abort when you pass them to a variadic funciton.

HOWEVER. This is still undefined behaviour. nullptr is an object of type std::nullptr_t. When you pass it to execlp, it just gets pushed through as a bit-pattern (as with any type passed to a variadic C function), and there is no guarantee what comes out looks like a "true" (void*)0 C-style null pointer.

EDIT: On further inspection, both g++ and clang++ on mac (and I'd guess on linux) choose to pass 'nullptr' to variadic functions as an 8-byte all-0 value. That makes sense -- it makes it easier to implement nullptr I guess if you treat it as a null pointer! But, that's not required behaviour by the standard, I could make nullptr internally be any mess of values I like.

>But, that's not required behaviour by the standard, I could make nullptr internally be any mess of values I like.

But compiler writers usually aren't trying to go out of their way to trip you up.

Can you provide an example of a compiler where nullptr doesn't have a 0 bit pattern ?

No, but once you start invoking undefined behaviour, all bets are off.

For example, there are compilers that print nothing for the following function, with uninitialised 'i', whereas some programmers assume that i must take "some value", so one of A and B will be printed.

    int func() {
      int i;
      if(i > 0) printf("A");
      if(i <= 0) printf("B");
    }
> When you pass it to execlp, it just gets pushed through as a bit-pattern

No, it doesn't. It goes through a proper conversion to a pointer.

> [passing nullptr and getting 0 out of va_arg is] not required behaviour by the standard

Yes, it is. See C++11 and C++14 standards, section 5.2.2 paragraph 7.

You've been all over this thread shouting at people about how wrong they were, and even in your correction you are still mistaken. Please try to refrain from that in the future; lots of people reading these threads are going to be very confused about what is technically correct here, and I'm not willing to go reply to all of them.

Perhaps some edits and/or deletes are appropriate?

You are right, I learnt this stuff in a C++0x draft, and it has been notably improved. I'm not discussing C++ minutiae in future, I've learnt my lesson.

Ycombinator allows neither edits or deletions after a short amount of time it seems.

> I learnt this stuff in a C++0x draft, and it has been notably improved.

I guess the lesson is: don't yell at people if you aren't up-to-date on what you are yelling about :)

Yes, yes, I agree.

In my defence, I thought I was up to date. I did know all about how C++ and varargs interact, I was just unaware there was a specific addition to varargs just for nullptr, and I'll be honest it didn't even occur to me to check!

While I agree it's a sensible feature, it's also (for me) been the final straw on giving up on understanding C++ (not using, but understanding...)

(comment deleted)
Wow, so many wrong answers in this subthread.

From the execlp manpage: "The list of arguments must be terminated by a null pointer, and, since these are variadic functions, this pointer must be cast (char *) NULL."

Using bare "NULL" or "nullptr" in this context is wrong (although you might get away with the former on many platforms).

(comment deleted)
A great example of the falsehood that people often repeat mindlessly about "thousands of eyeballs watching the source".
It's actually a great example of why that works, but simultaneously an example of why it's not sufficient
See, that user actually watched the source and readily detected the bug.

I just wonder if thousands, or hundreds, or at least a dozen eyeballs actually watched this source. I suspect 1-2 pairs of eyeballs would detect the problem if a proper code review was done.

Even better, if an automated test existed for this pretty critical piece of code (passing the password around), thousands of eyeballs won't even be needed that badly.

I suspect that just too few people actually use this piece of software. If a large org used it, they probably would have tested this, or even audited the code.

Anyway, it's great that everyone has access to the code, so the bug was detected by a user. For closed-source software, a ticket like this might linger longer for a developer's attention.

Oh if only it was more popular, yeah just like Heartbleed was caught in OpenSSL.

My point was the eyeballs argument is often made as a claim about the absence of bugs at the present time.

When in fact it simply means "it is possible for people to look at it and find and fix bugs" no more no less.

The mere ability does not magically translate to security. And the ability does not automatically imply that it is being done.

The 'p' it sends is asked for by encfs when setting up a new encrypted folder (not when mounting an existing in encfs).

As the comment in the code says, it's for setting the pre-configured "Paranoia" mode (AES, PBKDF2, IV-chaining, etc) in encfs.

Bad that it's not checking any results whatsoever when sending stuff..

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

Yet another example of why command-line interfaces are not safe. Can we start to use something other than arrays of null-terminated strings for input?
What alternative do you propose?
I don't have an easy one. If you can do things in-process, a shared library means type safety. But I don't know what the best practice for IPC (which this is essentially a form of) is.
- Structured data instead of always somewhat wonky text formats

- Request-reply -- Acknowledge stuff. Especially when it's security relevant

- For security relevant stuff, don't just silently fail or fall back to some idiot default. Eg. say a command message for setting a password doesn't contain the password field -- then fail the request, don't assume an empty password.

(Ie. be liberal about what you accept but not too liberal)

You shouldn't be liberal at all if security is a concern. You should be strict, use a well defined format and refuse anything that doesn't comply.

It isn't difficult to be strict so being liberal is just asking for trouble uselessly.

I propose protocol buffers or equivalent.
There are some streams of binary data there too...

It's not THAT bad. That's why everybody is still using it. But there are many better options, since the CLI, eg. people have created distributable libraries, and network APIs. Yet, changing APIs will break any of them.

CLI- as opposed to what ? What other option is there that is easier and safer to implement ?
powershell approach comes to mind - you just pass objects.
Actual libraries.
And then someone wraps it in a CLI interface.

What you need is a kernel that exposes programs as a file system like plan9

That is what the REPL like on Lisp Machines, Interlisp-D, Smalltalk, Mesa/Cedar, Oberon is for.
(comment deleted)
>What you need is a kernel that exposes programs as a file system like plan9

Powershell?

But if it's a C library then there are memory issues, buffer overflows, undefined behaviour — everything that's bad about C.

The right thing IMHO would be to use pipes & stdin/stdout/stderr, but with a well-defined machine interface.

Agreed, which is why C should be treated as the "portable macro assembler" it is and just like Assembly, those libraries should be wrapped in higher level languages.
Passing JSON over stdin and stdout comes to mind.
> CLI- as opposed to what ? What other option is there that is easier and safer to implement ?

A library, with an exposed pubic API that a compiler will, y'know, refuse to compile against when the expected arguments to a call you depend on are completely changed?

Depending on blind CLI calls that silently fail is about the least safe option possible.

There is no "safe" standard, but command line is primarily designed for users, not applications. It almost by definition requires extra scrutiny.

But the issue here was not the command line. It was not handling the interface properly. Somebody wrote a 5 minute hack to handle security functionality and probably didn't even read the application source to look for proper use, much less version checking. The same thing happens with APIs.

And null terminated strings have worked for 40 years, that doesn't have anything to do with this.

This is a breaking API change. The fact that the API was consumed over the CLI is irrelevant, the actual problem is that there was an API and it broke.
Are you sure? Would this not manifest differently if it were, say, a function call?
Well, it might cause a compile error if the function changed. Unless you're using a dynamic language, of course.
Differently, yes. Better, unlikely.

Unless you've got a password type and a regular string type, how's the compiler to know you've swapped the password with a config string?

Joel on Software wrote an interesting article that's related to this, oddly about Hungarian notation.

His point was that *lwistrp = "foo" tells you meaningless stuff (lwip and str) about your data. That's what the compiler is supposed to track. But 'u_pw' and 's_pw' (unsafe/safe) for pre and post-processed user-input tells you something meaningful the compiler has a harder time tracking. (Not impossible with taint-tracking, but ...)

The relevance is that types tend to be based on the implementation details instead of the higher-level facts. For this sort of function to be actually safe, not just compiler-safe, it would need higher-level types and to be compiled into your language (not called via an opaque FFI) to check them.

> It looks as though cryptkeeper makes assumptions about encfs' command-line interface that are no longer valid.

This looks like a developer mistaking a command line interface for an API.

Unless an (interactive) CL interface is explicitly marked as being an API, and documented in such a way, and regression tests exist to make sure the interface remains backwards-compatible - you should never program against it. Ask for a proper API to access.

(edit: formatting)

If you offer a cli with output reasonable to parse as a text stream, assume someone is going to script against it and don't change the args without detecting and warning on the old usage.
Better yet, don't offer a CLI as the primary interface; provide a reasonable library to use instead.
Though, that only works for the language that you use. Other languages may opt to use the CLI as the API
Thinking a bit about this problem, I'm a bit torn

In one side, a CLI offers a "better" way of using the product (as with a library it's easier to misuse something)

On another, issues like this

(But I guess in this specific case the issue is that encfs is not doing a minimal validity check of the password being provided)

It would be good if CLIs would/could be more consistent, but in the Unix/Gnu options world, probably it won't

> In one side, a CLI offers a "better" way of using the product (as with a library it's easier to misuse something)

A CLI provides a better interactive interface, but a terrible programmatic interface.

Yes, it is awful in the programmatic sense, I don't want to assemble command lines by hand or parse error messages

It is nice in the conceptual sense, because it usually fits the common use cases

A CLI is usable by any language that can fork/exec and read/write IO. A library is more likely to be usable by only one language.
A C library, or a library with a C interface, is usable by any language with an FFI.
That's true in theory, but in practice most Ruby developers don't know how to use C or the FFI well enough to build a library out of a C or C++ library.

It's reasonable to shell out sometimes. It's what Github did when they were first starting. It's what 500px did when they were first starting. Trying to do everything the right way early on is just going to slow you down.

I think with security-related software in particular, the proper answer there is "tough". If you can't figure out how to call a C function, you should probably just not write critical software.
One of the important lessons that the profession has yet to fully internalize is that there is very little software that is NOT security-related.
It's very, very difficult to get product managers to internalize that there are real security restrictions on how they can enable users to make things pretty.

Almost everything is security-related. Almost nobody is willing to work with this.

OTOH, a C library will almost certainly be unsafe and insecure, while POSIX pipes can be secure.
Exactly what I had in mind when I said "or a library with a C interface".

(Someday, I hope we have a better cross-language ABI than C.)

its not up to the developer to guess all the wrong ways[1] users are using the software. such changes will be described in the changelog, I expect you read that if you are updating the software.

1. https://xkcd.com/1172/

It's worth noting that what is arguably the largest and most successful software project in history, Microsoft Windows, has succeeded in large part because of an obsession with maintaining backwards compatibility, even with the "wrong" ways of using the software.
I agree, but if a script is going to use the CLI then it's the script's responsibility to parse the prompts and not to blindly send strings to the program. `expect` exists since 1990, use it or reinvent it.
Can you go over some good ways to use expect to avoid this type of issue?
You simply use it to wait for the appropriate prompt before sending anything. If the expected prompt never arrives, something is wrong (CLI changed), so abort. The snippet for this particular case could be something like

    set timeout=1
    expect {
      "enter \"p\" for pre-configured paranoia mode"  { puts p\n }
      timeout     abort
      eof         abort
    }
(comment deleted)
expecially for a tool intended for those use cases as encfs.
Thanks for posting this. I was working on a chrome native messaging extension to talk to GPG... and I was just using the CLI. You're right, that's not the right solution, even if learning their API is more work.

I'm going to do some refactoring now

Maybe implementing a mandatory '--json' flag to all our favorite CLIs might solve the issue of parseable outputs once and for all.
It should at least have been using `expect`!
Please tell me this isn't a case where Debian has "improved" something crypto and broken it.
This isn't such a case. Cryptkeeper is old and crufty.
If you'd read the article you could have answered the question yourself: no, it is not.

But why let facts get in the way of some easy karma-whoring?

What bothers me is how it got into 'testing' without any actual testing. This tells something about processes in Debian.
testing is _for_ testing, you know. Testing happens there, not earlier.
Ouch. For a long time this form of error was the #1 cause of system breakage in SunOS. Someone would go in and change an option or add a 'version' line that would print before the output, or re-order how you typed args, and blam! a bunch of bug reports would come flying in what some program was broken (sometimes commercial programs where now source was available) because they had system('foo bar bletch'); in them somewhere.

Very hard to test for as there weren't explicit dependencies and no amount of 'don't count on this output staying the same' warning messages helped.