-
Notifications
You must be signed in to change notification settings - Fork 45
v2 hardening effort part 1 #578
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
96b78a1
08d01e2
e4f43fd
9ab6254
c2a559a
a962ad1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,11 +82,14 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags | |
| if err != nil { | ||
| return err | ||
| } | ||
| content, err := io.ReadAll(rc) | ||
| content, err := io.ReadAll(io.LimitReader(rc, consts.MaxManifestBytes+1)) | ||
| rc.Close() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if int64(len(content)) > consts.MaxManifestBytes { | ||
| return fmt.Errorf("product manifest for [%s] exceeds maximum allowed size (%d bytes)", productName, consts.MaxManifestBytes) | ||
| } | ||
|
|
||
| // Ensure each manifest starts with a YAML document separator. | ||
| if !strings.HasPrefix(string(content), "---") { | ||
|
|
@@ -161,7 +164,7 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags | |
| if strings.HasPrefix(haulPath, "http://") || strings.HasPrefix(haulPath, "https://") { | ||
| l.Debugf("detected remote manifest... starting download... [%s]", haulPath) | ||
|
|
||
| h := getter.NewHttp() | ||
| h := getter.NewHttpWithOptions(getter.HttpOptions{AllowInternalTargets: rso.AllowInternalTargets}) | ||
| parsedURL, err := url.Parse(haulPath) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -184,9 +187,13 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags | |
| } | ||
| defer out.Close() | ||
|
|
||
| if _, err = io.Copy(out, rc); err != nil { | ||
| n, err := io.Copy(out, io.LimitReader(rc, consts.MaxDownloadBytes+1)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if n > consts.MaxDownloadBytes { | ||
| return fmt.Errorf("remote manifest at %s exceeds maximum allowed size (%d bytes)", haulPath, consts.MaxDownloadBytes) | ||
| } | ||
|
Comment on lines
192
to
+196
|
||
| } | ||
|
|
||
| fi, err := os.Open(haulPath) | ||
|
|
@@ -213,7 +220,7 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags | |
| if strings.HasPrefix(haulPath, "http://") || strings.HasPrefix(haulPath, "https://") { | ||
| l.Debugf("detected remote image.txt... starting download... [%s]", haulPath) | ||
|
|
||
| h := getter.NewHttp() | ||
| h := getter.NewHttpWithOptions(getter.HttpOptions{AllowInternalTargets: rso.AllowInternalTargets}) | ||
| parsedURL, err := url.Parse(haulPath) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -236,9 +243,13 @@ func SyncCmd(ctx context.Context, o *flags.SyncOpts, s *store.Layout, rso *flags | |
| } | ||
| defer out.Close() | ||
|
|
||
| if _, err = io.Copy(out, rc); err != nil { | ||
| n, err := io.Copy(out, io.LimitReader(rc, consts.MaxDownloadBytes+1)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if n > consts.MaxDownloadBytes { | ||
| return fmt.Errorf("remote image.txt at %s exceeds maximum allowed size (%d bytes)", haulPath, consts.MaxDownloadBytes) | ||
| } | ||
|
Comment on lines
248
to
+252
|
||
| } | ||
|
|
||
| fi, err := os.Open(haulPath) | ||
|
|
@@ -296,7 +307,7 @@ func processContent(ctx context.Context, fi *os.File, o *flags.SyncOpts, s *stor | |
| return err | ||
| } | ||
| for _, f := range cfg.Spec.Files { | ||
| if err := storeFile(ctx, s, f); err != nil { | ||
| if err := storeFile(ctx, s, f, rso.AllowInternalTargets); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -150,8 +150,8 @@ if [ ! -d "${HAULER_DIR}" ]; then | |||||
| mkdir -p "${HAULER_DIR}" || fatal "Failed to Create Hauler Directory: ${HAULER_DIR}" | ||||||
| fi | ||||||
|
|
||||||
| # ensure hauler directory is writable (by user or root privileges) | ||||||
| chmod -R 777 "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" | ||||||
| # ensure hauler directory is only accessible by the owner | ||||||
| chmod -R 0700 "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" | ||||||
|
||||||
| chmod -R 0700 "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" | |
| chmod -R go-rwx,u+rwX "${HAULER_DIR}" || fatal "Failed to Update Permissions of Hauler Directory: ${HAULER_DIR}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This download path bounds the copy with io.LimitReader(rc, MaxDownloadBytes+1) and then checks
n > MaxDownloadBytesafter writing. That can leave an oversized temp file (up to +1 byte) on disk even though the operation fails. Consider copying only up toMaxDownloadBytesand probing for one extra byte (or truncating/removing the temp file) to enforce a strict on-disk cap; also ensure the reported URL in any error message isn’t using the rewritten localhaulPathvalue.