Topics

First impressions on CII Best Practices and badges -- part 3


Kevin W. Wall
 

Okay, this is a question about interpretations of the 'crypto_password_storage'
question.

The question I am referring to was this:
If passwords are stored for authentication of external users, the project
MUST store them as iterated hashes with a per-user salt by using a key
stretching (iterated) algorithm (e.g., PBKDF2, Bcrypt or Scrypt).
[crypto_password_storage]

I answered it as 'Unmet' with the following explanation:
We don't store passwords per se, but there is a toy Authentication reference
implementation that does so. It really was never intended for real use (and
trust me, I really rebelled at making it a part of ESAPI from the get go,
but at the time, I lost). Fortunately, I doubt it is SO LAME, that it is
doubtful anyone is actually using it. (I wanted to replace it with a
reference LDAPAuthentication model, but lost the argument; maybe some day.)

In ESAPI, we have an Authenticator interface. We also have real reference
implementations except for a few interfaces (specifically, Authenticator
and AccessController) which only have toy implementations. Those toy
implementations however were needed for JUnit testing of some of the
JavaEE servlet filters that use the Authenticator and AccessController
interfaces. The toy implementation class that we have for the 'Authenticator'
is FileBasedAuthenticator and other than for trivial testing and simply
providing an implementation example, it is not expected that anyone would
actually use it. That said, there is a possibility that someone _could_.
That is why I answered 'Unmet', but I'm thinking perhaps 'N/A' would be
more appropriate because the OWASP ESAPI project itself does not store any
actual user passwords.

So, how do you recommend answering that question? As 'Unmet' with the
explanation that I did, or as 'N/A' stating that we do not store any
user passwords as part of the ESAPI project?

Thanks,
-kevin
--
Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall
NSA: All your crypto bit are belong to us.


David A. Wheeler
 

Kevin W. Wall:

Okay, this is a question about interpretations of the 'crypto_password_storage' question.
The question I am referring to was this:
If passwords are stored for authentication of external users, the project
MUST store them as iterated hashes with a per-user salt by using a key
stretching (iterated) algorithm (e.g., PBKDF2, Bcrypt or Scrypt).
[crypto_password_storage]
I answered it as 'Unmet' with the following explanation:
We don't store passwords per se, but there is a toy Authentication reference
implementation that does so. It really was never intended for real use (and
trust me, I really rebelled at making it a part of ESAPI from the get go,
but at the time, I lost). Fortunately, I doubt it is SO LAME, that it is
doubtful anyone is actually using it. (I wanted to replace it with a
reference LDAPAuthentication model, but lost the argument; maybe some day.)
Sounds like something trivially fixable. Why not just replace that reference implementation
with a call to a PBKDF2 or Bcrypt library, and declare victory? It's just a few lines of code.

In ESAPI, we have an Authenticator interface. We also have real reference implementations except for a few interfaces (specifically, Authenticator and AccessController) which only have toy implementations. Those toy implementations however were needed for JUnit testing of some of the JavaEE servlet filters that use the Authenticator and AccessController interfaces. The toy implementation class that we have for the 'Authenticator' is FileBasedAuthenticator and other than for trivial testing and simply providing an implementation example, it is not expected that anyone would actually use it. That said, there is a possibility that someone _could_.
Hmm. If it's only for testing, perhaps it could be renamed and documented as being for "test purposes only", then you're fine. The intent was that this requirement was for user functionality. *Lots* of test code has to temporarily disable things for purposes of tests, but we want to make sure that users don't end up using dangerous code.

That is why I answered 'Unmet', but I'm thinking perhaps 'N/A' would be more appropriate because the OWASP ESAPI project itself does not store any actual user passwords.
So, how do you recommend answering that question? As 'Unmet' with the explanation that I did, or as 'N/A' stating that we do not store any user passwords as part of the ESAPI project?
If I understand you correctly (and I might not be), the project *is* providing the code that implements storing passwords. If that's so, then the project needs to do it correctly. You could say "N/A" if the provided sample code isn't intended for end-user use, I guess, but then you'd need to make *sure* that it's clear that it's not intended for end-user use. I suggest using naming conventions or some other hard-to-ignore signal that the code is testing only, and not for developer use, if you go that route.

Let me know if I've misunderstood something.

--- David A. Wheeler


Kevin W. Wall
 

On Mon, May 16, 2016 at 10:30 AM, Wheeler, David A <dwheeler@ida.org> wrote:
Kevin W. Wall:

Okay, this is a question about interpretations of the 'crypto_password_storage' question.
The question I am referring to was this:
If passwords are stored for authentication of external users, the project
MUST store them as iterated hashes with a per-user salt by using a key
stretching (iterated) algorithm (e.g., PBKDF2, Bcrypt or Scrypt).
[crypto_password_storage]
I answered it as 'Unmet' with the following explanation:
We don't store passwords per se, but there is a toy Authentication reference
implementation that does so. It really was never intended for real use (and
trust me, I really rebelled at making it a part of ESAPI from the get go,
but at the time, I lost). Fortunately, I doubt it is SO LAME, that it is
doubtful anyone is actually using it. (I wanted to replace it with a
reference LDAPAuthentication model, but lost the argument; maybe some day.)
Sounds like something trivially fixable. Why not just replace that
reference implementation with a call to a PBKDF2 or Bcrypt library, and
declare victory? It's just a few lines of code.
Actually, I went back and looked at it and it does NOT store plaintext
passwords; they are stored as hashes in a text file as:

account id | account name | hashed password | roles | lockout | status
| old password hashes | last hostname | last change | last login |
last failed | expiration | failed

where "|" is the field delimiter used.

So passwords ARE hashed and then base64-encoded. The hashing could be
improved (they are using a secret salt and mixing that in with a username
as the salt and the password and only making 1000 iterations, but it's
better than I thought; if you're going to use a 'secret' though, they'd
be better off using an HMAC iustnstead). There is also a LOCAL timing
side-channel attack possible because they are using String.equals() to
compare the stored hash to th the computed one instead of using time-constant
comparison, but hey, not as bad as I thought and that's pretty easy to fix
just by calling MessageDigest.isEqual() on the raw hashes instead of
String.equals() on the base64-encoded hashes. And best of all, that won't
even break backward compatibility.

I think I was recalling the plaintext passwords for the *dummy* user accounts
that were being created in various JUnit tests and never really dug into
it. (I don't think the Authenticator gets used IRL because it doesn't
scale to production sizes, and thus doesn't have a lot of open bugs against
it, so I don't look at it very often.)

For once, it was nice to be wrong about something! :)

I will go back and change that answer in the BadgeApp.

-kevin
--
Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall
NSA: All your crypto bit are belong to us.