Skip to content

Commit 86c5f61

Browse files
committed
fix(server): improve label matching and code clarity in HugeAuthenticator
1. Introduced a safe wildcard-based label matching method to prevent ReDoS attacks, replacing direct regex usage. 2. Refactored code for better readability, reordered admin checks, and made minor comment and formatting improvements throughout HugeAuthenticator.java. 3. Enhance zip extraction security with path validation
1 parent aaec4da commit 86c5f61

File tree

2 files changed

+57
-41
lines changed
  • hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth
  • hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/util

2 files changed

+57
-41
lines changed

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java

Lines changed: 50 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.util.HashMap;
2121
import java.util.List;
2222
import java.util.Map;
23+
import java.util.regex.Pattern;
2324

2425
import org.apache.hugegraph.HugeException;
2526
import org.apache.hugegraph.HugeGraph;
@@ -135,10 +136,9 @@ static HugeAuthenticator loadAuthenticator(HugeConfig conf) {
135136
ClassLoader cl = conf.getClass().getClassLoader();
136137
try {
137138
authenticator = (HugeAuthenticator) cl.loadClass(authClass)
138-
.newInstance();
139+
.getDeclaredConstructor().newInstance();
139140
} catch (Exception e) {
140-
throw new HugeException("Failed to load authenticator: '%s'",
141-
authClass, e);
141+
throw new HugeException("Failed to load authenticator: '%s'", authClass, e);
142142
}
143143

144144
authenticator.setup(conf);
@@ -290,7 +290,7 @@ private static Object matchedAction(HugePermission action,
290290
}
291291
for (Map.Entry<HugePermission, Object> e : perms.entrySet()) {
292292
HugePermission permission = e.getKey();
293-
// May be required = ANY
293+
// Maybe required = ANY
294294
if (action.match(permission) ||
295295
action.equals(HugePermission.EXECUTE)) {
296296
// Return matched resource of corresponding action
@@ -324,12 +324,13 @@ public static boolean matchApiRequiredPerm(Object role, RequiredPerm requiredPer
324324

325325
public static boolean match(Object role, HugePermission required,
326326
ResourceObject<?> resourceObject) {
327-
if (RolePermission.isAdmin((RolePermission) role)) {
328-
return true;
329-
}
330327
if (role == null || ROLE_NONE.equals(role)) {
331328
return false;
332329
}
330+
if (RolePermission.isAdmin((RolePermission) role)) {
331+
return true;
332+
}
333+
333334
RolePerm rolePerm = RolePerm.fromJson(role);
334335
// Check if user is space manager(member cannot operate auth api)
335336
if (rolePerm.matchSpace(resourceObject.graphSpace(), "space")) {
@@ -340,13 +341,14 @@ public static boolean match(Object role, HugePermission required,
340341

341342
public static boolean match(Object role, RolePermission grant,
342343
ResourceObject<?> resourceObject) {
343-
if (RolePermission.isAdmin((RolePermission) role)) {
344-
return true;
345-
}
346344
if (role == null || ROLE_NONE.equals(role)) {
347345
return false;
348346
}
349347

348+
if (RolePermission.isAdmin((RolePermission) role)) {
349+
return true;
350+
}
351+
350352
if (resourceObject != null) {
351353
SchemaDefine.AuthElement element =
352354
(SchemaDefine.AuthElement) resourceObject.operated();
@@ -427,8 +429,7 @@ private boolean matchResource(HugePermission requiredAction,
427429

428430
// * or {graph}
429431
String owner = requiredResource.graph();
430-
for (Map.Entry<String, Map<HugePermission, Object>> e :
431-
innerRoles.entrySet()) {
432+
for (Map.Entry<String, Map<HugePermission, Object>> e : innerRoles.entrySet()) {
432433
if (!matchedPrefix(e.getKey(), owner)) {
433434
continue;
434435
}
@@ -445,12 +446,10 @@ private boolean matchResource(HugePermission requiredAction,
445446
continue;
446447
}
447448

448-
Map<String, List<HugeResource>> ressMap = (Map<String,
449-
List<HugeResource>>) permission;
449+
var ressMap = (Map<String, List<HugeResource>>) permission;
450450

451451
ResourceType requiredType = requiredResource.type();
452-
for (Map.Entry<String, List<HugeResource>> entry :
453-
ressMap.entrySet()) {
452+
for (Map.Entry<String, List<HugeResource>> entry : ressMap.entrySet()) {
454453
String[] typeLabel = entry.getKey().split(POUND_SEPARATOR);
455454
ResourceType type = ResourceType.valueOf(typeLabel[0]);
456455
/* assert one type can match but not equal to other only
@@ -463,33 +462,28 @@ private boolean matchResource(HugePermission requiredAction,
463462
}
464463

465464
// check label
466-
String requiredLabel = null;
465+
String requiredLabel;
467466
if (requiredType.isSchema()) {
468-
requiredLabel =
469-
((Nameable) requiredResource.operated()).name();
467+
requiredLabel = ((Nameable) requiredResource.operated()).name();
470468
} else if (requiredType.isGraph()) {
471469
if (requiredResource.operated() instanceof HugeElement) {
472-
requiredLabel =
473-
((HugeElement) requiredResource.operated()).label();
470+
requiredLabel = ((HugeElement) requiredResource.operated()).label();
474471
} else {
475-
requiredLabel =
476-
((Nameable) requiredResource.operated()).name();
477-
472+
requiredLabel = ((Nameable) requiredResource.operated()).name();
478473
}
479474
} else {
480475
return true;
481476
}
482477
String label = typeLabel[1];
483-
if (!(ANY.equals(label) || "null".equals(label)
484-
|| requiredLabel.matches(label))) {
478+
if (!(ANY.equals(label) ||
479+
"null".equals(label) || matchLabel(requiredLabel, label))) {
485480
continue;
486481
} else if (requiredType.isSchema()) {
487482
return true;
488483
}
489484

490485
// check properties
491-
List<HugeResource> ress =
492-
ressMap.get(type + POUND_SEPARATOR + label);
486+
List<HugeResource> ress = ressMap.get(type + POUND_SEPARATOR + label);
493487

494488
for (HugeResource res : ress) {
495489
if (res.filter(requiredResource)) {
@@ -500,6 +494,28 @@ private boolean matchResource(HugePermission requiredAction,
500494
}
501495
return false;
502496
}
497+
498+
/**
499+
* Safely match a label pattern against the required label.
500+
* Prevents ReDoS attacks by using controlled wildcard matching instead of
501+
* arbitrary regex patterns.
502+
*
503+
* @param requiredLabel the label to match against
504+
* @param pattern the pattern (may contain * and ? wildcards)
505+
* @return true if the label matches the pattern
506+
*/
507+
private static boolean matchLabel(String requiredLabel, String pattern) {
508+
// Use simple wildcard matching instead of arbitrary regex
509+
if (pattern.contains("*") || pattern.contains("?")) {
510+
// Convert pattern to safe regex: escape special chars, then convert wildcards
511+
String regex = Pattern.quote(pattern)
512+
.replace("\\*", ".*")
513+
.replace("\\?", ".");
514+
return Pattern.matches(regex, requiredLabel);
515+
}
516+
// Simple equality check if no wildcards
517+
return requiredLabel.equals(pattern);
518+
}
503519
}
504520

505521
class RequiredPerm {
@@ -592,7 +608,7 @@ private void parseAction(String action) {
592608
int offset = action.lastIndexOf('_');
593609
if (0 < offset && ++offset < action.length()) {
594610
/*
595-
* In order to be compatible with the old permission mechanism,
611+
* To be compatible with the old permission mechanism,
596612
* here is only to provide pre-control by extract the
597613
* resource_action {vertex/edge/schema}_{read/write},
598614
* resource_action like vertex_read.
@@ -604,20 +620,17 @@ private void parseAction(String action) {
604620
this.action = HugePermission.valueOf(action.toUpperCase());
605621
}
606622

607-
public static String roleFor(String graphSpace, String owner,
608-
HugePermission perm) {
623+
public static String roleFor(String graphSpace, String owner, HugePermission perm) {
609624
/*
610-
* Construct required permission such as:
625+
* Construct required permission such as
611626
* $owner=graph1 $action=read
612627
* (means required read permission of any one resource)
613628
*
614629
* In the future maybe also support:
615630
* $owner=graph1 $action=vertex_read
616631
*/
617-
return String.format("%s=%s %s=%s %s=%s",
618-
KEY_GRAPHSPACE, graphSpace,
619-
KEY_OWNER, owner,
620-
KEY_ACTION, perm.string());
632+
return String.format("%s=%s %s=%s %s=%s", KEY_GRAPHSPACE, graphSpace,
633+
KEY_OWNER, owner, KEY_ACTION, perm.string());
621634
}
622635

623636
public static RequiredPerm fromJson(String json) {
@@ -626,8 +639,7 @@ public static RequiredPerm fromJson(String json) {
626639

627640
public ResourceObject<?> resourceObject() {
628641
Nameable elem = HugeResource.NameObject.ANY;
629-
return ResourceObject.of(this.graphSpace, this.owner,
630-
this.resource, elem);
642+
return ResourceObject.of(this.graphSpace, this.owner, this.resource, elem);
631643
}
632644
}
633645
}

hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/util/ZipUtils.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,16 @@ public static void decompress(final String sourceFile, final String outputDir,
9292
final CheckedInputStream cis = new CheckedInputStream(fis, checksum);
9393
final ZipInputStream zis = new ZipInputStream(new BufferedInputStream(cis))) {
9494
ZipEntry entry;
95+
final String canonicalOutputPath = new File(outputDir).getCanonicalPath();
9596
while ((entry = zis.getNextEntry()) != null) {
9697
final String fileName = entry.getName();
97-
if (fileName.contains("..")) {
98-
throw new IOException("Entry with an illegal path: " + fileName);
99-
}
10098
final File entryFile = new File(Paths.get(outputDir, fileName).toString());
99+
final String canonicalEntryPath = entryFile.getCanonicalPath();
100+
101+
// Validate that the entry is within the output directory
102+
if (!canonicalEntryPath.startsWith(canonicalOutputPath + File.separator)) {
103+
throw new IOException("Entry is outside of the target dir: " + fileName);
104+
}
101105
FileUtils.forceMkdir(entryFile.getParentFile());
102106
try (final FileOutputStream fos = new FileOutputStream(entryFile);
103107
final BufferedOutputStream bos = new BufferedOutputStream(fos)) {

0 commit comments

Comments
 (0)