Deprecate current HttpUtilities.setRememberToken() and replace with one not requiring user password

[meg23]

From [email protected] on November 26, 2013 13:27:30

The current HttpUtilities.setRememberToken(String password, int maxAge, String domain, String path) interface takes a user's cleartext password. The reference implementation (DefaultHttpUtilities) creates an AES encrypted HTTP cookie that contains the username and password of the user to be remembered. Storing the password using reversible encryption is contrary to the stated corporate security policies of many companies and therefore using this as a technique to provide unattended login access should be discouraged if for no other reason than corporate policy will (or should) prevent this method from seeing widespread use.

Instead, a new replacement interface something like this:
HttpUtilities.setRememberToken(int maxAge, String domain, String path)
should be implemented to take advantage of CryptoToken which does not require a user's password to be made secure.

Original issue: http://code.google.com/p/owasp-esapi-java/issues/detail?id=311

[xeno6696]

@kwwall, is there a way we can "assign" this issue to me? I'm willing to make this improvement.

[kwwall]

@xeno6696 Matt - Go for it. Generally we have people either submit a pull request or attach a patch which one of us first reviews before we give someone commit access. That doesn't mean we doubt your mad skillz, but that's just been our practice because before we started that in Google Code we had (at one point) maybe a dozen people who had commit access but never submitted any code. Unfortunately, I don't think I can officially assign this issue to you until you get added to this project on GitHub as having commit access. (Well, let me put it this way, I tried and I was unable to assign it to your user id, so I'm speculating that is the reason why.)

[xeno6696]

@kwwall do we want to just deprecate the old method and create a new one, or do we want to nukey nukey?

[xeno6696]

The unit test for setRememberToken is actually partially commented out. We never test it due to some dependency where the cookie is set via setSafeHeader.

I don't know why that makes it untestable.

[xeno6696]

@kwwall , I had to use a bit of reflection but I have a passing unit test for the replacement. So, how did ESAPI typically test something like Cookies in an "integration-like" environment? Our new cookie certainly gets set in the mock response, but before I push I wanted your input.

[kwwall]

@xeno6696 This definitely has to be deprecated. If we flat out remove it, it's liable to break existing application code that is using it. So I would recommend (for time being) just adding a new method to the HttpUtilities interface, but replace 'password' with 'username''. (Even if we didn't use 'username', it would be useful for logging.) Since that would have the same '(String, int, String, String)' args, you would have to use a different method name...maybe something like setRememberMeToken().

As far as testing codes, using the mock responses is likely as good as it gets. Personally, I think a lot of unit testing would have been easier had we been using HttpUnit or something similar, but it may have been (at least pre-Maven days) that they didn't want yet another dependency plus any transitive dependencies that it required that were not already being used. (Although with Maven, I don't think that's such a big deal as we could just mark the as 'test'.)

[xeno6696]

@kwwall I went ahead and pushed a little bit ago. I deprecated the old method and added warnings in the javadocs. I then created the new method as specified here, it does the same thing as before, just without the password var.

HttpUnit would be alright, but @jeremiahjstacey actually introduced me to Wiremock, which sets up mini jetty servers during test execution, so the need for Mocked classes is lower. I'm not sure what the test performance is, but we could try it. It should be faster than HTTPUnit.

In the meantime, I'm closing this issue.

[kwwall]

Oops...obviously I meant '(HttpServletRequest, HttpServletResponse, String, int, String, String)' for the args. Sorry if any confusion.

[xeno6696]

I knew what you meant. Also, if you weren't aware, you can wrap inline code comments with backticks so that github will mark them as code.

(HttpServletRequest, HttpServletResponse, String, int, String, String)

[kwwall]

@xeno6696 We also should only attempt to set this 'remember me' token if the request is being sent over https, and then in such cases, we should set the cookie's 'secure' and 'httpOnly' flags. If the HttpServletRequest not https, then we should through an exception. Together, those things should help prevent cookie from getting hijacked.

[kwwall]

Does that work when replied to from GitHub emails too?
Test: HttpUtilities.setRememberToken(...)

On Sat, May 21, 2016 at 12:26 PM, Matt Seil [email protected]
wrote:

I knew what you meant. Also, if you weren't aware, you can wrap inline
code comments with backticks so that github will mark them as code.

(HttpServletRequest, HttpServletResponse, String, int, String, String)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#316 (comment)

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

[kwwall]

Apparently not.

[xeno6696]

Unfortunately, no.

[xeno6696]

Do you want me to update the deprecated logic with that httpOnly attribute as well? (I plan to, until you say otherwise.)

[kwwall]

​​

@xeno6696 wrote:

Do you want me to update the deprecated logic with that httpOnly
attribute as well? (I plan to, until you say otherwise.)

Good question. On one hand, it may be better to leave it more broken than
to 'fix' it up a little as there may be companies that they object to the
absence of the 'httpOnly' & 'secure' flags as much or more than they do to
the fact that it was storing an encrypted password and if that's the case,
they may have more incentive to keep using the broken one. (I
still can't believe that setRememberToken() was not setting the 'secure'
flag on the cookie.) But still, even though it might prolong
developers from migrating from the deprecated method, it will at least make
things a bit more secure, so yes, I think we shold do it. However, we
should probably only set the 'secure' and 'httpOnly' flags if the
corresponding ESAPI properties are set. You can test those with
ESAPI.securityConfiguration().getForceSecureCookies() and
ESAPI.securityConfiguration().getForceHttpOnlyCookies() respectively.

But yes, let's fix it there too. And note it in the javadoc
of the deprecated method.

The other issue with doing this--i.e., assuming we want to do so with
javax.servlet.http.Cookie--is if you look at the pom.xml, it looks as
though we are only currently using (thus requiring) Servlet 2.5 API.
According to this javadoc from Java EE 6,
https://docs.oracle.com/javaee/6/api/javax/servlet/http/Cookie.html#setHttpOnly%28boolean%29
it looks as though the Cookie.setHttpOnly() method is only since Servlet
3.0. (In part, I guess that's why there is a private
DefaultHTTPUtilities.createCookieHeader() method that sets that.) I guess
we could use that private createCookieHeader to create the cookie instead
of javax.servlet.http.Cookie so we don't force people to use Servlet 3.0
API or newer, which might be a showstopper. What do you think of that as an
alternate implementation instead of using javax.servlet.http.Cookie and
requiring at least Servlet 3.0? (Sigh; backward compatibility is the curse
of software design.)

-kevin

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

[xeno6696]

How about this solution: We fix the current version using ESAPI, but then create a new DefaultHttpUtilities class to support servlet 3.0. We could then update ESAPI over the next months and then in the next-next major release (2.3) we switch the default implementations over to use servlet 3.0. We deprecate all the old 2.5 API items and in 2.2 which would force those who upgrade to change the implementing classes back to the old 2.5 esapi versions which would all be deprecated.

Side-note: @jeremiahjstacey had some fantastic ideas to get rid of ESAPI's singleton problem without breaking any existing call structures, while also supporting my vision of breaking esapi up into multiple libraries, as opposed to the kind of monolithic lib it is now.

[xeno6696]

Also @kwwall, the default setting for secure-only cookies in ESAPI.properties is "false" if that makes any difference.

[kwwall]

On Sat, May 21, 2016 at 5:13 PM,
​ @xeno6696 wrote:

How about this solution: We fix the current version using ESAPI, but then
create a new DefaultHttpUtilities class to support servlet 3.0. We could
then update ESAPI over the next months and then in the next-next major
release (2.3) we switch the default implementations over to use servlet
3.0. We deprecate all the old 2.5 API items and in 2.2 which would force
those who upgrade to change the implementing classes back to the old 2.5
esapi versions which would all be deprecated
​.

​I could get behind that plan. Let's do it. But next "major" release
should be ESAPI 2.2, not 2.3. In 2.2, I'd also like to only be support JDK
7 or newer. (There comes a time where if you have organizations using ESAPI
and they are still on ​

​Java 6, IMO they are not really serious about security in the first place
and I don't think those people should be holding everyone else back.)​

​
Side-note: @jeremiahjstacey https://github.com/jeremiahjstacey had some
fantastic ideas to get rid of ESAPI's singleton problem without breaking
any existing call structures, while also supporting my vision of breaking
esapi up into multiple libraries, as opposed to the kind of monolithic lib
it is now.

​Okay, let's talk. I already had it ripped out of JavaEncryptor in a
private build, and I don't think it would break anything, but let's talk. I
suggest either a private email conversation or, if you both feel
comfortable with i​t, let's discuss it on the ESAPI-Dev mailing list. (But
let's not continue that conversation here; it's too hard to follow, cross
reference, etc.)

-kevin

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

[xeno6696]

@kwwall :

We also should only attempt to set this 'remember me' token if the request is being sent over https, and then in such cases, we should set the cookie's 'secure' and 'httpOnly' flags. If the HttpServletRequest not https, then we should through an exception. Together, those things should help prevent cookie from getting hijacked.

Help me out here... as far as I can tell, we don't call this except for unit tests!

[kwwall]

@xeno6696 I little confused what you meant by that last comment 'Help me out here...as far as I can tell, we don't call this except for unit tests!' I'm not exactly sure what method the 'this' is referring to?
Were you referring to:

DefaultHTTPUtilities.setRememberToken(HttpServletRequest, HttpServletResponse, String, int, String, String)
or
AbstractAuthenticator.getUserFromRememberToken()
or something else?

And I guess the better question would be, why does this surprise you as it seems to (based on your exclamation point)?

If what was confusing you was my comment that you quoted, what I meant is that we should look at the "HttpServletRequest request" argument and call request.getScheme() to see if it returns "https". If and only if it does, we should set the 'secure' flag. (We can set the 'httpOnly' flag regardless.) If the scheme were, "http" instead of "https", then setting the 'secure' flag is only going to cause problems, because it would appear to work from the server-side, but the but a proper client side browser would refuse to accept the cookie as it is supposed to accept any cookie marked 'secure' over a non-secure channel. That likely would cause some subtle bugs.

[xeno6696]

I merged this earlier today... not sure why it didn't close.