Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address signature reuse vulnerability #223

Closed
wants to merge 2 commits into from
Closed

Conversation

bifurcation
Copy link
Contributor

@AGWA pointed out on the IETF ACME list that the current ACME challenges are vulnerable to a signature reuse attack, with an artfully chosen account key pair. This PR addresses that vulnerability by replacing the signature with a digest and reducing the client's control over the contents of the validation object.

@jdkasten
Copy link
Contributor

👍

@kuba
Copy link
Contributor

kuba commented Sep 15, 2015

I though this was editorial changes only repo and significant changes should go to https://github.com/ietf-wg-acme/acme instead. @bifurcation?

{
"token": "evaGxfADs6pSRb2LAv9IZf17Dt3juxGJ-PCt92wr-oA",
"key": /* account key, as a JWK object */
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of [{token: A, key: K1}, {token: B, key: K2}]... which key do I use for the JWS? :O In other part of the spec we require that "JWS MUST use the Flattened JSON Serialization", which implies one signature (and key) per JWS. Actually, do we ever sign this object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object is never signed, so there's no problem.

@bifurcation
Copy link
Contributor Author

@jdkasten @kuba Thanks for the comments. Implementing them now.

@bifurcation
Copy link
Contributor Author

@kuba IIRC, the decision was "clarifications and critical fixes". I think this falls in the latter bin.

* The "type" field is set to "dvsni"
* The "token" field matches the "token" value in the challenge
1. Compute the Z-value from the authorized keys object in the same way as the
client.
4. Open a TLS connection to the domain name being validated on port 443,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Use 2. 3. rather than 4. 5. Sorry I missed this before.

@jsha
Copy link
Collaborator

jsha commented Sep 23, 2015

I think it is suboptimal for each of the challenges to treat the authorizedKeys object differently. Also, I think given the current design, having a list of authorizedKeys doesn't actually gain us much. On the ACME list, I had suggested that provisioning a JSON list of authorizedKeys would be nice because it would make for an easy mechanism for revoking authorization from accounts you no longer use (i.e. old hosting providers). However, it only has that property so long as the CA re-validates each authorization immediately prior to issuance. That's a much deeper spec change, and I think is more than we want to do right now.

Your goal in using a JSON array for authorizedKeys rather than a single object is that it allows a subscriber to satisfy multiple SimpleHTTP challenges at one time. However the same objective can be satisfied just as well by provisioning multiple files at the same time.

A cleaner design here would be to say that for every challenge, the client POST includes an authorizedKey field consisting of a single object base64-encoded ({accountKey: ..., token: ...}), and the value they have to provision, whether in DNS, HTTP, or DVSNI, is the sha256 hash of that authorizedKey object. This could potentially simplify both client and server code significantly.

authorizedKeys (required, string):
: A serialized authorized keys object, base64-encoded. This object MUST have
only one entry, whose token value matches the "token" value in the challenge,
and "key" value matches the client's account key.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I see what you are saying here, but I think it should be crisper. The point isn't that you must have one matching key, but rather that it must ahve one key and it must match.

"The object MUST have exactly one entry. That entry's token value must match..."

@bifurcation
Copy link
Contributor Author

This has been moved to ietf-wg-acme/acme#6

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.

None yet

5 participants