20070719

Servlet Filter for httpOnly

Link

Well, zot! It turns out that you can still get at the cookies by XHR's getAllResponseHeaders().

But since it's still not really that harmful to add it, I've thrown together a servlet filter for adding the attribute when addCookie() is called. Except for the httpOnly attribute, this is no safer than the existing addCookie - meaning if you had header injection flaws before, they're still there. Also, I didn't read RFC 2109 really carefully, so this may break version 1 cookies (however, RFC 2109 cookies are still considered experimental, and you wouldn't be using them by accident).

Now, your container probably has control over the session cookies, and those get added after all the filters run, so you'll still need to consult your app server's documentation to see how to get it to set httpOnly on the session cookie (the one cookie that needs it most).

Furthermore, this does not check for cookies being added by addHeader() or setHeader(). Whether you want to add similar checking to those or not is purely up to you.


public class HttpOnlyResponseWrapper extends HttpServletResponseWrapper {
private static SimpleDateFormat cookieFormat = new SimpleDateFormat("EEE, d MMM yyyy HH:mm:ss zzz");

public HttpOnlyResponseWrapper(HttpServletResponse res) {
super(res);
}

public void addCookie(Cookie cookie) {
System.out.println("Adding cookie");
StringBuffer header = new StringBuffer();
if ((cookie.getName() != null) && (!cookie.getName().equals(""))) {
header.append(cookie.getName());
}
if (cookie.getValue() != null) {
// Empty values allowed for deleting cookie
header.append("=" + cookie.getValue());
}

if (cookie.getVersion() == 1) {
header.append(";Version=1");
if (cookie.getComment() != null) {
header.append(";Comment=\"" + cookie.getComment() + "\"");
}
if (cookie.getMaxAge() > -1) {
header.append(";Max-Age=" + cookie.getMaxAge());
}
} else {
if (cookie.getMaxAge() > -1) {
Date now = new Date();
now.setTime(now.getTime() + (1000L * cookie.getMaxAge()));
header.append(";Expires=" + HttpOnlyResponseWrapper.cookieFormat.format(now));
}
}

if (cookie.getDomain() != null) {
header.append(";Domain=" + cookie.getDomain());
}
if (cookie.getPath() != null) {
header.append(";Path=" + cookie.getPath());
}
if (cookie.getSecure()) {
header.append(";Secure");
}
header.append(";httpOnly");
addHeader("Set-Cookie", header.toString());
}
}

public class HttpOnlyFilter implements Filter {
private FilterConfig config;

@Override
public void destroy() {
this.config = null;
}

@Override
public void doFilter(ServletRequest req, ServletResponse res,
FilterChain chain) throws IOException, ServletException {
System.out.println("Got here");

HttpOnlyResponseWrapper hres = new HttpOnlyResponseWrapper((HttpServletResponse)res);
chain.doFilter(req, hres);
}

public FilterConfig getFilterConfig() {
return this.config;
}

@Override
public void init(FilterConfig config) throws ServletException {
this.config = config;
}
}

5 comments:

  1. Dunno this seems much more concise to me:


    class HttpOnlyResponseWrapper extends HttpServletResponseWrapper {
    static cookieFormat = new SimpleDateFormat("EEE, d MMM yyyy HH:mm:ss zzz")

    HttpOnlyResponseWrapper(res) {
    super(res)
    }

    void addCookie(Cookie cookie) {
    println "Adding cookie"
    def header = new StringBuffer()
    if (cookie.name) header << cookie.name
    if (cookie.value) header << "=${cookie.value}"
    if (cookie.version == 1) {
    header << ";Version=1"
    if (cookie.comment) header << ";Comment=\"${cookie.comment}"\"")
    if (cookie.maxAge > -1) header << ";Max-Age=${cookie.maxAge}"
    } else {
    if (cookie.maxAge > -1) {
    def now = new Date()
    now.setTime(now.getTime() + (1000L * cookie.maxAge))
    header << ";Expires=${HttpOnlyResponseWrapper.cookieFormat.format(now)}"
    }
    }

    if (cookie.domain) header << ";Domain=${cookie.domain}"
    if (cookie.path) header << ";Path=${cookie.path}"
    if (cookie.secure) header << ";Secure"
    header << ";httpOnly")
    addHeader("Set-Cookie", header.toString())
    }
    }

    class HttpOnlyFilter implements Filter {
    FilterConfig config

    @Override
    public void destroy() {
    this.config = null
    }

    @Override
    doFilter(ServletRequest req, ServletResponse res,
    FilterChain chain) {
    println "Got here"
    def hres = new HttpOnlyResponseWrapper(res)
    chain.doFilter(req, hres)
    }
    @Override
    public void init(FilterConfig config) throws ServletException {
    this.config = config;
    }
    }

    ReplyDelete
  2. Wow - another author The Definitive Guide to Grails! (And yes, it's on my bookshelf).

    @Graeme, thanks - that's almost ver batim what I had. But because Java doesn't do duck-typing, there are still the extends/implements, and all the methods have to have the parameters strictly typed. But yes, it is more concise. Just not quite as "groovy" as everything else grails.

    ReplyDelete
  3. Nice.

    How about

    org.apache.commons.httpclient.Cookie c = new org.apache.commons.httpclient.Cookie("", "hidden", "value");

    response.addHeader("Set-Cookie", c.toExternalForm() + "; httpOnly");

    ReplyDelete
  4. Nadav Elyada08:35

    Nice work.
    By the way, SimpleDateFormat is not guaranteed to be thread-safe:

    "Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally."
    --http://java.sun.com/j2se/1.5.0/docs/api/java/text/SimpleDateFormat.html

    ReplyDelete
  5. Another good catch. This is why we have code review.

    I would say that since it's private here, the chances of the format string getting changed externally (one of the worst-case scenarios) is unlikely. However, the API documentation doesn't say *what* is not thread safe, which could be some internal representation during the format, so you're better off making a new one for each call to addCookie.

    Thanks again for the find.

    ReplyDelete