password generation must be more secured

[eanyx]

After creation of a new user or modification of password of an existing account, the generated hashed password in /etc/shadow is too simple (can be cracked esaily).

Need more secured hashing algorithm + salt.

[eanyx]

haiku /etc/shadow

[waddlesplash]

Multiuser security is a R2 problem.

[pulkomandy]

We use the "R2" milestone only for things that can't be done at all in R1 (need API changes, or the like). For things that could be done in R1 but we plan to do in R2, we use the "Unscheduled" milestone.

[axeld]

Since when do we use this strange logic, Adrien?

[pulkomandy]

Since I created the R2 milestone and assigned the 13 tickets needing API changes to it. Before that there was only the Unscheduled branch.

[i80and]

Added a draft patch that modifies crypt() to have three modes:

• If the salt is less than 16 bytes long, crypt() will use the current UFC crypt() implementation.
• If the salt is NULL, then crypt() will generate a random salt string using /dev/random, and continue into...
• If the salt is a string matching the format "$s$%2u$%64s$", then crypt() will apply the scrypt KDF function using the first parameter as log2(N) (defaults to 14, requiring 16 MB of RAM), and the second parameter as a 32-byte salt.

It will return a string in the format "$s$%2u$%64s$64s", appending the 32-bit scrypt output to the end of the salt.

This patch additionally tweaks passwd and screen_blanker, and fixes #4812 in my tests.

Issues with this patch:

• I wrote a test, but I'm not sure if I integrated it correctly.
• Ideally the insecure_memzero would be a systemwide bzero_explicit or memset_s something.
• Hex conversion functions should probably be shared. Possibly split into a separate patch?
• It's big. I can break it down if desired.

[axeld]

Thanks for the patch! It looks good for the most part, though I have a few comments/questions:

• crypt.cpp doesn't really follow our coding style yet.
• Why does libroot link against shared now?
• It looks like we should update AboutSystem to refer to the license.
• Doesn't this make logging in very slow? I would find 5 seconds inconvenient enough to prefer to go with bcrypt instead.
• Is this already scrypt 1.2 from August 2015?
• The test looks fine; it's as integrated as the rest of it. Ideally, it would be part of our unit test suite, though.
• Don't we already have a number of SHA 256 computation code in our tree that could be reused (or replaced if slower)?

Thanks for working on this!

[i80and]

• crypt.cpp doesn't really follow our coding style yet.
  • OK, I'll take a closer look at styling.
• Why does libroot link against shared now?
  • It needs access to the SHA256 implementation in shared. Is there a better way to do this?
• It looks like we should update AboutSystem to refer to the license.
  • I'll add that.
• Doesn't this make logging in very slow? I would find 5 seconds inconvenient enough to prefer to go with bcrypt instead.
  • On a VM running in my laptop, this crypt() takes real 0.105 seconds. Where did you get 5 seconds?
• Is this already scrypt 1.2 from August 2015?
  • Yup! Changed to use the in-tree SHA-256 implementation, and to compile as C++.
• The test looks fine; it's as integrated as the rest of it. Ideally, it would be part of our unit test suite, though.
  • I'll take a stab at using the unit test stuff. Any particular example I should use?
• Don't we already have a number of SHA 256 computation code in our tree that could be reused (or replaced if slower)?
  • Yup, this uses that code. I removed the SHA-256 implementation from sha256.c and sha256.h, so they're just PBKDF2 functions. I should rename them.

[i80and]

scrypt_v2 changelog:

• Updates style in crypt.cpp
• Removes sysendian.h in favor of ByteOrder.h
• Uses TestUnitUtils.h instead of hand-rolled verify() macro in test_crypt.cpp
• Adds scrypt credit in AboutSystem.cpp

[axeld]

Thanks for the update!

Ah, you tricked me with the sha256 files :-) Would be great if you could rename them appropriately. Linking against shared then makes a lot of sense.

I got the five seconds from the scrypt website, but maybe I misread that part ("We estimate that on modern (2009) hardware, if 5 seconds are spent computing a derived key, the cost of a hardware brute-force attack against scrypt is roughly 4000 times greater than the cost of a similar attack against bcrypt (to find the same password), and 20000 times greater than a similar attack against PBKDF2.").

A good example for the unit tests would be (sorry, I can only point you to files that I wrote myself with that introduction ;-)) the tests for ​libnetapi.so to name one. You seem to have added the test twice now accidentally, under two different names.

Last but not least, I still have some coding style remakes for crypt.cpp, although it has greatly improved since the last patch (we do take our coding style very serious, sorry for that):

• Copyright: did you really write that between 2007 and 2009? :-)
• Two blank lines after the copyright header.
• The include order is correct, but we use blank lines between the different sections; ie. one after the POSIX headers, and one after SupportDefs.h.
• Line 38: it seems it wouldn't hit the 80 character per line limit if you did not break that line in two.
• buf_len, buf, outbuf, outbuf_len aren't following our naming style for variables. That would be: bufferLength, buffer, outBuffer, and outBufferLength respectively (we use camel case).
• 'i' as index is allowed, but since outi or (camel case) outI looks rather terrible, I'd actually use 'index' and 'outIndex'.
• Line 72 (and other places): we don't use curly braces for single line statements (but even a newline would make it a two line statement).
• Line 99 (and other places): we use doxygen style comments for functions, so that would be:

  /*! Generate a new salt appropriate for crypt().
  */
  

  or even (since it's just a single line):

  //! Generate a new salt appropriate for crypt().
  

  Note, there is no blank line between the comment and the function it belongs to.
• Line 104: Opening curly braces go to the next line for functions (as you noticed at other places).
• Line 117, line 135: Inconsistent asterisk style.
• N_log2 would be nLog2.
• But that should be all.

[i80and]

Updated!

• More style tweaks! I changed N_log2 to NLog2 rather than nLog2, because it's referring to the scrypt parameter N. I think changing the case of the parameter name would make things muddier. But of course, I'll change that if required.
• Thanks for the help with the unit test system! :D It seems to work!

Regarding the scrypt website blurb --- you can customize scrypt to take different amounts of time and memory. The parameter N=14 is recommended for interactive systems, which shouldn't take longer than 250ms even on older hardware. But for things like FDE or file archival encryption, you would crank up N and you might well get up to 5 seconds.

[axeld]

Great, thanks! Since 'NLog2' is a variable, it should start with lower-case 'n' -- for constants, the UPPER_CASE_STYLE is tolerable, although kConstant is preferred; so using 'N' there is okay. But I also wouldn't mind to lose the const and call it 'n' to make the two combine better :-)

You've introduced a number of new coding style violations in the tests (most notably in LibRootPosix.cpp), but I'd be okay with committing it as is as part of the tests, if you're okay with that.

[i80and]

Updated those two variables; fixed some style issues in the tests; and regenerated with git format-patch.

[i80and]

Reading 1003.1-2008, it looks like my patch is noncompliant in several ways. I should have read this before.

Sorry :/ I'll make this POSIX-compliant.

[i80and]

scrypt_v5 replaces the crypt(..., NULL) interface with a new crypt_gensalt(int hardness) function. POSIX states that

The salt argument shall be a string of at least two bytes in length...

implying to me that it's not acceptable for salt to be NULL, necessitating this new interface.

[axeld]

We don't have to follow POSIX by the word; we already have quite a few functions that deviate from the standard, and, for example, accept NULL as an argument where POSIX states otherwise.

As long as it's an addition, and doesn't affect programs that follow the POSIX standard, I fail to see any problems; the code we have now is not portable in either case; when using on another system, it may choke on a NULL salt, or misses the crypt_gensalt() function.

IOW I don't think we need that separation, and clobber the global namespace. If v4 and v5 are otherwise identical, I'd prefer to apply v4.

[axeld]

Parsing of the salt string is also not exactly POSIX, btw :-)

[i80and]

It seems to leave parsing of the salt string unspecified; it just has to be at least two bytes :D

v6 is just like v4, with two changes:

• Two spaces after license in crypt.cpp,
• If crypt_gensalt() returns NULL, then crypt() also returns NULL rather than trying to call strlen() on a NULL pointer.

[axeld]

Thanks a lot! I've applied your patch in hrev50881.