Repository navigation
Pre-launch security audit: 29 fixes - #89
Merged
Merged
Conversation
…d x-amz-target to be signed The AWS handler read up to 100 MB of body before looking at the signature, so anyone could make the server buffer that much per connection. It now checks the signature's freshness and access key first, verifies the signature before reading the body when the client declared the payload hash, and caps unsigned (public operation) bodies at 1 MB. SigV4 requests that do not sign host, or x-amz-target when present, are rejected as AWS does.
…public request bodies The console sign-in counted a failure only after verifying the password, so a burst of parallel guesses all passed the throttle; it now reserves a slot first and gives it back on success. Unknown users pay the same bcrypt cost as known ones. Sign-in and Cognito public routes cap their JSON body at 64 KB instead of 64 MB, and servers get an idle timeout and a header size limit.
…et blocklist HTTP_PROXY integrations used a default HTTP client, so anyone able to create an integration could read loopback services, the VPS provider's metadata service or the Docker bridge through a public API. They now use core.SafeClient, which checks the resolved address at dial time, ignores proxy environment variables and does not follow redirects. The shared blocklist now also covers IPv4-mapped/NAT64/6to4 forms, 0.0.0.0/8, non-link-local metadata addresses and every address of the host itself; HOMECLOUD_DENY_PRIVATE_TARGETS=1 also blocks RFC 1918 and unique-local ranges.
…ion registration The native task definition route (and CloudFormation, which uses it) accepted a task role without any PassRole check, so a caller with only ecs:RegisterTaskDefinition and ecs:RunTask could run a container holding any role that trusts ecs-tasks. The check now lives in registerTaskDef and is made against the role's real ARN, so alternative spellings such as role/x-/admin cannot slip past a scoped policy.
…very launch path The native RunInstances route, launch templates and Auto Scaling groups launching from a template accepted any instance profile without iam:PassRole (only the AWS RunInstances call checked it), letting a caller with ec2:RunInstances boot an instance holding any role's credentials through IMDS. The check is now shared (ec2.PassProfile) and applied on the native route, when a launch template carries a profile, and when a group is created or updated from such a template.
…rget ARNs Permission checks used the resource string exactly as the caller wrote it, so a Deny or scoped Allow on role/admin could be dodged with the bare name or an ARN with another path part (role/dev-/admin) that the IAM lookup still resolved to admin. Principal.Permits now canonicalizes the resource and IAM resolves role references to the real role ARN for iam:PassRole. Delivery to EventBridge, scheduler, CloudWatch and Step Functions targets, and Secrets Manager rotation functions, now rejects ARNs of another account or region instead of delivering to the same-named local resource.
A RedrivePolicy only had to name an existing queue, so a caller with sns:SetSubscriptionAttributes could make failed deliveries write attacker-chosen messages into any queue, including ones they cannot send to. Naming a dead-letter queue now needs sqs:SendMessage on it, like subscribing a queue.
Lambda container functions and ECS task definitions pulled from HomeCloud's registry (ECR-style or localhost URIs) without any ecr: check, so any user able to create a function or task could run, and read the contents of, another team's private repository. Both now need ecr:BatchGetImage and ecr:GetDownloadUrlForLayer on the repository.
…clickjacking headers The console opens an object inline with ?access_token=<session> in the URL, and the response was sandboxed with allow-scripts, so script inside an uploaded HTML object could read its own location and send the viewer's session token away. Inline views now run no scripts and load nothing from elsewhere, API responses send Referrer-Policy: no-referrer, and the console itself is served with X-Frame-Options/frame-ancestors so it cannot be framed.
…themselves ?access_token= accepted a full accessKeyId:secret, which would land in access logs and browser history; only console session tokens may travel in the query string now. GetSessionToken also refuses temporary credentials, as AWS does, so a stolen session cannot be renewed indefinitely.
…upload Packages were only checked for being a valid zip, then unpacked in memory at every cold start (and copied a second time into a tar), so a tiny archive of zeros could make each invocation allocate hundreds of megabytes. checkZip now sums the declared uncompressed sizes against the 250 MB limit and caps the entry count, and unzip reads no more than the remaining budget per file.
…paths) safeNext refused "//host" but let "/\\evil.example" and "/<tab>/evil.example" through; browsers read both as "//evil.example", so a crafted sign-in link sent the user to another site right after entering their password. The path is now rejected on backslashes and control characters and must resolve to this origin.
Unexpected errors (file paths, Docker daemon output, database errors) were copied into 500 responses, SQS batch results and, through the error message of the trail, into other users' CloudTrail LookupEvents. Clients now get a fixed message and the detail stays in the server log.
…/lambda-code/ out of S3 Website hosting decided that a bucket was public by looking for the strings "*" and s3:GetObject in MinIO's copy of the policy, then served every key with the server's storage credentials: a policy granting one prefix, or carrying a Deny or a condition, exposed the whole bucket. Each requested key is now evaluated against the bucket policy as an anonymous caller. DeleteObjects body keys get the same dot-segment and length checks as URL keys, and /lambda-code/ and /_s3/ are never claimed by anonymous S3 routing (a bucket named lambda-code broke code downloads).
Functions and HTTP_PROXY upstreams choose their own response headers, and respond() copied them over the sandbox Content-Security-Policy, so a function could serve a page with script on the console's origin and take over the session of an administrator who opened its URL. The sandbox and nosniff headers are now re-applied when the response is written.
The sign-in throttle was keyed by pool, user and source address, so rotating addresses had no per-account limit, and self sign-up (a bcrypt hash and a store write per call) had none at all. Password and SRP sign-in now also count attempts per account across addresses, and each pool accepts a bounded number of self sign-ups per window.
…lock headers A SigV2 signature covers only the sub-resources SigV2 defines, so whoever held a presigned URL could append ?retention, ?publicAccessBlock and similar and have the signer's permissions applied to the changed operation; those requests are now refused. Uploads that set retention or a legal hold through headers now need s3:PutObjectRetention / s3:PutObjectLegalHold, as in AWS.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 39 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (49)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying homecloud with
|
| Latest commit: |
9027d14
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://898f60ed.homecloud.pages.dev |
| Branch Preview URL: | https://security-audit.homecloud.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A security review before HomeCloud goes on public servers. Findings and reasoning: docs/security-audit-2026-10.md.
High-severity fixes:
Also: sign-in throttling before the password check and no user-existence timing leak, open redirect after sign-in, Lambda decompression bombs, internal error text no longer returned to clients or CloudTrail, ECR pull permissions for local images, DeleteObjects key validation, Cognito guess and sign-up limits, SigV2 sub-resource signing, SNS dead-letter permission, access keys refused in ?access_token, session tokens can't renew themselves, object-lock header authorization, lambda-code bucket routing.
Accepted risks are listed in the report with reasoning. VM instance code (PR #82) was out of scope and needs its own review.
Each fix has a regression test (except the console redirect, checked by hand); the full suite passes locally with Docker.