Cryptkeeper sets the same password “p” independently of user input
bugs.debian.org
bugs.debian.org
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)
set timeout=1
expect {
"enter \"p\" for pre-configured paranoia mode" { puts p\n }
timeout abort
eof abort
}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.
Almost everything is security-related. Almost nobody is willing to work with this.
(Someday, I hope we have a better cross-language ABI than C.)
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
A CLI provides a better interactive interface, but a terrible programmatic interface.
It is nice in the conceptual sense, because it usually fits the common use cases
I'm going to do some refactoring now
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.
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..
Is it a vulnerability if the product wasn't even working in the first place?
He figured out from the source code that the password was "p", but for many people it'd just mean important data lost.
Deleted comment
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?
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.
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 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.
Does any distro (outside OpenBSD) have a continuous process of re-evaluating included packages?
Do you know whether OpenBSD strives for consistency and throws away stuff when there is a better alternative?
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.
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'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.
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.
The right thing IMHO would be to use pipes & stdin/stdout/stderr, but with a well-defined machine interface.
What you need is a kernel that exposes programs as a file system like plan9
Powershell?
- 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)
It isn't difficult to be strict so being liberal is just asking for trouble uselessly.
- 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?
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.
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).
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:
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);
To quote the linux man page, "The list of arguments must be terminated by a NULL pointer...".
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).
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.
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.
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) 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 g++ compiler is using a gnu extension through the __GNUG__ code path. The "true" C++ code path is at line 11.
Edit:
Answering the person below,the file that defines NULL is "stddef.h"
An example of the file online that defines NULL is here: https://github.com/Alexpux/mingw-w64/blob/d0d7f784833bbb0b2d...
#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.
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).
#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~"
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.
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?
Ycombinator allows neither edits or deletions after a short amount of time it seems.
I guess the lesson is: don't yell at people if you aren't up-to-date on what you are yelling about :)
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...)
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 ?
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");
}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).
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.
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.