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

The public key doesn't match if any field doesn't match #13382

Merged
merged 1 commit into from
Jan 15, 2022

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Oct 2, 2021

Fix #13381

@ahrtr
Copy link
Member Author

ahrtr commented Oct 12, 2021

Could anyone provide feedback on this PR? Thanks.

@serathius
Copy link
Member

Can you add a test?

@ahrtr
Copy link
Member Author

ahrtr commented Oct 14, 2021

Can you add a test?

Thanks for the comment.

In theory, the current implementation isn't correct, because it regards the public keys as not matched when all fields do not match. It means that if some fields match, and others do not match, then it will regard them as matched. So I submitted this PR, it will only regard the public keys as matched when all fields match.

Actually I thought about adding unit test to cover the corner case, but eventually I gave up. Reasons are below.

Firstly, the existing unit tests in jwt_test.go already cover all the basic scenarios.

Secondly, it's difficult to create a real private & public key to hit the corner case.

Thirdly, I thought about adding some mock functions (see below), and add some fake public key & private key of json marshaled strings in jwt_test.go. When the values for "sign-method" in the –auth-token is "fake-rsa" or "fake-ecdsa", then etcd will use the mock functions. But it will have some impacts on the production environment as well and accordingly have security concern, so I gave up this approach.

type jwtOptions struct {
	SignMethod jwt.SigningMethod
	PublicKey  []byte
	PrivateKey []byte
	TTL        time.Duration

	parsePrivateKey func(key []byte) (v interface{}, err error)
	parsePublicKey func(key []byte) (v interface{}, err error)
}

What do you think?

@ahrtr
Copy link
Member Author

ahrtr commented Oct 26, 2021

@serathius any comments please?

@serathius
Copy link
Member

Sorry, I don't think competent enough in Auth code to review this change. Was hoping that adding tests would show exact impact of this change.

Best to ask original author or reviewer of PR that implemented JWT #9883
cc @joelegasse @gyuho

@joelegasse
Copy link
Contributor

Talk bout a blast from the past... I'm no longer active with etcd, but I'll try to add some context here.

When the code was written three years ago (2018), the Equal() methods did not exist in the crypto packages (added in Go 1.15, released in August 2020).

There is actually a subtle bug in the current logic, as described in the last line of the referenced issue. Apparently I failed to properly distribute the negation.

Current RSA stdlib implementation:

return pub.N.Cmp(xx.N) == 0 && pub.E == xx.E

vs. existing logic:

if pub != nil && pub.E != priv.E && pub.N.Cmp(priv.N) != 0 {

Since it's meant to be checking for an error, it should have been:

if pub != nil && (pub.E != priv.E || pub.N.Cmp(priv.N) != 0) {

Since E is generally a common value, this means that the current logic would accept any two keys as "matching". Not great, but without spending the rest of the day trying to remember the details around this, I can't say if the bug would be exploitable for anything other than a running system that can't verify its own tokens (or if it would just defer to the private, since that contains the public parts anyway).

Wrapping up:

From a mechanical perspective, this code does fix some incorrect logic in the key comparison that I wrote years ago. I can't say much more beyond that.

@ahrtr ahrtr force-pushed the public_key_match_issue branch 2 times, most recently from ef0b86a to 68ae5c4 Compare November 15, 2021 18:18
@ahrtr
Copy link
Member Author

ahrtr commented Nov 15, 2021

Just rebased the PR and squashed the commits.

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

Thanks @joelegasse, your comment was very helpful.
+1 based on @joelegasse comment

@ahrtr ahrtr force-pushed the public_key_match_issue branch from 68ae5c4 to 63ff6d4 Compare November 24, 2021 21:58
@ahrtr
Copy link
Member Author

ahrtr commented Nov 24, 2021

Rebased again although there is no conflict with the base branch.

@serathius
Copy link
Member

cc @hexfusion @ptabor

@ptabor ptabor merged commit b8c5d44 into etcd-io:main Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

The logic of checking whether the public keys match may not correct
4 participants