Skip to content

Avoid reflective final field mutation in BasicLookupStrategy #19331

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.

Sign up for GitHub

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

Open
seonwooj0810 wants to merge 1 commit into
base: main
Choose a base branch
from
+32 −36
Open

Avoid reflective final field mutation in BasicLookupStrategy #19331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 16 additions & 36 deletions acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2004, 2005, 2006, 2017 Acegi Technology Pty Limited
* Copyright 2004, 2005, 2006, 2017, 2026 Acegi Technology Pty Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -127,8 +127,6 @@ public class BasicLookupStrategy implements LookupStrategy {

private final Field fieldAces = FieldUtils.getField(AclImpl.class, "aces");

private final Field fieldAcl = FieldUtils.getField(AccessControlEntryImpl.class, "acl");

// SQL Customization fields
private String selectClause = DEFAULT_SELECT_CLAUSE;

Expand Down Expand Up @@ -171,7 +169,6 @@ public BasicLookupStrategy(DataSource dataSource, AclCache aclCache,
this.objectIdentityGenerator = new ObjectIdentityRetrievalStrategyImpl();
this.aclClassIdUtils = new AclClassIdUtils();
this.fieldAces.setAccessible(true);
this.fieldAcl.setAccessible(true);
}

private String computeRepeatingSql(String repeatingSql, int requiredRepetitions) {
Expand Down Expand Up @@ -201,22 +198,11 @@ private List<AccessControlEntryImpl> readAces(AclImpl acl) {
}
}

private void setAclOnAce(AccessControlEntryImpl ace, AclImpl acl) {
try {
this.fieldAcl.set(ace, acl);
}
catch (IllegalAccessException ex) {
throw new IllegalStateException("Could not or set AclImpl on AccessControlEntryImpl fields", ex);
}
}

private void setAces(AclImpl acl, List<AccessControlEntryImpl> aces) {
try {
this.fieldAces.set(acl, aces);
}
catch (IllegalAccessException ex) {
throw new IllegalStateException("Could not set AclImpl entries", ex);
}
private void addAces(AclImpl acl, List<AccessControlEntryImpl> aces) {
// Populate the existing aces list in place rather than replacing the
// (final) field reference via reflection, which Java 26+ blocks. The
// list itself is mutable; only the field that holds it is final.
readAces(acl).addAll(aces);
}

/**
Expand Down Expand Up @@ -407,25 +393,19 @@ private AclImpl convert(Map<Serializable, Acl> inputMap, Long currentIdentity) {
AclImpl result = new AclImpl(inputAcl.getObjectIdentity(), inputAcl.getId(), this.aclAuthorizationStrategy,
this.grantingStrategy, parent, null, inputAcl.isEntriesInheriting(), owner);

// Copy the "aces" from the input to the destination

// Obtain the "aces" from the input ACL
// Copy the "aces" from the input to the destination.
//
// Re-create each AccessControlEntryImpl rather than mutating its final
// "acl" back-reference via reflection, so that StubAclParent references
// are replaced with the new "result" AclImpl instance (as per SEC-951).
// Java 26+ blocks reflective mutation of final fields.
List<AccessControlEntryImpl> aces = readAces(inputAcl);

// Create a list in which to store the "aces" for the "result" AclImpl instance
List<AccessControlEntryImpl> acesNew = new ArrayList<>();

// Iterate over the "aces" input and replace each nested
// AccessControlEntryImpl.getAcl() with the new "result" AclImpl instance
// This ensures StubAclParent instances are removed, as per SEC-951
List<AccessControlEntryImpl> acesNew = new ArrayList<>(aces.size());
for (AccessControlEntryImpl ace : aces) {
setAclOnAce(ace, result);
acesNew.add(ace);
acesNew.add(new AccessControlEntryImpl(ace.getId(), result, ace.getSid(), ace.getPermission(),
ace.isGranting(), ace.isAuditSuccess(), ace.isAuditFailure()));
}

// Finally, now that the "aces" have been converted to have the "result" AclImpl
// instance, modify the "result" AclImpl instance
setAces(result, acesNew);
addAces(result, acesNew);

return result;
}
Expand Down
16 changes: 16 additions & 0 deletions ...rc/test/java/org/springframework/security/acls/jdbc/AbstractBasicLookupStrategyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.springframework.security.acls.domain.ObjectIdentityImpl;
import org.springframework.security.acls.domain.PrincipalSid;
import org.springframework.security.acls.domain.SpringCacheBasedAclCache;
import org.springframework.security.acls.model.AccessControlEntry;
import org.springframework.security.acls.model.Acl;
import org.springframework.security.acls.model.AuditableAccessControlEntry;
import org.springframework.security.acls.model.MutableAcl;
Expand Down Expand Up @@ -320,6 +321,21 @@ public void testCreateGrantedAuthority() {
assertThat(((GrantedAuthoritySid) result).getGrantedAuthority()).isEqualTo("sid");
}

// gh-19127
@Test
public void readAclsByIdWhenChildLoadedThenAcesPointToConvertedAcl() {
ObjectIdentity topParentOid = new ObjectIdentityImpl(TARGET_CLASS, 100L);
ObjectIdentity middleParentOid = new ObjectIdentityImpl(TARGET_CLASS, 101L);
ObjectIdentity childOid = new ObjectIdentityImpl(TARGET_CLASS, 102L);
Map<ObjectIdentity, Acl> map = this.strategy
.readAclsById(Arrays.asList(topParentOid, middleParentOid, childOid), null);
for (Acl acl : map.values()) {
for (AccessControlEntry ace : acl.getEntries()) {
assertThat(ace.getAcl()).isSameAs(acl);
}
}
}

@Test
public void setObjectIdentityGeneratorWhenNullThenThrowsIllegalArgumentException() {
// @formatter:off
Expand Down