DATA RACE in pkg/endpoint.(*Endpoint).GetIdentity() #11932
Assignees
Labels
kind/bug
This is a bug in the Cilium logic.
Comments
aanm
added
kind/bug
|
where did you see this race, CI run or locally? |
CI run |
|
The bug itself is not serious, it can result in old/new value being assigned to a flow as endpoint is changing the id. There isn't much that can be done about this, as flows are constantly decoded It could definitely create weird situations where endpoint struct is mid-update, and we read some old and some new values. For fixing the race, it does look like there is a missing protection for get/set identity. |
|
I think Simply protecting A more functional way to fix this is to make sure all necessary values are read right away in cilium/pkg/hubble/parser/threefour/parser.go Line 249 in 3d57190 Otherwise it's a pointer and sub-sequent calls can be racy |
|
There are several places which are vulnerable, this is not Hubble specific:
|
tgraf
removed
the
needs/triage
tgraf
added a commit
that referenced
this issue
Jun 7, 2020
Calls to GetIdentity() have been assuming both that the endpoint is locked or not locked. * daemon/cmd/fqdn.go / notifyOnDNSMsg(): LookupEndpointIDByIP(). No locking. --> Vulnerable * pkg/datapath/linux/config/config.go / writeStaticData() e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData() The endpoint is locked. Not using epCacheInfo. Must use a non-locking variation or a deadlock may occur. --> Not vulnerable * pkg/datapath/loader/template.go e.realizeBPFState() -> -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions() -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() -> -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions() Uses epInfoCache --> Not vulnerable * pkg/envoy/server.go e.updateNetworkPolicy() -> UpdateNetworkPolicy() The endpoint is locked. Must use a non-locking variation. --> Not vulnerable * pkg/hubble/parser/threefour/parser.go Decode() -> resolveEndpoint() --> Vulnerable Fixes: #11932 Signed-off-by: Thomas Graf <thomas@cilium.io>
joestringer
pushed a commit
that referenced
this issue
Jun 8, 2020
[ upstream commit 10f4e2c ] [ Backporter's notes: Minor conflicts scattered across five files. ] Calls to GetIdentity() have been assuming both that the endpoint is locked or not locked. * daemon/cmd/fqdn.go / notifyOnDNSMsg(): LookupEndpointIDByIP(). No locking. --> Vulnerable * pkg/datapath/linux/config/config.go / writeStaticData() e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData() The endpoint is locked. Not using epCacheInfo. Must use a non-locking variation or a deadlock may occur. --> Not vulnerable * pkg/datapath/loader/template.go e.realizeBPFState() -> -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions() -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() -> -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions() Uses epInfoCache --> Not vulnerable * pkg/envoy/server.go e.updateNetworkPolicy() -> UpdateNetworkPolicy() The endpoint is locked. Must use a non-locking variation. --> Not vulnerable * pkg/hubble/parser/threefour/parser.go Decode() -> resolveEndpoint() --> Vulnerable Fixes: #11932 Signed-off-by: Thomas Graf <thomas@cilium.io> Signed-off-by: Joe Stringer <joe@cilium.io>
aanm
pushed a commit
that referenced
this issue
Jun 9, 2020
[ upstream commit 10f4e2c ] Calls to GetIdentity() have been assuming both that the endpoint is locked or not locked. * daemon/cmd/fqdn.go / notifyOnDNSMsg(): LookupEndpointIDByIP(). No locking. --> Vulnerable * pkg/datapath/linux/config/config.go / writeStaticData() e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData() The endpoint is locked. Not using epCacheInfo. Must use a non-locking variation or a deadlock may occur. --> Not vulnerable * pkg/datapath/loader/template.go e.realizeBPFState() -> -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions() -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() -> -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions() Uses epInfoCache --> Not vulnerable * pkg/envoy/server.go e.updateNetworkPolicy() -> UpdateNetworkPolicy() The endpoint is locked. Must use a non-locking variation. --> Not vulnerable * pkg/hubble/parser/threefour/parser.go Decode() -> resolveEndpoint() --> Vulnerable Fixes: #11932 Signed-off-by: Thomas Graf <thomas@cilium.io> Signed-off-by: André Martins <andre@cilium.io>
aanm
pushed a commit
that referenced
this issue
Jun 10, 2020
[ upstream commit 10f4e2c ] Calls to GetIdentity() have been assuming both that the endpoint is locked or not locked. * daemon/cmd/fqdn.go / notifyOnDNSMsg(): LookupEndpointIDByIP(). No locking. --> Vulnerable * pkg/datapath/linux/config/config.go / writeStaticData() e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData() The endpoint is locked. Not using epCacheInfo. Must use a non-locking variation or a deadlock may occur. --> Not vulnerable * pkg/datapath/loader/template.go e.realizeBPFState() -> -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions() -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() -> -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions() Uses epInfoCache --> Not vulnerable * pkg/envoy/server.go e.updateNetworkPolicy() -> UpdateNetworkPolicy() The endpoint is locked. Must use a non-locking variation. --> Not vulnerable * pkg/hubble/parser/threefour/parser.go Decode() -> resolveEndpoint() --> Vulnerable Fixes: #11932 Signed-off-by: Thomas Graf <thomas@cilium.io> Signed-off-by: André Martins <andre@cilium.io>
errordeveloper
pushed a commit
that referenced
this issue
Jun 10, 2020
[ upstream commit 10f4e2c ] [ Backporter's notes: Minor conflicts scattered across five files. ] Calls to GetIdentity() have been assuming both that the endpoint is locked or not locked. * daemon/cmd/fqdn.go / notifyOnDNSMsg(): LookupEndpointIDByIP(). No locking. --> Vulnerable * pkg/datapath/linux/config/config.go / writeStaticData() e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData() The endpoint is locked. Not using epCacheInfo. Must use a non-locking variation or a deadlock may occur. --> Not vulnerable * pkg/datapath/loader/template.go e.realizeBPFState() -> -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions() -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() -> -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions() Uses epInfoCache --> Not vulnerable * pkg/envoy/server.go e.updateNetworkPolicy() -> UpdateNetworkPolicy() The endpoint is locked. Must use a non-locking variation. --> Not vulnerable * pkg/hubble/parser/threefour/parser.go Decode() -> resolveEndpoint() --> Vulnerable Fixes: #11932 Signed-off-by: Thomas Graf <thomas@cilium.io> Signed-off-by: Joe Stringer <joe@cilium.io>
joestringer
pushed a commit
that referenced
this issue
Jun 12, 2020
[ upstream commit 10f4e2c ] [ Backporter's notes: Several minor conflicts, simple resolution. One extra previously-unnecessary function call change in pkg/proxy/epinfo.go. ] Calls to GetIdentity() have been assuming both that the endpoint is locked or not locked. * daemon/cmd/fqdn.go / notifyOnDNSMsg(): LookupEndpointIDByIP(). No locking. --> Vulnerable * pkg/datapath/linux/config/config.go / writeStaticData() e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData() The endpoint is locked. Not using epCacheInfo. Must use a non-locking variation or a deadlock may occur. --> Not vulnerable * pkg/datapath/loader/template.go e.realizeBPFState() -> -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions() -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() -> -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions() Uses epInfoCache --> Not vulnerable * pkg/envoy/server.go e.updateNetworkPolicy() -> UpdateNetworkPolicy() The endpoint is locked. Must use a non-locking variation. --> Not vulnerable * pkg/hubble/parser/threefour/parser.go Decode() -> resolveEndpoint() --> Vulnerable Fixes: #11932 Signed-off-by: Thomas Graf <thomas@cilium.io> Signed-off-by: Joe Stringer <joe@cilium.io>
joestringer
pushed a commit
that referenced
this issue
Jun 12, 2020
[ upstream commit 10f4e2c ] [ Backporter's notes: Several minor conflicts, simple resolution. One extra previously-unnecessary function call change in pkg/proxy/epinfo.go. ] Calls to GetIdentity() have been assuming both that the endpoint is locked or not locked. * daemon/cmd/fqdn.go / notifyOnDNSMsg(): LookupEndpointIDByIP(). No locking. --> Vulnerable * pkg/datapath/linux/config/config.go / writeStaticData() e.owner.Datapath().WriteEndpointConfig -> WriteEndpointConfig() -> writeStaticData() The endpoint is locked. Not using epCacheInfo. Must use a non-locking variation or a deadlock may occur. --> Not vulnerable * pkg/datapath/loader/template.go e.realizeBPFState() -> -> CompileOrLoad -> ELFSubstitutions() -> elfVariableSubstitutions() -> CompileAndLoad() -> compileAndLoad() -> realizeBPFState() -> ReloadDatapath() -> -> ReloadDatapath() -> reloadHostDatapath() -> patchHostNetdevDatapath() -> ELFSubstitutions() -> elfVariableSubstitutions() Uses epInfoCache --> Not vulnerable * pkg/envoy/server.go e.updateNetworkPolicy() -> UpdateNetworkPolicy() The endpoint is locked. Must use a non-locking variation. --> Not vulnerable * pkg/hubble/parser/threefour/parser.go Decode() -> resolveEndpoint() --> Vulnerable Fixes: #11932 Signed-off-by: Thomas Graf <thomas@cilium.io> Signed-off-by: Joe Stringer <joe@cilium.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Needs triage to assess how serious the bug is because to me it looks like
endpoint.SecurityIdentityis not as protected as it should be.The text was updated successfully, but these errors were encountered: