Skip to content

Commit

Permalink
[SECURITY-2180]
Browse files Browse the repository at this point in the history
  • Loading branch information
@daniel-beck
daniel-beck committed Mar 10, 2021
1 parent b698c30 commit bbe3585
Show file tree
Hide file tree
Showing 6 changed files with 600 additions and 47 deletions.
1 change: 1 addition & 0 deletions src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public ACL getACL(@Nonnull Job<?,?> project) {
}

@Restricted(NoExternalUse.class)
@Deprecated // Unused since SECURITY-2180 fix, TODO insert specific versions
public static ACL inheritingACL(final ACL parent, final ACL child) {
if (parent instanceof SidACL && child instanceof SidACL) {
return ((SidACL) child).newInheritingACL((SidACL) parent);
Expand Down
35 changes: 29 additions & 6 deletions src/main/java/org/jenkinsci/plugins/matrixauth/inheritance/InheritGlobalStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@
package org.jenkinsci.plugins.matrixauth.inheritance;

import hudson.Extension;
import hudson.model.Item;
import hudson.security.ACL;
import hudson.security.AccessControlled;
import hudson.security.ProjectMatrixAuthorizationStrategy;
import jenkins.model.Jenkins;
import hudson.security.Permission;
import org.acegisecurity.Authentication;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.util.Arrays;

/**
* Strategy that inherits only the global ACL -- parent, grandparent, etc. ACLs are not inherited.
Expand All @@ -42,10 +44,31 @@ public class InheritGlobalStrategy extends InheritanceStrategy {
public InheritGlobalStrategy() {

}

@Override
public ACL getEffectiveACL(ACL acl, AccessControlled subject) {
return ProjectMatrixAuthorizationStrategy.inheritingACL(Jenkins.get().getAuthorizationStrategy().getRootACL(), acl);
protected boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission, ACL child, @CheckForNull ACL parent, ACL root) {
if (a.equals(ACL.SYSTEM)) {
return true;
}
if (isParentReadPermissionRequired() && parent != null && (Item.READ.equals(permission) || Item.DISCOVER.equals(permission))) {
/*
* We need special handling for Read/Discover permissions to prevent SECURITY-2180:
* Item/Read is expected to only be effective if it is granted on every ancestor, similar to how permissions
* granted while lacking Overall/Read are pointless.
* If and only if we check for Item/Read or Item/Discover, do not fall back to the permission granted globally.
* No need to check #isUltimatelyImpliedByAdminister like NonInheritingStrategy does, we know it to be true for these permissions.
*
* This is a nested element.
* We need to ensure that all of the following are true:
* - The permission is granted in the parent
* - The permission is granted globally or explicitly on this element (the child)
*/
final boolean grantedViaChild = child.hasPermission(a, permission);
final boolean grantedGlobally = root.hasPermission(a, permission);
final boolean grantedInParent = parent.hasPermission(a, permission);
return (grantedViaChild || grantedGlobally) && grantedInParent;
}
return child.hasPermission(a, permission) || root.hasPermission(a, permission);
}

@Symbol("inheritingGlobal")
Expand Down
42 changes: 26 additions & 16 deletions src/main/java/org/jenkinsci/plugins/matrixauth/inheritance/InheritParentStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,16 @@

import hudson.Extension;
import hudson.model.AbstractItem;
import hudson.model.ItemGroup;
import hudson.model.Item;
import hudson.security.ACL;
import hudson.security.AccessControlled;
import hudson.security.ProjectMatrixAuthorizationStrategy;
import jenkins.model.Jenkins;
import hudson.security.Permission;
import org.acegisecurity.Authentication;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.util.Arrays;

/**
* Strategy that inherits the ACL from the parent.
Expand All @@ -48,19 +49,28 @@ public InheritParentStrategy() {
}

@Override
public ACL getEffectiveACL(ACL acl, AccessControlled subject) {
if (subject instanceof AbstractItem) {
AbstractItem item = (AbstractItem) subject;
ItemGroup<?> parent = item.getParent();
final ACL parentACL;
if (parent instanceof AbstractItem) {
parentACL = Jenkins.get().getAuthorizationStrategy().getACL((AbstractItem) parent);
} else {
parentACL = Jenkins.get().getAuthorizationStrategy().getRootACL();
}
return ProjectMatrixAuthorizationStrategy.inheritingACL(parentACL, acl);
protected boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission, ACL child, @CheckForNull ACL parent, ACL root) {
if (a.equals(ACL.SYSTEM)) {
return true;
}
if (isParentReadPermissionRequired() && parent != null && (Item.READ.equals(permission) || Item.DISCOVER.equals(permission))) {
/*
* If we have an item parent, only grant Item/Read and Item/Discover if it's granted on the parent.
* In this case, it doesn't even matter whether it's explicitly granted on the child.
*/
return parent.hasPermission(a, permission);
}
if (parent == null) {
/*
* Without an item parent (i.e. topmost level item) we need to check both grants on this item, as
* well as grants on the root (parent) ACL:
* - Explicitly granted here but possibly not globally (on root): That's OK
* - NOT explicitly granted here, but globally: That's also OK
*/
return root.hasPermission(a, permission) || child.hasPermission(a, permission);
} else {
throw new IllegalArgumentException("Expected subject to be AbstractItem, but got " + subject);
/* If we have an item parent, check both explicit grants here and inherited permissions from parent. */
return parent.hasPermission(a, permission) || child.hasPermission(a, permission);
}
}

Expand Down
40 changes: 39 additions & 1 deletion src/main/java/org/jenkinsci/plugins/matrixauth/inheritance/InheritanceStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,52 @@

import hudson.ExtensionPoint;
import hudson.model.AbstractDescribableImpl;
import hudson.model.AbstractItem;
import hudson.model.ItemGroup;
import hudson.security.ACL;
import hudson.security.AccessControlled;
import hudson.security.Permission;
import jenkins.model.Jenkins;
import org.acegisecurity.Authentication;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

public abstract class InheritanceStrategy extends AbstractDescribableImpl<InheritanceStrategy> implements ExtensionPoint {
@Restricted(NoExternalUse.class)
/* package */ static boolean isParentReadPermissionRequired() {
// TODO Switch to SystemProperties in 2.236+
String propertyName = hudson.security.AuthorizationMatrixProperty.class.getName() + ".checkParentPermissions";
String value = System.getProperty(propertyName);
if (value == null) {
return true;
}
return Boolean.parseBoolean(value);
}

@Override
public InheritanceStrategyDescriptor getDescriptor() {
return (InheritanceStrategyDescriptor) super.getDescriptor();
}

public abstract ACL getEffectiveACL(ACL acl, AccessControlled subject);
@CheckForNull
private ACL getParentItemACL(AccessControlled accessControlled) {
ACL parentACL = null;
if (accessControlled instanceof AbstractItem) {
AbstractItem item = (AbstractItem) accessControlled;
ItemGroup<?> parent = item.getParent();
if (parent instanceof AbstractItem) {
parentACL = Jenkins.get().getAuthorizationStrategy().getACL((AbstractItem) parent);
}
}
return parentACL;
}

public ACL getEffectiveACL(final ACL acl, final AccessControlled subject) {
return ACL.lambda((a, p) -> hasPermission(a, p, acl, getParentItemACL(subject), Jenkins.get().getAuthorizationStrategy().getRootACL()));
}

protected abstract boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission, ACL child, @CheckForNull ACL parent, ACL root);
}
59 changes: 35 additions & 24 deletions src/main/java/org/jenkinsci/plugins/matrixauth/inheritance/NonInheritingStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,17 @@
package org.jenkinsci.plugins.matrixauth.inheritance;

import hudson.Extension;
import hudson.model.Item;
import hudson.security.ACL;
import hudson.security.AccessControlled;
import hudson.security.Permission;
import jenkins.model.Jenkins;
import org.acegisecurity.Authentication;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.util.Arrays;

/**
* Strategy that disables inheritance except for the globally defined Administer permission.
Expand All @@ -45,30 +47,39 @@ public NonInheritingStrategy() {
}

@Override
public ACL getEffectiveACL(ACL acl, AccessControlled subject) {
final ACL rootACL = Jenkins.get().getAuthorizationStrategy().getRootACL();
return new ACL() {
@Override
public boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission) {
/*
I see two possible approaches here:
One would be to just grant every permission if the root ACL grants Administer.
This could result in weird situations where disabling inheritance would grant permissions like the optional
Run/Artifacts permission not implied by anything else.
The chosen, second approach checks whether the given permission is ultimately (transitively) implied by
Administer, and, if so, grants it if the user has Administer.
As this is a tree, any permission implication rooted in Administer should then be granted to administrators.
*/
return isUltimatelyImpliedByAdminister(permission) && rootACL.hasPermission(a, Jenkins.ADMINISTER) || acl.hasPermission(a, permission);
}
protected boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission, ACL child, @CheckForNull ACL parent, ACL root) {
if (a.equals(ACL.SYSTEM)) {
return true;
}
if (isUltimatelyImpliedByAdminister(permission) && root.hasPermission(a, Jenkins.ADMINISTER)) {
/*
* I see two possible approaches here:
* One would be to just grant every permission if the root ACL grants Administer.
* This could result in weird situations where disabling inheritance would grant permissions like the optional
* Run/Artifacts permission not implied by anything else.
* The chosen, second approach checks whether the given permission is ultimately (transitively) implied by
* Administer, and, if so, grants it if the user has Administer.
* As this is a tree, any permission implication rooted in Administer should then be granted to administrators.
*/
return true;
}
if (isParentReadPermissionRequired() && parent != null && (Item.READ.equals(permission) || Item.DISCOVER.equals(permission))) {
/*
* We are not inheriting permissions from the parent, but we only grant Read permission if the parent
* also has Read permission.
*/
return parent.hasPermission(a, permission) && child.hasPermission(a, permission);
} else {
/* Only grant permission if it is explicitly granted here. */
return child.hasPermission(a, permission);
}
}

private boolean isUltimatelyImpliedByAdminister(Permission permission) {
while (permission.impliedBy != null) {
permission = permission.impliedBy;
}
return permission == Jenkins.ADMINISTER;
}
};
private static boolean isUltimatelyImpliedByAdminister(Permission permission) {
while (permission.impliedBy != null) {
permission = permission.impliedBy;
}
return permission == Jenkins.ADMINISTER;
}

@Symbol("nonInheriting")
Expand Down
Loading

0 comments on commit bbe3585

Please sign in to comment.