Skip to Content [alt-c]

January 4, 2020

This Is Why You Always Review Your Dependencies, AGPL Edition

Before adding a dependency to one of my software projects, I do some basic vetting of the dependency. Among the things I check are:

  • How is the code licensed?
  • Who are the authors?
  • Are there any serious unresolved issues in the issue tracker?
  • Is there a history of serious bugs in the issue tracker?
  • What kind of code review process is used for pull requests?

Finally, I do a cursory review of the code. I look for anything blatantly insecure or malicious, and try to get a feel for the quality of the code base. I look for "Brown M&Ms" - minor inattention to detail that might indicate a larger problem.

I repeat the above recursively on transitive dependencies as many times as necessary. I also repeat the cursory code review any time I upgrade a dependency.

This is quite a bit of work, but is necessary to avoid falling victim to attacks like event-stream. I was recently reminded of yet another reason to review dependencies, as I reviewed Duo's highly-publicized Go library for WebAuthn, github.com/duo-labs/webauthn.

It started off poorly when I noticed some Brown M&M's: despite being a library, it was logging messages to stdout, and there were several code smells which indicated inexperience with Go. Sure enough, these minor issues foreshadowed a far larger problem: when I started reviewing the transitive dependency github.com/katzenpost/core/crypto/eddsa, I was greeted with an AGPLv3 license header.

This was bad news for most people wanting to use Duo's WebAuthn library. Although Duo had licensed their library under a BSD license, when you linked your application with Duo's library, you'd also be linking with the AGPL-licensed library, creating a "modified" work in the eyes of the (A)GPL, thus subjecting your application to section 13 of the AGPL:

Notwithstanding any other provision of this License, if you modify the Program, your modified version must prominently offer all users interacting with it remotely through a computer network (if your version supports such interaction) an opportunity to receive the Corresponding Source of your version by providing access to the Corresponding Source from a network server at no charge, through some standard or customary means of facilitating copying of software.

In other words, if you used github.com/duo-labs/webauthn in a public-facing web app, your web app had to be open source.

The most galling thing about this dependency is that it's redundant with golang.org/x/crypto/ed25519, which is one of Go's quasi-standard "x" libraries. In fact, github.com/duo-labs/webauthn originally used golang.org/x/crypto/ed25519. That changed during a pull request from an external collaborator titled "Consolidate COSE things to their own area". In the process of moving some code from one file to another, this pull request subtly changed the implementation of OKPPublicKeyData.Verify.

Here's the old OKPPublicKeyData.Verify, which uses golang.org/x/crypto/ed25519:

// Verify Octet Key Pair (OKP) Public Key Signature
func (k *OKPPublicKeyData) Verify(data []byte, sig []byte) (bool, error) {
	f := HasherFromCOSEAlg(COSEAlgorithmIdentifier(k.PublicKeyData.Algorithm))
	h := f()
	h.Write(data)
	return ed25519.Verify(k.XCoord, h.Sum(nil), sig), nil
}

Here's the new OKPPublicKeyData.Verify, which uses the AGPL-licensed github.com/katzenpost/core/crypto/eddsa:

// Verify Octet Key Pair (OKP) Public Key Signature
func (k *OKPPublicKeyData) Verify(data []byte, sig []byte) (bool, error) {
	f := HasherFromCOSEAlg(COSEAlgorithmIdentifier(k.PublicKeyData.Algorithm))
	h := f()
	h.Write(data)
	var oKey eddsa.PublicKey
	err := oKey.FromBytes(k.XCoord)
	if err != nil {
		return false, err
	}
	return oKey.Verify(h.Sum(nil), sig), nil
}

There was zero explanation provided for this change. The pull request was reviewed by two Duo employees, who approved and merged it.

Aside: this is why I don't like to accept pull requests that move code around. Even if the new code organization is better, it's usually not worth the time it takes to ensure the pull request isn't doing anything extra.

I filed an issue about the AGPL-licensed dependency, and the developers switched back to using golang.org/x/crypto/ed25519. Nevertheless, I've decided not to use github.com/duo-labs/webauthn. The bulk of the library and its dependencies are to support a WebAuthn misfeature called attestation, which I have less-than-zero desire to use. I just finished writing a vastly simpler, attestation-free library which is less than one tenth the size (I will open source it soon - watch this space). (There's another lesson here, which is that complicated "features" like attestation that serve a minority's use case shouldn't be added to Web standards.) Developing this library is less costly than the liability of using an existing WebAuthn Go library.

This incident reminded me of why I like programming in Go. Go's extensive standard library, along with its quasi-standard "x" libraries, mean that the dependency graph of my projects is typically quite small. The bulk of my trust is consolidated in the Go project, and thanks to their stellar reputation and solid operating procedures, I don't feel a need to review the source code of the Go compiler and standard libraries. Even though I love Rust, I am terrified every time I look at the dependency graph of a typical Rust library: I usually see dozens of transitive dependencies written by Internet randos whom I have zero reason to trust. Vetting all those dependencies takes far too much time, which is why I'm much less productive in Rust than Go.

One final note: as a fan of verifiable data structures like Certificate Transparency, I have to love the new Go checksum database. However, the checksum database does you no good if you don't take the time to review your dependencies. Unfortunately, I've already seen one over-enthusiastic Go user claim that the Go checksum database solves all problems with dependency management. It doesn't. There's no easy way around this basic fact: you have to review your dependencies.

Comments

Anonymous on 2020-01-06 at 14:35:

Great post, I was just wondering about the attestation part: do you see it as potentially harmful or just useless?

Reply

Andrew Ayer on 2020-01-06 at 17:07:

Thanks!

I do see attestation as harmful. First, because it adds complexity that will make WebAuthn harder for website operators to implement. Second, because it creates a risk that websites will use whitelists of security key models, which will cause uncertainty among users about whether their security key will work on all websites.

I'll be elaborating on the problems with attestation when I release my easy WebAuthn library.

Reply

Anonymous on 2020-01-06 at 20:44:

Great article, just one question:

It started off poorly when I noticed some Brown M&M's: despite being a library, it was logging messages to stdout

What is wrong with a library logging messages to stdout?

Reply

Andrew Ayer on 2020-01-07 at 00:57:

stdout is meant for the output of a program. It's where curl writes the file that it downloads. It's where a CGI webapp writes the response headers and body. It's how a server started by inetd communicates with the client. If a library logs messages to stdout, those messages will disrupt a program's output and could cause serious problems to the whoever is processing it.

Using stderr instead of stdout avoids the worst problems, but it's still usually not what you want a library to do. If the library needs to report an error, which was the case with github.com/duo-labs/webauthn, it should return it to the caller (or throw an exception, depending on the language's conventions). If a library has a legitimate need to log, then it should log to a logger provided by the user of the library, so they have control over where the messages go.

Reply

Reader Don on 2021-01-31 at 04:27:

Are you still planning on releasing your library?

Reply

Post a Comment

Your comment will be public. To contact me privately, email me. Please keep your comment polite, on-topic, and comprehensible. Your comment may be held for moderation before being published.

(Optional; will be published)

(Optional; will not be published)

(Optional; will be published)

  • Blank lines separate paragraphs.
  • Lines starting with > are indented as block quotes.
  • Lines starting with two spaces are reproduced verbatim (good for code).
  • Text surrounded by *asterisks* is italicized.
  • Text surrounded by `back ticks` is monospaced.
  • URLs are turned into links.
  • Use the Preview button to check your formatting.