Skip to content

Don't keep a copy of the nonce that nonce() hands to its caller - #58

Open
datrixlab wants to merge 1 commit into
mholt:masterfrom
datrixlab:fix/nonce-handed-out-twice
Open

datrixlab wants to merge 1 commit into
mholt:masterfrom
datrixlab:fix/nonce-handed-out-twice

Conversation

@datrixlab

Copy link
Copy Markdown

The bug

doHTTPRequest pushes the Replay-Nonce of every response onto c.nonces (acme/http.go:294). That includes the HEAD newNonce response, whose header Client.nonce() then returns to its caller (acme/client.go:151). One nonce ends up with two owners: the caller signs a JWS with it, and the next nonce() call pops the same value out of the pool. The server rejects the second use with badNonce.

Measured with three sequential nonce() calls against a test server that tags each nonce:

nonces handed out newNonce requests left in the pool
before [nonce1 nonce1 nonce2] 2 nonce2 (also handed out)
after [nonce1 nonce2 nonce3] 3 none

Impact is bounded, which is why I am not filing this as a security issue: nonce() has one caller, inside httpPostJWS's retry loop, and the badNonce branch there retries with the fresh nonce from the error response. So the cost is a wasted round trip per duplicate, and the LIFO pool hides it in the common sequential case — the duplicate sinks to the bottom of the stack. It surfaces when the pool drains (two requests in a row both popping the same stale value) and under concurrent use of one client, which the stack type is explicitly built for ("a simple thread-safe stack").

The fix

Skip the push when the request is the newNonce HEAD. Nonces from every other response are still remembered, which is what §6.5 and §7.2 ask for: those are nonces the client is not consuming at the time.

x/crypto/acme, the ancestor of this file, draws the same line — its fetchNonce does the HEAD and returns the header without ever calling addNonce, and addNonce is called only from the directory GET and from POST responses.

I chose this over having nonce() pop the value it just pushed: with a concurrent caller the pool can be empty at that moment, and nonce() has no retry (httpPostJWS returns its error straight to the caller), so that shape turns a harmless duplicate into a failed request.

Tests

New acme/nonce_test.go:

  • TestNonceIsHandedOutOnce — three nonce() calls must return three distinct nonces, hit the endpoint three times, and leave the pool empty.
  • TestNonceFromOtherResponsesIsRemembered — a POST response's nonce is still pooled and served to the next nonce() call, so the fix does not stop the client remembering nonces generally.

go test ./... passes on this branch. Reverting only acme/http.go fails the first test with the numbers in the table above, and the second test passes either way.

One thing I left alone

nonce() returns resp.Header.Get(replayNonce) without checking for an empty value, so a server that omits the header (a MUST in §6.5) gets a JWS with an empty nonce and a badNonce retry instead of a clear error. x/crypto/acme errors there ("acme: nonce not found"). Happy to add that here or in a separate PR, but it changes behaviour for a spec-violating server, so I kept it out of this one.

doHTTPRequest pushes the Replay-Nonce of every response onto c.nonces,
including the newNonce response whose header Client.nonce() returns
directly. One nonce therefore gets two owners: the caller signs a JWS with
it, and the next nonce() call pops the same value from the pool. The
server rejects the second use with badNonce, which costs a round trip
through httpPostJWS's retry loop.

Measured with three sequential nonce() calls against a test server that
tags each nonce:

  before: handed out [nonce1 nonce1 nonce2], 2 newNonce requests,
          "nonce2" also left in the pool
  after:  handed out [nonce1 nonce2 nonce3], 3 newNonce requests,
          pool empty

Skip the push for a newNonce request. Nonces from every other response are
still remembered, which is what §6.5 and §7.2 are about: those are nonces
the client is not consuming at the time. x/crypto/acme draws the same
line, its fetchNonce never calls addNonce.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant