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

Default value for Pod.spec.containers.securityContext.allowPrivilegeEscalation #30104

Open
dilyanpalauzov opened this issue Oct 15, 2021 · 21 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. language/en Issues or PRs related to English language sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/security Categorizes an issue or PR as relevant to SIG Security. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@dilyanpalauzov
Copy link

At https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#security-context-1 for

securityContext.allowPrivilegeEscalation (boolean)

it is not described what the default value is (when the container has no CAP_SYS_ADMIN and is not run as privileged)

@dilyanpalauzov dilyanpalauzov added the kind/bug Categorizes issue or PR as related to a bug. label Oct 15, 2021
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 15, 2021
@dilyanpalauzov
Copy link
Author

Moreover the text about allowPrivilegeEscalation is:

AllowPrivilegeEscalation is true always when the container is: 1) run as Privileged 2) has CAP_SYS_ADMIN

Does this mean “1) or 2)” or it means “1) and 2)”?

@sftim
Copy link
Contributor

sftim commented Oct 16, 2021

/sig auth
/sig security
/language en

This might be an API reference generation bug?

@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/security Categorizes an issue or PR as relevant to SIG Security. language/en Issues or PRs related to English language labels Oct 16, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 14, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 13, 2022
@enj
Copy link
Member

enj commented Feb 14, 2022

/remove-lifecycle rotten
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 14, 2022
@mk46
Copy link
Member

mk46 commented Mar 3, 2022

Moreover the text about allowPrivilegeEscalation is:

AllowPrivilegeEscalation is true always when the container is: 1) run as Privileged 2) has CAP_SYS_ADMIN

Does this mean “1) or 2)” or it means “1) and 2)”?

I think 1) and 2). See the validation https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L6178-L6191

@dilyanpalauzov
Copy link
Author

Please use “and” / “or” in the documentation, when glueing 1) with 2).

@mk46
Copy link
Member

mk46 commented Mar 3, 2022

AllowPrivilegeEscalation controls whether a process can gain more
privileges than its parent process. This bool directly controls if
the no_new_privs flag will be set on the container process. 
Default to false.
AllowPrivilegeEscalation is true always when the container is:
1. run as Privileged and
2. has CAP_SYS_ADMIN
Note that this field cannot be set when spec.os.name is windows.

if above make sense as per https://github.com/kubernetes/kubernetes/blob/4d78db54a58e250b049c4fe17ac484e5c3ec662d/staging/src/k8s.io/pod-security-admission/policy/check_allowPrivilegeEscalation.go and https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L6178-L6191 then
I am happy to open a PR. @enj Feel free to assign me. Thanks!!

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2022
@mk46
Copy link
Member

mk46 commented Jun 1, 2022

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 1, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 30, 2022
@mk46
Copy link
Member

mk46 commented Aug 31, 2022

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 31, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 29, 2022
@enj enj moved this to Backlog in SIG Auth Dec 5, 2022
@enj enj added this to SIG Auth Dec 5, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 29, 2022
@divya-mohan0209
Copy link
Contributor

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 18, 2023
@0xbf00
Copy link

0xbf00 commented Mar 17, 2023

AllowPrivilegeEscalation controls whether a process can gain more
privileges than its parent process. This bool directly controls if
the no_new_privs flag will be set on the container process. 
Default to false.
AllowPrivilegeEscalation is true always when the container is:
1. run as Privileged and
2. has CAP_SYS_ADMIN
Note that this field cannot be set when spec.os.name is windows.```

I believe this description is not (no longer?) correct, because:

  1. AllowPrivilegeEscalation defaults to true, see this code. The code returns false both if AllowPrivilegeEscalation is not set and if AllowPrivilegeEscalation is set to true. This is also supported by my understanding of the code you already linked. Here, containers with container.SecurityContext.AllowPrivilegeEscalation == nil are flagged as bad by the script.
  2. the current implementation adds an error if either the container is privileged or has the CAP_SYS_ADMIN capability.

Note however that therefore and confusingly, no_new_privs defaults to false. The documentation however describes AllowPrivilegeEscalation and not no_new_privs directly.

However, I don't have the full context here. @mk46 Please let me know if I missed anything.

See also the description in Datree's documentation:

Privileged escalation allows a process to change the security context under which its running. In their default state, containers allow privilege escalation. Attackers may use this default behavior to manipulate the application or process and to gain more permissions than they should have.

If my analysis is correct, the following text makes sense:

AllowPrivilegeEscalation controls whether a process can gain more
privileges than its parent process. This bool directly controls if
the `no_new_privs` flag will be set on the container process. 
Defaults to `true`.
AllowPrivilegeEscalation is `true` always when the container is:
1. run as Privileged or
2. has CAP_SYS_ADMIN
Note that this field cannot be set when spec.os.name is windows.```

@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Mar 16, 2024
@natalisucks
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 30, 2024
@dbowling
Copy link

dbowling commented Oct 9, 2024

@0xbf00 said:

AllowPrivilegeEscalation defaults to true

When I read this, I see it as defaulting to false.

// handle the case where the user did not set the default and did not explicitly set allowPrivilegeEscalation
	if sc.AllowPrivilegeEscalation == nil {
		return false
	}

https://github.com/kubernetes/kubernetes/blob/d9c46d8ecb1ede9be30545c9803e17682fcc4b50/pkg/securitycontext/util.go#L175-L187

This seems like an important security setting to have clarity around the behavior. If there's a consensus that this actually does default to false, I'd be happy to make a PR with the original proposal from @mk46

AllowPrivilegeEscalation controls whether a process can gain more
privileges than its parent process. This bool directly controls if
the no_new_privs flag will be set on the container process. 
Default to false.
AllowPrivilegeEscalation is true always when the container is:
1. run as Privileged and
2. has CAP_SYS_ADMIN
Note that this field cannot be set when spec.os.name is windows.

(I don't know where the text from kubectl explain lives in the code yet, but can dig for it.)

As well as the website:

* `allowPrivilegeEscalation`: Controls whether a process can gain more privileges than
its parent process. This bool directly controls whether the
[`no_new_privs`](https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt)
flag gets set on the container process.
`allowPrivilegeEscalation` is always true when the container:
- is run as privileged, or
- has `CAP_SYS_ADMIN`

@0xbf00
Copy link

0xbf00 commented Oct 9, 2024

@dbowling The code that we both linked to relates to a derived property (no_new_privs), not allowPrivilegeEscalation. The return false here:

// AddNoNewPrivileges returns if we should add the no_new_privs option.
func AddNoNewPrivileges(sc *v1.SecurityContext) bool {
    // ...
    // handle the case where the user did not set the default and did not explicitly set allowPrivilegeEscalation
    if sc.AllowPrivilegeEscalation == nil {
        return false
    }
    // ...
}

negates a negation. After this code, if allowPrivilegeEscalation is not set, no_new_privs is set to false. A double no is a yes. no_new_privs == false is basically the same as new_privs == true (this variable does not exist in the code, I made it up for this point). So, this to me means that new privileges can be obtained and allowPrivilegeEscalation -- the original boolean this issue is about -- must be be considered to be (logically) true by default.

To be fair, I have not investigated this any more other than reading your comment and checking the linked code, but I cannot help but notice that the points I raised originally all seem to apply to the current implementation which you also referenced. Yet your conclusion is the exact opposite of my conclusion.

If you @dbowling or somebody else can let me know what I'm missing I'd be really grateful.

If I'm not missing anything this is really, really confusing code that should not exist.

@dbowling
Copy link

dbowling commented Oct 9, 2024

@0xbf00

If I'm not missing anything this is really, really confusing code that should not exist.

I'll agree that the code is confusing, for sure.

negates a negation. After this code, if allowPrivilegeEscalation is not set, no_new_privs is set to false. A double no is a yes.

Ugh, I may be reading things wrong then. The code comment says that when this is true, add the no_new_privs option (I think.) Which would mean what you have said makes sense to me now (even though it required mental gymnastics to get there.)

The more I look at the docs, code, and history of the proposal, the more I realize I am not qualified to determine what is going on here.

Before arriving here, I initially reviewed the archived design proposal, which had this table:

allowPrivilegeEscalation setting uid = 0 or unset uid != 0 privileged/CAP_SYS_ADMIN
nil no_new_privs=true no_new_privs=false no_new_privs=false
false no_new_privs=true no_new_privs=true no_new_privs=false
true no_new_privs=false no_new_privs=false no_new_privs=false

Further, the last line of that doc says:

A new bool field named allowPrivilegeEscalation will be added to the Pod Security Policy as well to gate whether or not a user is allowed to set the security context to allowPrivilegeEscalation=true. This field will default to false.

Specifically, I'm calling out This field will default to false.

I think that gets to some of what is at question. I'm not familiar with the Kubernetes process on if the proposals are ever updated as the code changes, so that coupled with the banner "repository has been archived by the owner on Dec 2, 2021" means it is just background info.

I do find it interesting that none of the code I'm looking at from the links in this issue seems to indicate that the no_new_privs actually relates to the uid, which I understand the thinking for, but from a developer experience perspective makes this setting in it's default rather hard to reason about.

I'm just trying to figure out if I need to enforce a setting on this for all the workloads in my cluster, or if I am safe with the defaults, and this just furthers the need to give better guidance in the Kubernetes docs on the issue.

In case anyone comes to this issue to figure more out, I'll also point to a resource that went into more depth than others: https://blog.christophetd.fr/stop-worrying-about-allowprivilegeescalation/.

The author (@christophetd - a cloud security researcher & advocate at Datadog) claims the default value is true (although he cites this issue as one of the sources, so that might by cyclical reasoning.)

He also demonstrates a scenario where setreuid is used in a container under allowPrivilegeEscalation for both true and false. I suppose the final state could be determine by following his instructions with a default setting and see if the true or false behavior applies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. language/en Issues or PRs related to English language sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/security Categorizes an issue or PR as relevant to SIG Security. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

10 participants