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

etcdserver: don't attempt to grant nil permission to a role #13086

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

dlowe
Copy link
Contributor

@dlowe dlowe commented Jun 4, 2021

Similar to #13084, this is a crashing bug found while fuzzing etcd under Mayhem for API, aka mapi. I'll repeat my disclosure from that ticket: working on mapi is my day job!

On a fresh-out-of-the-box bin/etcd:

$ curl -d '{"name": "foo"}' http://localhost:2379/v3/auth/role/add
{"header":{"cluster_id":"14841639068965178418", ...
$ curl -d '{"name": "foo"}' http://localhost:2379/v3/auth/role/grant
curl: (52) Empty reply from server

The server has crashed hard, with:

go.etcd.io/etcd/server/v3/auth.(*authStore).RoleGrantPermission.func1(0x0, 0x17288ea)
	etcd/server/auth/store.go:799 +0x4f
sort.Search(0x1, 0xc00031cba0, 0xc000234840)
	/usr/local/Cellar/go/1.16.4/libexec/src/sort/search.go:66 +0x58
go.etcd.io/etcd/server/v3/auth.(*authStore).RoleGrantPermission(0xc0007be360, 0xc00056ffc0, 0x0, 0x0, 0x0)
	etcd/server/auth/store.go:798 +0x166
go.etcd.io/etcd/server/v3/etcdserver.(*applierV3backend).RoleGrantPermission(0xc0007980c0, 0xc00056ffc0, 0x106f21b, 0x2f6f001fec762, 0xc0395f9f40)
	etcd/server/etcdserver/apply.go:880 +0x4e
go.etcd.io/etcd/server/v3/etcdserver.(*applierV3backend).Apply(0xc0007980c0, 0xc000cf4360, 0x1, 0x0)
	etcd/server/etcdserver/apply.go:227 +0x7dd
go.etcd.io/etcd/server/v3/etcdserver.(*authApplierV3).Apply(0xc0007c00f0, 0xc000cf4360, 0x1, 0x0)
	etcd/server/etcdserver/apply_auth.go:61 +0xdf
go.etcd.io/etcd/server/v3/etcdserver.(*EtcdServer).applyEntryNormal(0xc0003b4300, 0xc00031d538)
	etcd/server/etcdserver/server.go:2212 +0x7d0
go.etcd.io/etcd/server/v3/etcdserver.(*EtcdServer).apply(0xc0003b4300, 0xc000e92930, 0x1, 0x1, 0xc000120100, 0xc0002a97a0, 0xc00067a6b8, 0x186170e)
	etcd/server/etcdserver/server.go:2122 +0x8ed
go.etcd.io/etcd/server/v3/etcdserver.(*EtcdServer).applyEntries(0xc0003b4300, 0xc000120100, 0xc0002db3f0)
	etcd/server/etcdserver/server.go:1363 +0xe5
go.etcd.io/etcd/server/v3/etcdserver.(*EtcdServer).applyAll(0xc0003b4300, 0xc000120100, 0xc0002db3f0)
	etcd/server/etcdserver/server.go:1185 +0x88
go.etcd.io/etcd/server/v3/etcdserver.(*EtcdServer).run.func8(0x1f99fd0, 0xc0001444c0)
	etcd/server/etcdserver/server.go:1117 +0x3c
go.etcd.io/etcd/pkg/v3/schedule.(*fifo).run(0xc0007be3c0)
	etcd/pkg/schedule/schedule.go:157 +0xf3
created by go.etcd.io/etcd/pkg/v3/schedule.NewFIFOScheduler
	etcd/pkg/schedule/schedule.go:70 +0x13b

The fix seems straightforward enough, in this case.

I found what seemed like the right place to add unit test coverage, as well. I decided to extend the existing TestRoleGrantPermission rather than add a new test; just let me know if you'd prefer the latter.

Prevent etcd from crashing when given a bad grant payload, e.g.:

$ curl -d '{"name": "foo"}' http://localhost:2379/v3/auth/role/add
{"header":{"cluster_id":"14841639068965178418", ...
$ curl -d '{"name": "foo"}' http://localhost:2379/v3/auth/role/grant
curl: (52) Empty reply from server
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

awesome thanks!

@gyuho gyuho merged commit ed790d9 into etcd-io:main Jun 4, 2021
@gyuho
Copy link
Contributor

gyuho commented Jun 4, 2021

Can we also highlight this in CHANGELOG?

@dlowe
Copy link
Contributor Author

dlowe commented Jun 4, 2021

Can we also highlight this in CHANGELOG?

Sure! Do you want a tiny pull request for that?

@dlowe
Copy link
Contributor Author

dlowe commented Jun 4, 2021

#13087

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants