diff --git a/SECURITY_CONFIG.md b/SECURITY_CONFIG.md index ad96085bb..3c1caec3c 100644 --- a/SECURITY_CONFIG.md +++ b/SECURITY_CONFIG.md @@ -155,6 +155,45 @@ trust.host = *.example.com - `*.example.com` 会匹配 `cdn.example.com`、`api.internal.example.com`，但不匹配根域 `example.com` - 对于 IP 风格通配（如 `192.168.*`、`10.*`），仅匹配字面量 IPv4 地址，不匹配域名 +## 🧱 应用层防护 Guard Core（可选，默认关闭） + +kkFileView 可以选择启用 [Guard Core](https://github.com/Guard-Core/spring-boot-guard)（spring-boot-guard，MIT），在应用内增加请求频率限制、攻击特征检测和自动封禁。它**不替代** `trust.host` / `not.trust.host` 和各个安全修复，只限制谁能访问这些接口、访问多快，并封禁持续探测的客户端。 + +Guard Core is an optional, off-by-default application-layer guard (rate limits, attack screening, auto-ban). It does not replace `trust.host` / `not.trust.host` or the code fixes for the advisories. It limits who can reach the routes, how fast, and bans clients that keep probing. + +### 启用 / Enable + +```properties +guard.enabled = true +# 或环境变量 / or: KK_GUARD_ENABLED=true +``` + +不设置 `guard.enabled` 或设置为 `false` 时，不会注册任何 Guard 组件，行为与之前完全一致。建议先用 `KK_GUARD_PASSIVE_MODE=true`（只记录不拦截）观察一段时间，再关闭观察模式。完整配置见 `application.properties` 的“十五、Guard Core 应用层防护”。 + +### 它做什么 / What it does + +| 风险 / Risk | 措施 / Measure | 配置 / Setting | +|---|---|---| +| 转换、预览、远程读取、入队被刷（LibreOffice 转换消耗大量 CPU） | 每个客户端的全局和分接口请求限制，超限返回 429 | `guard.rate-limit`、`guard.endpoint-rate-limits[...]` | +| 目录穿越、注入、XSS、扫描探测（路径、查询参数、请求头、请求体，包含 Base64 解码后的 url） | 攻击特征检测，返回 400，持续触发的客户端自动封禁 | `guard.enabled-detection-categories`、`guard.auto-ban-*` | +| 反复读取不存在的远程文件 | 同一接口累计大量 404 的客户端封禁 | `guard.global-behavior-rules[0]` | +| 删除密码被猜解 | 删除失败超过 5 次的客户端封禁 1 小时 | `FileController#deleteFile` 上的 `@GuardReturnMonitor` | +| 上传请求过大或类型不对 | `/fileUpload` 只接受 `multipart/form-data`，Content-Length 超过 500MB 直接返回 413 | `FileController#fileUpload` 上的注解 | +| 写操作接口被匿名调用（上传、创建文件夹、删除、addTask） | IP 白名单和/或密钥（请求头 `X-KK-Admin-Key`），同时设置时两者都必须满足 | `KK_GUARD_ADMIN_ALLOWED_IPS`、`KK_GUARD_ADMIN_KEY` | + +### 诚实的限制 / Honest limits + +- 不检测 `ssrf` 类别：预览内网地址是正常用法，SSRF 仍由 `trust.host` / `not.trust.host` 负责。Guard 会解码 Base64 的 `url` 并检测解码后的内容，但 AES 加密（`encryption=aes`）的 `url` 无法检测。 +- 默认不启用 `sensitive_file` 和 `recon` 类别：它们会把 `.java`、`.py`、`.sql`、`.sh`、`.jsp`、`.asp` 等源码文件的访问路径当作探测，而 kkFileView 会预览这类文件。可以用 `KK_GUARD_DETECTION_CATEGORIES` 覆盖。 +- 使用 `local.preview.dir`（`file://` 预览）时，需要把 `url`、`urlPath`、`urls`、`currentUrl` 加入 `guard.excluded-detection-params`，否则 `file://` 会被当作文件包含攻击。 +- 404 规则按接口统计，不是按路径统计：遍历大量不同静态路径的扫描器只会被请求限制限速，不会被封禁。 +- `/fileUpload` 的 multipart 请求体不做检测（`scanBody = false`）：spring-boot-guard 1.3.0 会检测表单字段和文件部分的内容，而 kkFileView 会保存并预览 html、sql、sh 等源码，这类文件会被当作攻击拒绝。路径、查询参数和请求头仍然检测，`path` 表单字段由 `FileController` 校验。Guard 看不到上传文件部分的内容，`Content-Type` 检查的是请求而不是文件部分，文件类型仍由 `prohibit` 控制。声明的 Content-Length 达到 2GB 或以上时，引擎的大小检查会抛出异常并返回 500（请求仍然被拒绝）。 +- 引擎不会封禁回环地址（127.0.0.1）：来自回环地址的探测会被拒绝，但不会被封禁。 +- 计数和封禁默认保存在进程内存中：多实例之间不共享，重启后清空。需要共享时设置 `KK_GUARD_ENABLE_REDIS=true` 和 `KK_GUARD_REDIS_URL`（独立于 `cache.type=redis`）。 +- 全局限制默认 600 次/分钟：预览页面会加载大量静态资源，限制按客户端地址统计，办公室出口共用一个地址时请调大 `KK_GUARD_RATE_LIMIT`。 +- 响应头和 CORS 保持关闭：kkFileView 常被嵌入 iframe，Guard 的 `X-Frame-Options` 会破坏嵌入。 +- 反向代理之后必须设置 `KK_GUARD_TRUSTED_PROXIES`（代理的 IP/CIDR），否则所有客户端共用代理的地址。只信任来自这些地址的 `X-Forwarded-For`。不要同时设置 `server.forward-headers-strategy=native/framework`，两者二选一；启动日志会说明当前由谁决定客户端地址。 + ## 🚨 安全事件响应 如果发现可疑的预览请求： diff --git a/pom.xml b/pom.xml index ebdcf33d1..2899d076f 100644 --- a/pom.xml +++ b/pom.xml @@ -64,6 +64,11 @@ 1.70 + + 1.3.0 + 2.4.21 + 1.11.0 + 1.4.21 2.7.7 diff --git a/server/pom.xml b/server/pom.xml index 0ab813349..062837090 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -13,6 +13,22 @@ + + + org.jetbrains.kotlin + kotlin-bom + ${kotlin-bom.version} + import + pom + + + org.jetbrains.kotlinx + kotlinx-coroutines-bom + ${kotlinx-coroutines-bom.version} + import + pom + org.springframework.boot spring-boot-dependencies @@ -59,6 +75,13 @@ true + + + com.guard-core + spring-boot-guard + ${spring-boot-guard.version} + + org.jodconverter diff --git a/server/src/main/config/application.properties b/server/src/main/config/application.properties index 9bc1bd9e7..d86dd8bc8 100644 --- a/server/src/main/config/application.properties +++ b/server/src/main/config/application.properties @@ -480,3 +480,89 @@ kk.scriptjs = false # 纯文本文件类型，直接显示 simText = ${KK_SIMTEXT:txt,html,htm,asp,jsp,xml,json,properties,md,gitignore,log,java,py,c,cpp,sql,sh,bat,m,bas,prg,cmd} + + +############################################################################### +# 十五、Guard Core 应用层防护（可选，默认关闭，需要重启生效） +############################################################################### +# 开启后在应用内增加：每个客户端的访问频率限制、对路径/查询参数/请求头的攻击特征检测（目录穿越、注入、XSS、扫描探测） +# 并自动封禁持续探测的客户端、上传请求限制，以及对写操作接口的 IP 白名单/密钥保护。 +# 它不替代 trust.host / not.trust.host（SSRF 防护）和各个安全修复，只限制谁能访问、访问多快，并封禁探测者。 +# 注意：Guard 会解码 Base64 参数（url / urlPath / urls / fileName）并检测解码后的内容，但 AES 加密（encryption=aes）的 url 无法检测。 +# 域名、内网地址的校验仍由 TrustHostFilter 完成。 +# 详见 SECURITY_CONFIG.md + +# 总开关（spring-boot-guard 的 guard.enabled，库的默认值是 true，这里改为默认 false）：false 时不会注册任何 Guard 组件，行为与之前完全一致 +guard.enabled = ${KK_GUARD_ENABLED:false} + +# 仅记录不拦截（观察模式）：先用 true 上线，确认没有误报后再改为 false +guard.passive-mode = ${KK_GUARD_PASSIVE_MODE:false} + +# 反向代理（nginx 等）的 IP/CIDR，多个用逗号隔开。只信任来自这些地址的 X-Forwarded-For，其他来源一律使用 TCP 连接地址。 +# 在反向代理之后部署时必须设置，否则所有客户端会共用代理的地址。 +# 不要同时设置 server.forward-headers-strategy=native/framework，二选一 +guard.trusted-proxies = ${KK_GUARD_TRUSTED_PROXIES:} + +# 默认使用进程内存保存计数和封禁（多实例之间不共享，重启后清空）。 +# 多实例部署需要共享时，设置为 true 并配置 Redis 地址（lettuce 格式，如 redis://:password@127.0.0.1:6379/0） +guard.enable-redis = ${KK_GUARD_ENABLE_REDIS:false} +guard.redis-url = ${KK_GUARD_REDIS_URL:redis://127.0.0.1:6379} + +# 每个客户端的全局请求限制：窗口(秒)内最多请求数。预览页面会加载大量静态资源，所以全局值较大 +guard.enable-rate-limiting = true +guard.rate-limit = ${KK_GUARD_RATE_LIMIT:600} +guard.rate-limit-window = 60 + +# 消耗 CPU 的接口单独限制（转换文档、读取远程文件、入队转换、上传、删除），格式：窗口 60 秒内的最大请求数 +guard.endpoint-rate-limits[/onlinePreview].requests = ${KK_GUARD_RATE_LIMIT_PREVIEW:60} +guard.endpoint-rate-limits[/onlinePreview].window = 60 +guard.endpoint-rate-limits[/picturesPreview].requests = ${KK_GUARD_RATE_LIMIT_PREVIEW:60} +guard.endpoint-rate-limits[/picturesPreview].window = 60 +guard.endpoint-rate-limits[/getCorsFile].requests = ${KK_GUARD_RATE_LIMIT_CORSFILE:120} +guard.endpoint-rate-limits[/getCorsFile].window = 60 +guard.endpoint-rate-limits[/addTask].requests = ${KK_GUARD_RATE_LIMIT_ADDTASK:30} +guard.endpoint-rate-limits[/addTask].window = 60 +guard.endpoint-rate-limits[/fileUpload].requests = ${KK_GUARD_RATE_LIMIT_WRITE:10} +guard.endpoint-rate-limits[/fileUpload].window = 60 +guard.endpoint-rate-limits[/createFolder].requests = ${KK_GUARD_RATE_LIMIT_WRITE:10} +guard.endpoint-rate-limits[/createFolder].window = 60 +guard.endpoint-rate-limits[/deleteFile].requests = ${KK_GUARD_RATE_LIMIT_WRITE:10} +guard.endpoint-rate-limits[/deleteFile].window = 60 + +# 攻击特征检测（路径、查询参数、请求头、请求体）。默认类别不包含： +# ssrf：预览内网地址（url=http://192.168...）是 kkFileView 的正常用法，SSRF 由 trust.host / not.trust.host 负责 +# sensitive_file、recon：会把 .java .py .sql .sh .jsp .asp 等源码文件的访问路径当作探测，而 kkFileView 会预览这类文件 +# （例如压缩包内或 demo 目录中的源码文件），误报会导致这些文件无法预览。大量遍历不同路径的扫描由全局请求限制限速 +# 需要调整时，通过环境变量 KK_GUARD_DETECTION_CATEGORIES 覆盖（逗号分隔） +guard.enable-penetration-detection = true +guard.enabled-detection-categories = ${KK_GUARD_DETECTION_CATEGORIES:xss,sqli,dir_traversal,path_traversal,cmd_injection,file_inclusion,ldap,xml,nosql,file_upload,template,http_split,cms_probing,proto_pollution,code_injection,deserialization} + +# 持续触发检测的客户端自动封禁：累计次数达到阈值后封禁指定秒数 +guard.enable-ip-banning = true +guard.auto-ban-threshold = ${KK_GUARD_AUTO_BAN_THRESHOLD:10} +guard.auto-ban-duration = ${KK_GUARD_AUTO_BAN_DURATION:3600} + +# fileName 是 Base64 编码的 file://localhost/文件名（演示页的删除请求），解码后会被当作 file:// 文件包含，需要排除； +# 文件名本身仍由 FileController 校验。如果使用 local.preview.dir（file:// 预览），还需要把 url、urlPath、urls、currentUrl 加入 guard.excluded-detection-params +guard.excluded-detection-body-fields = fileName + +# 行为规则：同一接口上累计大量 404 的客户端封禁 1 小时（按接口统计，不是按路径统计）。 +# 它能封禁反复读取不存在的远程文件（/getCorsFile）的客户端，但不能封禁遍历大量不同静态路径的扫描器，那类扫描由全局请求限制限制速度 +guard.global-behavior-rules[0].rule-type = return_pattern +guard.global-behavior-rules[0].pattern = status:404 +guard.global-behavior-rules[0].threshold = ${KK_GUARD_BAN_404_THRESHOLD:50} +guard.global-behavior-rules[0].window = 300 +guard.global-behavior-rules[0].action = ban +guard.global-behavior-rules[0].ban-duration = 3600 +# 删除接口按响应内容统计失败次数（删除密码错误），只缓冲该接口的响应 +guard.behavior-scan-response-body = true + +# 响应头和 CORS 保持关闭：kkFileView 常被嵌入 iframe，Guard 的 X-Frame-Options 等响应头会破坏嵌入 +guard.security-headers.enabled = false +guard.enable-cors = false + +# 写操作接口（上传、创建文件夹、删除、addTask）的访问控制，默认都为空（不限制）： +# 允许访问的 IP/CIDR，多个用逗号隔开，例如 127.0.0.1,10.0.0.0/8 +guard.kk.admin-allowed-ips = ${KK_GUARD_ADMIN_ALLOWED_IPS:} +# 访问密钥，调用方在请求头 X-KK-Admin-Key 中携带。同时设置时两个条件都必须满足 +guard.kk.admin-key = ${KK_GUARD_ADMIN_KEY:} diff --git a/server/src/main/java/cn/keking/ServerMain.java b/server/src/main/java/cn/keking/ServerMain.java index 0e5b52988..0fb413014 100644 --- a/server/src/main/java/cn/keking/ServerMain.java +++ b/server/src/main/java/cn/keking/ServerMain.java @@ -18,12 +18,21 @@ public class ServerMain { private static final Logger logger = LoggerFactory.getLogger(ServerMain.class); + /** + * spring-boot-guard is on unless {@code guard.enabled=false}. The shipped application.properties turns it off, but + * {@code -Dspring.config.location} replaces that file with the operator's own copy, so the opt-in default is repeated + * here as the lowest-precedence property: any config file, environment variable or argument still wins. + */ + static SpringApplicationBuilder applicationBuilder() { + return new SpringApplicationBuilder(ServerMain.class) + .logStartupInfo(false) + .properties("guard.enabled=false"); + } + public static void main(String[] args) { StopWatch stopWatch = new StopWatch(); stopWatch.start(); - ConfigurableApplicationContext context = new SpringApplicationBuilder(ServerMain.class) - .logStartupInfo(false) - .run(args); + ConfigurableApplicationContext context = applicationBuilder().run(args); stopWatch.stop(); ServerProperties serverProperties = context.getBean(ServerProperties.class); Integer port = serverProperties.getPort(); diff --git a/server/src/main/java/cn/keking/guard/GuardAdminAccessValidator.java b/server/src/main/java/cn/keking/guard/GuardAdminAccessValidator.java new file mode 100644 index 000000000..d352cf107 --- /dev/null +++ b/server/src/main/java/cn/keking/guard/GuardAdminAccessValidator.java @@ -0,0 +1,144 @@ +package cn.keking.guard; + +import com.guardcore.protocols.GuardRequest; +import com.guardcore.protocols.GuardResponse; +import com.guardcore.spring.CustomRouteValidator; +import kotlin.coroutines.Continuation; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; + +import java.net.InetAddress; +import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; + +/** + * Limits who may reach the state-changing endpoints (upload, folder creation, delete, addTask). + * Both checks are optional and off by default: an IP allowlist ({@code guard.kk.admin-allowed-ips}) + * and an API key ({@code guard.kk.admin-key}, sent in the {@code X-KK-Admin-Key} header). + * When both are set, both must pass. The client IP is the one Guard Core resolved, so it honours + * {@code guard.trusted-proxies} and ignores a spoofed X-Forwarded-For from any other peer. + */ +@Component(GuardAdminAccessValidator.BEAN_NAME) +@ConditionalOnProperty(name = "guard.enabled", havingValue = "true") +public class GuardAdminAccessValidator implements CustomRouteValidator { + + public static final String BEAN_NAME = "kkGuardAdminAccessValidator"; + static final String KEY_HEADER = "x-kk-admin-key"; + private static final Pattern IPV4_LITERAL = Pattern.compile("(\\d{1,3}\\.){3}\\d{1,3}"); + private static final Pattern IPV6_LITERAL = Pattern.compile("[0-9a-fA-F:.]*:[0-9a-fA-F:.]*"); + + private final List allowedNetworks = new ArrayList<>(); + private final byte[] adminKey; + + public GuardAdminAccessValidator(@Value("${guard.kk.admin-allowed-ips:}") List allowedIps, + @Value("${guard.kk.admin-key:}") String adminKey) { + for (String entry : allowedIps) { + if (!entry.isBlank()) { + allowedNetworks.add(Network.parse(entry.trim())); + } + } + this.adminKey = adminKey.getBytes(StandardCharsets.UTF_8); + } + + @Override + public Object validate(GuardRequest request, Continuation continuation) { + if (isIpAllowed(request) && isKeyAccepted(request)) { + return null; + } + return forbidden(); + } + + private boolean isIpAllowed(GuardRequest request) { + if (allowedNetworks.isEmpty()) { + return true; + } + Object clientIp = request.getState().getAttr("client_ip"); + return clientIp instanceof String ip && allowedNetworks.stream().anyMatch(network -> network.contains(ip)); + } + + private boolean isKeyAccepted(GuardRequest request) { + if (adminKey.length == 0) { + return true; + } + String supplied = request.getHeaders().get(KEY_HEADER); + return supplied != null && MessageDigest.isEqual(adminKey, supplied.getBytes(StandardCharsets.UTF_8)); + } + + private static GuardResponse forbidden() { + byte[] body = "Forbidden".getBytes(StandardCharsets.UTF_8); + Map headers = new LinkedHashMap<>(); + headers.put("Content-Type", "text/plain; charset=utf-8"); + headers.put("Content-Length", String.valueOf(body.length)); + return new GuardResponse() { + @Override public int getStatusCode() { return 403; } + @Override public Map getHeaders() { return headers; } + @Override public byte[] getBody() { return body; } + }; + } + + /** An IP or CIDR literal. Typos fail startup instead of silently widening or narrowing access. */ + record Network(byte[] address, int prefixLength) { + + static Network parse(String entry) { + String[] parts = entry.split("/", -1); + byte[] address = parts.length <= 2 ? literal(parts[0]) : null; + if (address == null) { + throw invalid(entry); + } + int prefix = address.length * 8; + if (parts.length == 2) { + try { + prefix = Integer.parseInt(parts[1]); + } catch (NumberFormatException e) { + throw invalid(entry); + } + if (prefix < 0 || prefix > address.length * 8) { + throw invalid(entry); + } + } + return new Network(address, prefix); + } + + boolean contains(String ip) { + byte[] candidate = literal(ip); + if (candidate == null || candidate.length != address.length) { + return false; + } + int fullBytes = prefixLength / 8; + for (int i = 0; i < fullBytes; i++) { + if (candidate[i] != address[i]) { + return false; + } + } + int remainingBits = prefixLength % 8; + if (remainingBits == 0) { + return true; + } + int mask = 0xFF << (8 - remainingBits); + return (candidate[fullBytes] & mask) == (address[fullBytes] & mask); + } + + /** Only literals reach InetAddress, so this never performs a DNS lookup. IPv4-mapped IPv6 becomes IPv4. */ + private static byte[] literal(String value) { + if (!IPV4_LITERAL.matcher(value).matches() && !IPV6_LITERAL.matcher(value).matches()) { + return null; + } + try { + return InetAddress.getByName(value).getAddress(); + } catch (UnknownHostException e) { + return null; + } + } + + private static IllegalArgumentException invalid(String entry) { + return new IllegalArgumentException("guard.kk.admin-allowed-ips: invalid entry '" + entry + "'"); + } + } +} diff --git a/server/src/main/java/cn/keking/guard/GuardClientIdentityReport.java b/server/src/main/java/cn/keking/guard/GuardClientIdentityReport.java new file mode 100644 index 000000000..eaa88bc13 --- /dev/null +++ b/server/src/main/java/cn/keking/guard/GuardClientIdentityReport.java @@ -0,0 +1,48 @@ +package cn.keking.guard; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.boot.cloud.CloudPlatform; +import org.springframework.boot.context.properties.bind.Bindable; +import org.springframework.boot.context.properties.bind.Binder; +import org.springframework.core.env.Environment; +import org.springframework.stereotype.Component; + +import java.util.List; + +/** + * Logs, once at startup, which layer decides the client address, so Tomcat and Guard Core cannot disagree + * about proxy trust unnoticed. Tomcat rewrites the remote address only when server.forward-headers-strategy + * is native or framework (or a cloud platform that implies it); Guard Core trusts X-Forwarded-For only from + * guard.trusted-proxies. Use one of the two, not both. + */ +@Component +@ConditionalOnProperty(name = "guard.enabled", havingValue = "true") +public class GuardClientIdentityReport { + + private static final Logger logger = LoggerFactory.getLogger(GuardClientIdentityReport.class); + + public GuardClientIdentityReport(Environment environment) { + List trustedProxies = Binder.get(environment) + .bind("guard.trusted-proxies", Bindable.listOf(String.class)).orElse(List.of()); + String strategy = environment.getProperty("server.forward-headers-strategy"); + CloudPlatform platform = CloudPlatform.getActive(environment); + boolean tomcatRewrites = strategy != null + ? !"none".equalsIgnoreCase(strategy) + : platform != null && platform.isUsingForwardHeaders(); + if (tomcatRewrites && !trustedProxies.isEmpty()) { + logger.warn("Guard Core: Tomcat already rewrites the client address from forwarded headers " + + "(server.forward-headers-strategy) and guard.trusted-proxies is also set. Use only one of them, " + + "and keep server.tomcat.remoteip.internal-proxies as narrow as guard.trusted-proxies."); + } else if (tomcatRewrites) { + logger.info("Guard Core: client address comes from Tomcat (server.forward-headers-strategy), " + + "guard.trusted-proxies is empty."); + } else if (trustedProxies.isEmpty()) { + logger.info("Guard Core: client address is the TCP peer, X-Forwarded-For is ignored. Behind a reverse proxy " + + "set guard.trusted-proxies, otherwise every client shares the proxy address."); + } else { + logger.info("Guard Core: client address is the TCP peer, X-Forwarded-For is trusted only from {}.", trustedProxies); + } + } +} diff --git a/server/src/main/java/cn/keking/web/controller/FileController.java b/server/src/main/java/cn/keking/web/controller/FileController.java index 57e77e06c..a1057bee6 100644 --- a/server/src/main/java/cn/keking/web/controller/FileController.java +++ b/server/src/main/java/cn/keking/web/controller/FileController.java @@ -1,12 +1,18 @@ package cn.keking.web.controller; import cn.keking.config.ConfigConstants; +import cn.keking.guard.GuardAdminAccessValidator; import cn.keking.model.FileType; import cn.keking.model.ReturnResponse; import cn.keking.utils.CaptchaUtil; import cn.keking.utils.DateUtils; import cn.keking.utils.KkFileUtils; import cn.keking.utils.WebUtils; +import com.guardcore.spring.GuardContentTypeFilter; +import com.guardcore.spring.GuardCustomValidation; +import com.guardcore.spring.GuardDetectionExclusion; +import com.guardcore.spring.GuardMaxRequestSize; +import com.guardcore.spring.GuardReturnMonitor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.util.ObjectUtils; @@ -153,6 +159,12 @@ public String getStats() { } } + // Same ceiling as spring.servlet.multipart.max-request-size, checked on Content-Length before the body is parsed. + @GuardContentTypeFilter(allowedTypes = "multipart/form-data") + @GuardMaxRequestSize(sizeBytes = 524288000) + // Path, query and headers are still scanned. The multipart body is not: spring-boot-guard 1.3.0 scans file part payloads along with form fields, and kkFileView stores and previews html, sql and shell sources that look like attacks. + @GuardDetectionExclusion(scanBody = false) + @GuardCustomValidation(validatorBean = GuardAdminAccessValidator.BEAN_NAME) @PostMapping("/fileUpload") public ReturnResponse fileUpload(@RequestParam("file") MultipartFile file, @RequestParam(value = "path", defaultValue = "") String path) { @@ -180,6 +192,7 @@ public ReturnResponse fileUpload(@RequestParam("file") MultipartFile fil } } + @GuardCustomValidation(validatorBean = GuardAdminAccessValidator.BEAN_NAME) @PostMapping("/createFolder") public ReturnResponse createFolder(@RequestParam(value = "path", defaultValue = "") String path, @RequestParam("folderName") String folderName) { @@ -237,6 +250,9 @@ private Path createUploadDirectory(String requestedPath) throws IOException { return current; } + @GuardCustomValidation(validatorBean = GuardAdminAccessValidator.BEAN_NAME) + // Every failed delete (wrong password, unknown file) answers {"code":1,...}: ban a client that keeps failing, i.e. guessing the password. + @GuardReturnMonitor(pattern = "\"code\":1", maxOccurrences = 5, window = 3600, action = "ban") @PostMapping("/deleteFile") public ReturnResponse deleteFile(HttpServletRequest request, String fileName, String password) { ReturnResponse checkResult = this.deleteFileCheck(request, fileName, password); diff --git a/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java b/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java index 3d5e4d155..9120f79e7 100644 --- a/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java +++ b/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java @@ -1,6 +1,7 @@ package cn.keking.web.controller; import cn.keking.config.ConfigConstants; +import cn.keking.guard.GuardAdminAccessValidator; import cn.keking.model.FileAttribute; import cn.keking.service.FileHandlerService; import cn.keking.service.FilePreview; @@ -8,6 +9,7 @@ import cn.keking.service.cache.CacheService; import cn.keking.service.impl.OtherFilePreviewImpl; import cn.keking.utils.*; +import com.guardcore.spring.GuardCustomValidation; import fr.opensagres.xdocreport.core.io.IOUtils; import org.apache.commons.codec.binary.Base64; import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; @@ -268,6 +270,7 @@ public void getCorsFile(@RequestParam String urlPath, * * @param url 请编码后在入队 */ + @GuardCustomValidation(validatorBean = GuardAdminAccessValidator.BEAN_NAME) @GetMapping("/addTask") @ResponseBody public String addQueueTask(@RequestParam String url, diff --git a/server/src/test/java/cn/keking/ServerMainTestAccess.java b/server/src/test/java/cn/keking/ServerMainTestAccess.java new file mode 100644 index 000000000..da1a38dfc --- /dev/null +++ b/server/src/test/java/cn/keking/ServerMainTestAccess.java @@ -0,0 +1,14 @@ +package cn.keking; + +import org.springframework.boot.builder.SpringApplicationBuilder; + +/** Lets tests outside the cn.keking package build the application exactly as main() does. */ +public final class ServerMainTestAccess { + + private ServerMainTestAccess() { + } + + public static SpringApplicationBuilder applicationBuilder() { + return ServerMain.applicationBuilder(); + } +} diff --git a/server/src/test/java/cn/keking/config/ActuatorOverrideExposureSecurityTests.java b/server/src/test/java/cn/keking/config/ActuatorOverrideExposureSecurityTests.java index 75155ab40..515249b5f 100644 --- a/server/src/test/java/cn/keking/config/ActuatorOverrideExposureSecurityTests.java +++ b/server/src/test/java/cn/keking/config/ActuatorOverrideExposureSecurityTests.java @@ -3,7 +3,8 @@ import org.springframework.boot.test.context.SpringBootTest; @SpringBootTest(classes = ActuatorExposureSecurityTests.ManagementApp.class, - properties = "spring.config.name=test", + // guard.enabled: without application.properties spring-boot-guard turns itself on, and this test is about actuator only + properties = {"spring.config.name=test", "guard.enabled=false"}, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) class ActuatorOverrideExposureSecurityTests extends ActuatorExposureSecurityTests { } diff --git a/server/src/test/java/cn/keking/guard/GuardAdminAccessTests.java b/server/src/test/java/cn/keking/guard/GuardAdminAccessTests.java new file mode 100644 index 000000000..bc67675b7 --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardAdminAccessTests.java @@ -0,0 +1,68 @@ +package cn.keking.guard; + +import org.junit.jupiter.api.Test; +import org.springframework.test.context.TestPropertySource; + +import java.net.http.HttpResponse; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +/** Access control on the state-changing endpoints: an IP allowlist and an API key, both required when both are set. */ +@TestPropertySource(properties = { + "guard.enabled=true", + "guard.trusted-proxies=127.0.0.1", + "guard.rate-limit=1000", + "guard.kk.admin-allowed-ips=203.0.113.0/24,2001:db8::/32", + "guard.kk.admin-key=s3cret" +}) +class GuardAdminAccessTests extends GuardHttpTestSupport { + + private static final String ALLOWED_IP = "203.0.113.9"; + private static final String KEY_HEADER = "X-KK-Admin-Key"; + + private static void assertRefused(HttpResponse response) { + assertEquals(403, response.statusCode()); + assertEquals("Forbidden", response.body()); + } + + private static void assertReachedTheController(HttpResponse response) { + assertNotEquals("Forbidden", response.body()); + } + + @Test + void deleteIsRefusedFromAnAddressOutsideTheAllowlist() throws Exception { + assertRefused(postForm("/deleteFile", newClientIp(), "fileName=x&password=y", KEY_HEADER, "s3cret")); + } + + @Test + void deleteIsRefusedWithoutTheKeyOrWithAWrongKey() throws Exception { + assertRefused(postForm("/deleteFile", ALLOWED_IP, "fileName=x&password=y")); + assertRefused(postForm("/deleteFile", ALLOWED_IP, "fileName=x&password=y", KEY_HEADER, "wrong")); + } + + @Test + void deleteReachesTheControllerFromTheAllowlistWithTheKey() throws Exception { + assertReachedTheController(postForm("/deleteFile", ALLOWED_IP, "fileName=x&password=y", KEY_HEADER, "s3cret")); + assertReachedTheController(postForm("/deleteFile", "2001:db8::17", "fileName=x&password=y", KEY_HEADER, "s3cret")); + } + + @Test + void createFolderUploadAndAddTaskAreProtectedToo() throws Exception { + String outsider = newClientIp(); + assertRefused(postForm("/createFolder", outsider, "folderName=a")); + assertRefused(postMultipart("/fileUpload", outsider)); + assertRefused(get("/addTask?url=" + base64("http://files.example/a.pdf"), outsider)); + + assertReachedTheController(postForm("/createFolder", ALLOWED_IP, "folderName=a", KEY_HEADER, "s3cret")); + assertReachedTheController(postMultipart("/fileUpload", ALLOWED_IP, KEY_HEADER, "s3cret")); + } + + @Test + void previewAndListingStayOpen() throws Exception { + String outsider = newClientIp(); + assertNotEquals("Forbidden", get("/onlinePreview?url=" + base64("http://files.example/a.pdf"), outsider).body()); + assertEquals(200, postForm("/listFiles", outsider, "path=&page=0&size=5").statusCode()); + assertEquals(200, status("/index", outsider)); + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardAdminAccessValidatorTests.java b/server/src/test/java/cn/keking/guard/GuardAdminAccessValidatorTests.java new file mode 100644 index 000000000..aa8a92717 --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardAdminAccessValidatorTests.java @@ -0,0 +1,94 @@ +package cn.keking.guard; + +import com.guardcore.protocols.GuardRequest; +import com.guardcore.protocols.GuardResponse; +import com.guardcore.protocols.RequestState; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class GuardAdminAccessValidatorTests { + + private static GuardRequest request(String clientIp, Map headers) { + GuardRequest request = mock(GuardRequest.class); + RequestState state = mock(RequestState.class); + when(state.getAttr("client_ip")).thenReturn(clientIp); + when(request.getState()).thenReturn(state); + when(request.getHeaders()).thenReturn(headers); + return request; + } + + private static Object validate(GuardAdminAccessValidator validator, String clientIp, Map headers) { + return validator.validate(request(clientIp, headers), null); + } + + private static void assertForbidden(Object outcome) { + GuardResponse response = assertInstanceOf(GuardResponse.class, outcome); + assertEquals(403, response.getStatusCode()); + } + + @Test + void allowsEveryoneWhenNothingIsConfigured() { + GuardAdminAccessValidator validator = new GuardAdminAccessValidator(List.of(), ""); + assertNull(validate(validator, "198.51.100.1", Map.of())); + } + + @Test + void allowlistAcceptsAddressesAndCidrRangesOnly() { + GuardAdminAccessValidator validator = new GuardAdminAccessValidator(List.of("10.0.0.0/8", "192.0.2.7", "2001:db8::/32"), ""); + assertNull(validate(validator, "10.200.3.4", Map.of())); + assertNull(validate(validator, "192.0.2.7", Map.of())); + assertNull(validate(validator, "2001:db8:1::5", Map.of())); + assertForbidden(validate(validator, "11.0.0.1", Map.of())); + assertForbidden(validate(validator, "192.0.2.8", Map.of())); + assertForbidden(validate(validator, "2001:db9::1", Map.of())); + assertForbidden(validate(validator, null, Map.of())); + assertForbidden(validate(validator, "not-an-ip", Map.of())); + } + + @Test + void prefixesThatAreNotAMultipleOfEightBitsAreHonoured() { + GuardAdminAccessValidator validator = new GuardAdminAccessValidator(List.of("172.16.0.0/12"), ""); + assertNull(validate(validator, "172.31.255.255", Map.of())); + assertForbidden(validate(validator, "172.32.0.1", Map.of())); + } + + @Test + void anIpv4MappedIpv6PeerMatchesAnIpv4Entry() { + GuardAdminAccessValidator validator = new GuardAdminAccessValidator(List.of("192.0.2.0/24"), ""); + assertNull(validate(validator, "::ffff:192.0.2.9", Map.of())); + } + + @Test + void keyMustMatchExactly() { + GuardAdminAccessValidator validator = new GuardAdminAccessValidator(List.of(), "s3cret"); + assertNull(validate(validator, "198.51.100.1", Map.of("x-kk-admin-key", "s3cret"))); + assertForbidden(validate(validator, "198.51.100.1", Map.of())); + assertForbidden(validate(validator, "198.51.100.1", Map.of("x-kk-admin-key", "s3cret "))); + assertForbidden(validate(validator, "198.51.100.1", Map.of("x-kk-admin-key", "S3CRET"))); + } + + @Test + void bothMustPassWhenBothAreConfigured() { + GuardAdminAccessValidator validator = new GuardAdminAccessValidator(List.of("192.0.2.0/24"), "s3cret"); + assertNull(validate(validator, "192.0.2.9", Map.of("x-kk-admin-key", "s3cret"))); + assertForbidden(validate(validator, "192.0.2.9", Map.of())); + assertForbidden(validate(validator, "198.51.100.1", Map.of("x-kk-admin-key", "s3cret"))); + } + + @ParameterizedTest + @ValueSource(strings = {"10.0.0", "10.0.0.256", "10.0.0.0/33", "10.0.0.0/-1", "10.0.0.0/8/8", "example.com", "::1/129", "10.0.0.0/x"}) + void aMistypedEntryFailsStartup(String entry) { + assertThrows(IllegalArgumentException.class, () -> new GuardAdminAccessValidator(List.of(entry), "")); + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardClientIdentityReportTests.java b/server/src/test/java/cn/keking/guard/GuardClientIdentityReportTests.java new file mode 100644 index 000000000..5d720a853 --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardClientIdentityReportTests.java @@ -0,0 +1,81 @@ +package cn.keking.guard; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; +import org.springframework.mock.env.MockEnvironment; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class GuardClientIdentityReportTests { + + private final Logger logger = (Logger) LoggerFactory.getLogger(GuardClientIdentityReport.class); + private final ListAppender logs = new ListAppender<>(); + + @BeforeEach + void captureLogs() { + logs.start(); + logger.addAppender(logs); + } + + @AfterEach + void releaseLogs() { + logger.detachAppender(logs); + } + + private ILoggingEvent report(MockEnvironment environment) { + new GuardClientIdentityReport(environment); + assertEquals(1, logs.list.size()); + return logs.list.get(0); + } + + @Test + void warnsWhenTomcatAndGuardBothRewriteTheClientAddress() { + ILoggingEvent event = report(new MockEnvironment() + .withProperty("server.forward-headers-strategy", "native") + .withProperty("guard.trusted-proxies[0]", "10.0.0.0/8")); + assertEquals(Level.WARN, event.getLevel()); + assertTrue(event.getFormattedMessage().contains("Use only one of them")); + } + + @Test + void saysTheTcpPeerIsUsedWhenNothingIsTrusted() { + ILoggingEvent event = report(new MockEnvironment()); + assertEquals(Level.INFO, event.getLevel()); + assertTrue(event.getFormattedMessage().contains("TCP peer, X-Forwarded-For is ignored")); + } + + @Test + void namesTheTrustedProxies() { + ILoggingEvent event = report(new MockEnvironment().withProperty("guard.trusted-proxies", "10.0.0.7")); + assertEquals(Level.INFO, event.getLevel()); + assertTrue(event.getFormattedMessage().contains("[10.0.0.7]")); + } + + @Test + void saysTomcatDecidesWhenOnlyTomcatRewrites() { + ILoggingEvent event = report(new MockEnvironment().withProperty("server.forward-headers-strategy", "framework")); + assertEquals(Level.INFO, event.getLevel()); + assertTrue(event.getFormattedMessage().contains("comes from Tomcat")); + } + + @Test + void anExplicitNoneKeepsTomcatOutOfIt() { + ILoggingEvent event = report(new MockEnvironment().withProperty("server.forward-headers-strategy", "none")); + assertTrue(event.getFormattedMessage().contains("TCP peer")); + } + + @Test + void aCloudPlatformThatImpliesForwardedHeadersCountsAsTomcatRewriting() { + ILoggingEvent event = report(new MockEnvironment() + .withProperty("VCAP_APPLICATION", "{}") + .withProperty("guard.trusted-proxies", "10.0.0.7")); + assertEquals(Level.WARN, event.getLevel()); + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardDisabledTests.java b/server/src/test/java/cn/keking/guard/GuardDisabledTests.java new file mode 100644 index 000000000..94bdfab8f --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardDisabledTests.java @@ -0,0 +1,58 @@ +package cn.keking.guard; + +import com.guardcore.spring.GuardFilter; +import com.guardcore.spring.GuardMiddleware; +import com.guardcore.spring.GuardProperties; +import com.guardcore.spring.GuardSecurityAutoConfiguration; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.ApplicationContext; +import org.springframework.core.env.Environment; + +import java.net.http.HttpResponse; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** The shipped defaults: Guard Core is on the classpath but nothing of it is registered and behaviour is unchanged. */ +class GuardDisabledTests extends GuardHttpTestSupport { + + @Autowired + ApplicationContext context; + + @Autowired + Environment environment; + + @Test + void shippedConfigurationSwitchesTheLibraryOff() { + // spring-boot-guard defaults guard.enabled to true, so application.properties is what keeps the guard off. + assertEquals("false", environment.getProperty("guard.enabled")); + assertEquals(0, context.getBeanNamesForType(GuardSecurityAutoConfiguration.class).length); + assertEquals(0, context.getBeanNamesForType(GuardProperties.class).length); + } + + @Test + void registersNoGuardComponent() { + assertEquals(0, context.getBeanNamesForType(GuardMiddleware.class).length); + assertEquals(0, context.getBeanNamesForType(GuardFilter.class).length); + assertTrue(context.getBeansOfType(FilterRegistrationBean.class).values().stream() + .noneMatch(registration -> registration.getFilter() instanceof GuardFilter)); + assertEquals(0, context.getBeanNamesForType(GuardAdminAccessValidator.class).length); + assertFalse(context.containsBean("guardJsonConverterOrdering")); + } + + @Test + void leavesRequestsUntouched() throws Exception { + String client = newClientIp(); + for (int i = 0; i < 700; i++) { + assertEquals(200, status("/index", client), "request " + i + " was throttled"); + } + HttpResponse attack = get("/index?q=%3Cscript%3Ealert(1)%3C/script%3E", client); + assertEquals(200, attack.statusCode()); + assertNotEquals("Suspicious activity detected", attack.body()); + assertTrue(attack.headers().firstValue("X-Content-Type-Options").isEmpty()); + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardEnabledTests.java b/server/src/test/java/cn/keking/guard/GuardEnabledTests.java new file mode 100644 index 000000000..7bbe093d1 --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardEnabledTests.java @@ -0,0 +1,323 @@ +package cn.keking.guard; + +import cn.keking.config.ConfigConstants; +import com.sun.net.httpserver.HttpServer; +import jakarta.servlet.Filter; +import com.guardcore.spring.GuardMiddleware; +import com.guardcore.spring.GuardProperties; +import jakarta.servlet.http.HttpServletRequest; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.core.Ordered; +import org.springframework.test.context.TestPropertySource; +import org.springframework.web.util.ContentCachingResponseWrapper; + +import java.io.InputStream; +import java.io.OutputStream; +import java.net.InetSocketAddress; +import java.net.Socket; +import java.net.URLEncoder; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** Guard Core switched on with the shipped configuration, plus small budgets so the limits can be reached in a test. */ +@TestPropertySource(properties = { + "guard.enabled=true", + "guard.trusted-proxies=127.0.0.1", + "guard.rate-limit=8", + "guard.endpoint-rate-limits[/picturesPreview].requests=3", + "guard.auto-ban-threshold=3", + // a list in a test property source replaces the whole configured list, so the rule is repeated in full + "guard.global-behavior-rules[0].rule-type=return_pattern", + "guard.global-behavior-rules[0].pattern=status:404", + "guard.global-behavior-rules[0].threshold=3", + "guard.global-behavior-rules[0].window=300", + "guard.global-behavior-rules[0].action=ban", + "guard.global-behavior-rules[0].ban-duration=3600" +}) +class GuardEnabledTests extends GuardHttpTestSupport { + + private static final int CHUNK_BYTES = 1024 * 1024; + private static final Map RESPONSE_TYPES = new ConcurrentHashMap<>(); + + @Autowired + ApplicationContext context; + + @TempDir + static Path fileDir; + private static String previousFileDir; + private static HttpServer upstream; + private static final CountDownLatch RELEASE_REST_OF_FILE = new CountDownLatch(1); + + /** Records which response object the application receives, so a caching wrapper would be visible. */ + @TestConfiguration + static class ResponseTypeProbe { + @Bean + FilterRegistrationBean responseTypeProbeFilter() { + FilterRegistrationBean registration = new FilterRegistrationBean<>((request, response, chain) -> { + RESPONSE_TYPES.put(((HttpServletRequest) request).getRequestURI(), + response instanceof ContentCachingResponseWrapper ? "buffered" : "streamed"); + chain.doFilter(request, response); + }); + registration.setOrder(Ordered.LOWEST_PRECEDENCE); + return registration; + } + } + + @BeforeEach + void previewAnyHostIntoTheTemporaryDirectory() { + // The application re-reads its configuration file while starting, so this is set after the context is up. + ConfigConstants.setTrustHostValue("*"); + ConfigConstants.setFileDirValue(fileDir.toString()); + } + + @BeforeAll + static void startUpstream() throws Exception { + previousFileDir = ConfigConstants.getFileDir(); + upstream = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + upstream.createContext("/hello.png", exchange -> { + byte[] body = "hello from the intranet".getBytes(StandardCharsets.UTF_8); + exchange.getResponseHeaders().add("Content-Type", "image/png"); + exchange.sendResponseHeaders(200, body.length); + try (OutputStream out = exchange.getResponseBody()) { + out.write(body); + } + }); + upstream.createContext("/missing", exchange -> { + exchange.sendResponseHeaders(404, -1); + exchange.close(); + }); + upstream.createContext("/big.pdf", exchange -> { + exchange.getResponseHeaders().add("Content-Type", "application/pdf"); + exchange.sendResponseHeaders(200, 0); + try (OutputStream out = exchange.getResponseBody()) { + byte[] chunk = new byte[CHUNK_BYTES]; + out.write(chunk); + out.flush(); + try { + RELEASE_REST_OF_FILE.await(60, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + for (int i = 0; i < 7; i++) { + out.write(chunk); + } + } + }); + upstream.start(); + } + + @AfterAll + static void stopUpstreamAndRestoreConfiguration() { + RELEASE_REST_OF_FILE.countDown(); + upstream.stop(0); + ConfigConstants.setTrustHostValue("default"); + ConfigConstants.setFileDirValue(previousFileDir); + } + + private static String upstreamUrl(String path) { + return "http://127.0.0.1:" + upstream.getAddress().getPort() + path; + } + + @Test + void globalBudgetGivesTooManyRequests() throws Exception { + String client = newClientIp(); + for (int i = 0; i < 8; i++) { + assertEquals(200, status("/index", client), "request " + i); + } + assertEquals(429, status("/index", client)); + assertEquals(200, status("/index", newClientIp()), "another client keeps its own budget"); + } + + @Test + void expensiveEndpointHasItsOwnSmallerBudget() throws Exception { + String client = newClientIp(); + for (int i = 0; i < 3; i++) { + assertNotEquals(429, status("/picturesPreview", client), "request " + i); + } + assertEquals(429, status("/picturesPreview", client)); + assertEquals(200, status("/index", client), "the other routes are still within the global budget"); + } + + @Test + void traversalAndXssInTheQueryAreRejected() throws Exception { + assertEquals(400, status("/index?file=../../../../etc/passwd", newClientIp())); + assertEquals(400, status("/index?q=%3Cscript%3Ealert(1)%3C/script%3E", newClientIp())); + assertEquals(400, status("/index?x=1%27%20OR%201%3D1--", newClientIp())); + } + + @Test + void traversalInAFormBodyFieldIsRejected() throws Exception { + assertEquals(400, postForm("/listFiles", newClientIp(), "path=../../../../etc&page=0&size=5").statusCode()); + assertEquals(200, postForm("/listFiles", newClientIp(), "path=&page=0&size=5").statusCode()); + } + + @Test + void scannerPathIsRejected() throws Exception { + assertEquals(400, status("/wp-login.php", newClientIp())); + } + + @Test + void clientThatKeepsProbingIsBanned() throws Exception { + String attacker = newClientIp(); + assertEquals(400, status("/index?file=../../../../etc/passwd", attacker)); + for (int i = 0; i < 2; i++) { + status("/index?file=../../../../etc/passwd", attacker); + } + assertEquals(403, status("/index", attacker), "banned after repeated attacks"); + assertEquals(200, status("/index", newClientIp()), "other clients are not affected"); + } + + @Test + void clientThatKeepsHittingTheSameMissingPathIsBanned() throws Exception { + String client = newClientIp(); + for (int i = 0; i < 4; i++) { + assertEquals(404, status("/no-such-file", client)); + } + assertEquals(403, status("/index", client), "banned after repeated 404 responses on one endpoint"); + } + + @Test + void clientThatKeepsRequestingMissingRemoteFilesIsBanned() throws Exception { + String client = newClientIp(); + for (int i = 0; i < 4; i++) { + assertEquals(404, status("/getCorsFile?urlPath=" + base64(upstreamUrl("/missing/" + i + ".pdf")), client)); + } + assertEquals(403, status("/index", client), "the handler is one endpoint, whatever the remote file"); + } + + @Test + void notFoundResponsesAreCountedPerEndpointNotPerPath() throws Exception { + String scanner = newClientIp(); + for (int i = 0; i < 6; i++) { + assertEquals(404, status("/no-such-file-" + i, scanner)); + } + assertEquals(200, status("/index", scanner), "a scan over distinct paths is only slowed down by the budgets, not banned"); + } + + @Test + void clientThatKeepsFailingToDeleteIsBanned() throws Exception { + String client = newClientIp(); + String body = "fileName=" + base64("file://localhost/x.txt") + "&password=guess"; + for (int i = 0; i < 6; i++) { + HttpResponse response = postForm("/deleteFile", client, body); + assertEquals(200, response.statusCode(), "attempt " + i + " is screened and reaches the controller"); + assertTrue(response.body().contains("\"code\":1")); + } + assertEquals(403, postForm("/deleteFile", client, body).statusCode(), "banned after repeated failures"); + } + + @Test + void intranetPreviewUrlIsNotBlocked() throws Exception { + String client = newClientIp(); + String source = upstreamUrl("/hello.png"); + HttpResponse preview = send(request("/onlinePreview?url=" + URLEncoder.encode(base64(source), StandardCharsets.UTF_8)) + .header("X-Forwarded-For", client) + .header("Referer", "http://192.168.1.10/portal/documents") + .GET().build()); + assertEquals(200, preview.statusCode()); + assertTrue(preview.body().contains(source), "the preview page was rendered for the intranet address"); + + HttpResponse proxied = send(request("/getCorsFile?urlPath=" + URLEncoder.encode(base64(source), StandardCharsets.UTF_8)) + .header("X-Forwarded-For", client) + .header("Referer", "http://192.168.1.10/portal/documents") + .GET().build()); + assertEquals(200, proxied.statusCode()); + assertEquals("hello from the intranet", proxied.body()); + } + + @Test + void ordinaryPreviewRequestsAreNotFlagged() throws Exception { + // Untrusted hosts are refused by the application right after the guard, so nothing is downloaded here. + ConfigConstants.setTrustHostValue("default"); + String client = newClientIp(); + String[] sources = { + "https://oss-cn-hangzhou.aliyuncs.com/bucket/%E5%90%88%E5%90%8C%20(%E7%BB%88%E7%89%88).docx?Expires=1760000000&OSSAccessKeyId=LTAI5tAbCdEf&Signature=aB%2Bc%2FdE%3D", + "https://files.example.com/O%27Brien%20notes.xlsx?token=eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxIn0.c2lnbmF0dXJl", + "ftp://user:p%40ss@10.0.0.5:21/share/report.pdf", + "http://192.168.1.10:8080/files/a--b%20select%20union.pdf", + "http://127.0.0.1:9000/bucket/a&b+c,d.pptx;jsessionid=0123456789abcdef", + "https://h.example/a.pdf?x=1&y=%E4%B8%AD%E6%96%87#page=3" + }; + for (String source : sources) { + String url = URLEncoder.encode(base64(source), StandardCharsets.UTF_8); + String optional = "&fullfilename=O%27Brien%20%E5%90%88%E5%90%8C.docx&watermarkTxt=O%27Brien%20%26%20Sons%20%3Cinternal%3E&page=2&kkagent=false"; + assertNotEquals(400, status("/onlinePreview?url=" + url + optional, client), source); + } + } + + @Test + void enablingThePropertyRegistersTheGuard() { + assertEquals(1, context.getBeanNamesForType(GuardMiddleware.class).length); + assertEquals(1, context.getBeanNamesForType(GuardProperties.class).length); + assertEquals(1, context.getBeanNamesForType(GuardAdminAccessValidator.class).length); + } + + @Test + void uploadWithTheWrongContentTypeIsRejected() throws Exception { + assertEquals(415, postForm("/fileUpload", newClientIp(), "a=b").statusCode()); + } + + @Test + void uploadAboveTheSizeLimitIsRejectedBeforeTheBodyIsRead() throws Exception { + String request = "POST /fileUpload HTTP/1.1\r\nHost: 127.0.0.1\r\nX-Forwarded-For: " + newClientIp() + "\r\n" + + "Content-Type: multipart/form-data; boundary=kkguard\r\nContent-Length: 600000000\r\nConnection: close\r\n\r\n"; + try (Socket socket = new Socket("127.0.0.1", port)) { + socket.getOutputStream().write(request.getBytes(StandardCharsets.UTF_8)); + socket.getOutputStream().flush(); + String statusLine = new String(socket.getInputStream().readNBytes(12), StandardCharsets.UTF_8); + assertEquals("HTTP/1.1 413", statusLine); + } + } + + @Test + void wellFormedUploadReachesTheController() throws Exception { + HttpResponse response = postMultipart("/fileUpload", newClientIp()); + assertEquals(200, response.statusCode()); + assertTrue(response.body().contains("\"code\":1"), "the controller answered, upload is disabled by default"); + } + + @Test + void largeCorsFileDownloadStreamsWhileTheHandlerIsStillRunning() throws Exception { + String path = "/getCorsFile?urlPath=" + base64(upstreamUrl("/big.pdf")); + HttpResponse response = client.send( + request(path).header("X-Forwarded-For", newClientIp()).GET().build(), HttpResponse.BodyHandlers.ofInputStream()); + assertEquals(200, response.statusCode()); + assertEquals("streamed", RESPONSE_TYPES.get("/getCorsFile")); + try (InputStream body = response.body()) { + // Tomcat keeps the last partial 8 KB block of what it was given until more arrives, so ask for a bit less than the whole chunk. + byte[] first = body.readNBytes(CHUNK_BYTES - 64 * 1024); + assertEquals(CHUNK_BYTES - 64 * 1024, first.length); + assertEquals(1, RELEASE_REST_OF_FILE.getCount(), "the first chunk arrived before the upstream finished"); + RELEASE_REST_OF_FILE.countDown(); + assertEquals(8L * CHUNK_BYTES - first.length, body.readAllBytes().length); + } + } + + @Test + void guardResponsesAndSecurityHeadersStayOff() throws Exception { + HttpResponse response = get("/index", newClientIp()); + assertTrue(response.headers().firstValue("X-Content-Type-Options").isEmpty()); + assertTrue(response.headers().firstValue("X-Frame-Options").isEmpty()); + assertTrue(response.headers().firstValue("Strict-Transport-Security").isEmpty()); + assertTrue(response.headers().firstValue("Access-Control-Allow-Origin").isEmpty()); + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardHttpTestSupport.java b/server/src/test/java/cn/keking/guard/GuardHttpTestSupport.java new file mode 100644 index 000000000..d0e2bc048 --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardHttpTestSupport.java @@ -0,0 +1,81 @@ +package cn.keking.guard; + +import cn.keking.ServerMain; +import cn.keking.service.OfficePluginManager; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.bean.override.mockito.MockitoBean; + +import java.io.IOException; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * Boots the real application on a random port. The Office manager is replaced by a mock so the tests do not start LibreOffice. + * Every test uses its own client address (sent as X-Forwarded-For) so budgets and bans never leak between tests. + */ +@SpringBootTest(classes = ServerMain.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +abstract class GuardHttpTestSupport { + + private static final AtomicInteger NEXT_CLIENT = new AtomicInteger(1); + + @MockitoBean + OfficePluginManager officePluginManager; + + @Value("${local.server.port}") + int port; + + final HttpClient client = HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1).build(); + + static String newClientIp() { + return "198.51.100." + NEXT_CLIENT.getAndIncrement(); + } + + static String base64(String value) { + return Base64.getEncoder().encodeToString(value.getBytes(StandardCharsets.UTF_8)); + } + + HttpRequest.Builder request(String pathAndQuery) { + return HttpRequest.newBuilder(URI.create("http://127.0.0.1:" + port + pathAndQuery)); + } + + HttpResponse get(String pathAndQuery, String clientIp) throws IOException, InterruptedException { + return send(request(pathAndQuery).header("X-Forwarded-For", clientIp).GET().build()); + } + + int status(String pathAndQuery, String clientIp) throws IOException, InterruptedException { + return get(pathAndQuery, clientIp).statusCode(); + } + + HttpResponse send(HttpRequest request) throws IOException, InterruptedException { + return client.send(request, HttpResponse.BodyHandlers.ofString()); + } + + HttpResponse postForm(String path, String clientIp, String body, String... extraHeaders) throws IOException, InterruptedException { + HttpRequest.Builder builder = request(path).header("X-Forwarded-For", clientIp) + .header("Content-Type", "application/x-www-form-urlencoded") + .POST(HttpRequest.BodyPublishers.ofString(body)); + for (int i = 0; i < extraHeaders.length; i += 2) { + builder.header(extraHeaders[i], extraHeaders[i + 1]); + } + return send(builder.build()); + } + + HttpResponse postMultipart(String path, String clientIp, String... extraHeaders) throws IOException, InterruptedException { + String boundary = "kkguard"; + String body = "--" + boundary + "\r\nContent-Disposition: form-data; name=\"file\"; filename=\"a.txt\"\r\n" + + "Content-Type: text/plain\r\n\r\nhello\r\n--" + boundary + "--\r\n"; + HttpRequest.Builder builder = request(path).header("X-Forwarded-For", clientIp) + .header("Content-Type", "multipart/form-data; boundary=" + boundary) + .POST(HttpRequest.BodyPublishers.ofString(body)); + for (int i = 0; i < extraHeaders.length; i += 2) { + builder.header(extraHeaders[i], extraHeaders[i + 1]); + } + return send(builder.build()); + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardOffWithoutConfigFileTests.java b/server/src/test/java/cn/keking/guard/GuardOffWithoutConfigFileTests.java new file mode 100644 index 000000000..43d02a06d --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardOffWithoutConfigFileTests.java @@ -0,0 +1,42 @@ +package cn.keking.guard; + +import cn.keking.ServerMainTestAccess; +import com.guardcore.spring.GuardMiddleware; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.springframework.context.ConfigurableApplicationContext; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Operators start kkFileView with -Dspring.config.location pointing at their own copy of application.properties, + * which replaces the shipped one. A copy from before the guard existed has no guard.enabled, and spring-boot-guard + * turns itself on when the property is missing, so the opt-in default has to come from the application itself. + */ +class GuardOffWithoutConfigFileTests { + + @TempDir + Path directory; + + @Test + void guardStaysOffWhenTheOperatorsConfigFileDoesNotMentionIt() throws Exception { + Path config = Files.writeString(directory.resolve("application.properties"), "server.port=0\n"); + try (ConfigurableApplicationContext context = ServerMainTestAccess.applicationBuilder() + .run("--spring.config.location=" + config, "--spring.main.lazy-initialization=true")) { + assertEquals("false", context.getEnvironment().getProperty("guard.enabled")); + assertEquals(0, context.getBeanNamesForType(GuardMiddleware.class).length); + } + } + + @Test + void guardCanStillBeSwitchedOnFromTheConfigFile() throws Exception { + Path config = Files.writeString(directory.resolve("application.properties"), "server.port=0\nguard.enabled=true\nguard.enable-redis=false\n"); + try (ConfigurableApplicationContext context = ServerMainTestAccess.applicationBuilder() + .run("--spring.config.location=" + config, "--spring.main.lazy-initialization=true")) { + assertEquals(1, context.getBeanNamesForType(GuardMiddleware.class).length); + } + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardPassiveModeTests.java b/server/src/test/java/cn/keking/guard/GuardPassiveModeTests.java new file mode 100644 index 000000000..51b8fe58a --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardPassiveModeTests.java @@ -0,0 +1,25 @@ +package cn.keking.guard; + +import org.junit.jupiter.api.Test; +import org.springframework.test.context.TestPropertySource; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** The observation mode used to roll the guard out: violations are only logged, nothing is blocked. */ +@TestPropertySource(properties = { + "guard.enabled=true", + "guard.passive-mode=true", + "guard.trusted-proxies=127.0.0.1", + "guard.rate-limit=3" +}) +class GuardPassiveModeTests extends GuardHttpTestSupport { + + @Test + void attacksAndOverBudgetRequestsAreLoggedNotBlocked() throws Exception { + String client = newClientIp(); + assertEquals(200, status("/index?file=../../../../etc/passwd", client)); + for (int i = 0; i < 6; i++) { + assertEquals(200, status("/index", client), "request " + i); + } + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardUntrustedProxyTests.java b/server/src/test/java/cn/keking/guard/GuardUntrustedProxyTests.java new file mode 100644 index 000000000..f3b6081f4 --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardUntrustedProxyTests.java @@ -0,0 +1,39 @@ +package cn.keking.guard; + +import org.junit.jupiter.api.Test; +import org.springframework.test.context.TestPropertySource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +/** No trusted proxy is configured: the client is the TCP peer and X-Forwarded-For is never believed. */ +@TestPropertySource(properties = { + "guard.enabled=true", + "guard.trusted-proxies=", + "guard.rate-limit=1000", + "guard.endpoint-rate-limits[/picturesPreview].requests=3", + "guard.kk.admin-allowed-ips=203.0.113.0/24" +}) +class GuardUntrustedProxyTests extends GuardHttpTestSupport { + + @Test + void spoofedForwardedForDoesNotResetTheBudget() throws Exception { + for (int i = 0; i < 3; i++) { + assertNotEquals(429, status("/picturesPreview", newClientIp()), "request " + i); + } + assertEquals(429, status("/picturesPreview", newClientIp()), "a fresh spoofed address does not buy a fresh budget"); + } + + @Test + void spoofedForwardedForDoesNotGrantAdminAccess() throws Exception { + assertEquals(403, postForm("/createFolder", "203.0.113.9", "folderName=a").statusCode()); + } + + @Test + void aLoopbackClientIsRejectedButNeverBanned() throws Exception { + for (int i = 0; i < 12; i++) { + assertEquals(400, status("/index?file=../../../../etc/passwd", "ignored")); + } + assertEquals(200, status("/index", "ignored"), "the engine does not ban loopback addresses"); + } +} diff --git a/server/src/test/java/cn/keking/guard/GuardUploadTests.java b/server/src/test/java/cn/keking/guard/GuardUploadTests.java new file mode 100644 index 000000000..3b64cd52a --- /dev/null +++ b/server/src/test/java/cn/keking/guard/GuardUploadTests.java @@ -0,0 +1,90 @@ +package cn.keking.guard; + +import cn.keking.config.ConfigConstants; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.test.context.TestPropertySource; + +import java.io.ByteArrayOutputStream; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** Real uploads through the guard with the shipped detection settings: the file lands intact and previewable source files are not rejected. */ +@TestPropertySource(properties = {"guard.enabled=true", "guard.trusted-proxies=127.0.0.1", "file.dir=${java.io.tmpdir}/kkfileview-guard-upload-tests"}) +class GuardUploadTests extends GuardHttpTestSupport { + + private static final String BOUNDARY = "kkguardupload"; + + private static Boolean previousUploadDisable; + + @BeforeAll + static void rememberConfiguration() { + previousUploadDisable = ConfigConstants.getFileUploadDisable(); + } + + @AfterAll + static void restoreConfiguration() throws Exception { + ConfigConstants.setFileUploadDisableValue(previousUploadDisable); + deleteRecursively(uploadRoot()); + } + + @BeforeEach + void enableUploads() { + ConfigConstants.setFileUploadDisableValue(false); + } + + private static Path uploadRoot() { + return Path.of(System.getProperty("java.io.tmpdir"), "kkfileview-guard-upload-tests"); + } + + private static void deleteRecursively(Path root) throws Exception { + if (Files.exists(root)) { + try (var paths = Files.walk(root)) { + paths.sorted(java.util.Comparator.reverseOrder()).forEach(path -> path.toFile().delete()); + } + } + } + + private HttpResponse upload(String fileName, byte[] content, String clientIp) throws Exception { + ByteArrayOutputStream body = new ByteArrayOutputStream(); + body.write(("--" + BOUNDARY + "\r\nContent-Disposition: form-data; name=\"file\"; filename=\"" + fileName + "\"\r\n" + + "Content-Type: application/octet-stream\r\n\r\n").getBytes(StandardCharsets.UTF_8)); + body.write(content); + body.write(("\r\n--" + BOUNDARY + "--\r\n").getBytes(StandardCharsets.UTF_8)); + return send(request("/fileUpload").header("X-Forwarded-For", clientIp) + .header("Content-Type", "multipart/form-data; boundary=" + BOUNDARY) + .POST(HttpRequest.BodyPublishers.ofByteArray(body.toByteArray())).build()); + } + + @Test + void uploadedFileIsStoredIntact() throws Exception { + byte[] content = new byte[300_000]; + for (int i = 0; i < content.length; i++) { + content[i] = (byte) ('a' + i % 26); + } + HttpResponse response = upload("intact.py", content, newClientIp()); + assertEquals(200, response.statusCode()); + assertTrue(response.body().contains("\"code\":0"), response.body()); + assertArrayEquals(content, Files.readAllBytes(uploadRoot().resolve("demo").resolve("intact.py"))); + } + + @Test + void sourceFileWithScriptMarkupIsNotRejected() throws Exception { + // kkFileView previews html, jsp, sql and shell sources, so a file part that looks like an attack must not be refused as one. + byte[] content = "\n-- 1 UNION SELECT username, password FROM users\n" + .getBytes(StandardCharsets.UTF_8); + HttpResponse response = upload("page.html", content, newClientIp()); + assertEquals(200, response.statusCode()); + assertTrue(response.body().contains("\"code\":0"), response.body()); + assertArrayEquals(content, Files.readAllBytes(uploadRoot().resolve("demo").resolve("page.html"))); + } +}