Static analysis and constant time comparisons

This is the first in a series of new developer-written posts on our blog about Building 1Password, a behind-the-scenes look at what goes into making the app. You can expect these to be technical, nerdy, and frankly… not nearly as polished as what our crack marketing and content teams put out.

What are we trying to solve?

Before we get to the solution, let’s talk about the problem. It’s recommended that security-sensitive comparisons be done in a constant-time manner. In this case, the comparison was a token string that is sent to the user via email. The recipient uses this to effectively prove they received the email and control the email account. A constant-time approach ensures that the comparison always takes the same amount of time, regardless of the outcome. We use the Go programming language for our server, and Go has the crypto/subtle package which provides functions to do this. Fixing this is quite simple, you change something that looked like:

if user.VerificationToken != token

To something like:

if subtle.ConstantTimeCompare([]byte(user.VerificationToken), []byte(token)) != 1

An identical timeframe for “true” and “false” comparisons minimizes what an attacker can learn from a request. At 1Password we try our best to stop attackers from learning anything about what’s going on inside our servers. One such piece of information is how long the server takes to validate your information. Sometimes we have concrete attacks in mind: we don’t want to leak information on whether you supplied the right or wrong session key. Sometimes we don’t have a concrete attack in mind - because we can’t always look in the minds of our attackers, or look into the future of how our codebase evolves. As a general rule though, we try to make sure that whenever we verify your information we apply constant time comparisons.

So now we know why doing these comparisons in constant time is important, and how we might go about fixing this particular issue. As developers we’re less interested in solving one specific problem so much as a whole class of problems. How can we ensure that we aren’t making the same mistake elsewhere? We could manually check by searching for ‘user.VerificationToken’ references,’ but how do we ensure this mistake doesn’t happen again?

Building a solution

To solve this problem, we used a custom static analysis tool that we now run as part of our continuous integration system so that every Merge Request is pushed to our GitLab instance.

There are a plethora of great static analysis tools for Go, for example there’s securego/gosec to help find security problems. Unfortunately, it can’t help us solve this particular problem. Luckily for us though, the Go packages library comes with the tools for us to build a custom static analysis solution to this problem!

The only tricky part was to determine how we were going to find these cases without inundating ourselves with false-positives. We decided that the best way to do so was to define which fields in a Go struct were not safe to compare in a non-constant-time manner. Go has just the thing to describe this: field tags. Using our previous example, this is what it’d look like:

type User struct {
    VerificationToken string `db:"verification_token" security:"nodirectequality"`
}

The VerificationToken field on our User structure now has a security tag whose value is nodirectequality.

Now it’s just a matter of using the Go tools to build and walk the abstract syntax tree for a Go package and find violations of this rule. Sadly, we can’t take a structure and find all times that a field was referenced in the codebase, so our search is the other way around and requires that we find every equality comparison in the codebase and look at the left and right sides to determine if either is a field marked as being forbidden.

This gives us a tool that we can run as:

$ go-directequality-checker go.1password.io/b5/server/src/logic/action
[SECURITY] Found raw comparison of field 'VerificationToken'. Use constant time comparison function.
/Users/rfillion/go/src/go.1password.io/b5/server/src/logic/action/recovery.go:174
user.VerificationToken != token {

Once the tool was built, we just needed to annotate the relevant fields on our structures. In doing so, the tool identified two more cases that were not identified as part of the pentest, and best of all now that it’s part of our CI pipeline we shouldn’t be seeing this problem in the future. At least not with these fields. Assuming that they’re used incorrectly in the way that we expect.

Limitations of this approach

Which brings us to the part of this that’s a little less fun: this solution doesn’t technically solve the whole class of problems. It relies on us developers correctly identifying which fields deserve this designation. It also relies on code being structured in such a way that makes detecting this easy. It could easily be fooled by doing something like:

verificationToken := user.VerificationToken
if verificationToken != token

Since the left hand side of the “if” statement no longer references a specific field with annotations, the checker would no longer find it.

We haven’t yet thought of a way to solve the whole class of problems without banning direct equality entirely which isn’t realistic. This solution allows us to paint it into a sufficiently small corner that allows us to feel pretty confident about it and gives us the tools we need to do deeper searches for offending code should other instances be found in the future.

Get the sources

We’ve open-sourced our checker tool so that you can either use it or look at how it works. You can find it on our 1Password/go-directequality-checker github repo.

Rick Fillion

VP of Engineering: 1Password.com