Skip to content

Commit 29aa371

Browse files
authored
Merge pull request #6 from github/nodeselector/lockfile-security-hardening
Security hardening: input validation and resource limits
2 parents 997d2e0 + 484f680 commit 29aa371

6 files changed

Lines changed: 567 additions & 6 deletions

File tree

‎go/pkg/lockfile/action_meta.go‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,30 @@ type ActionMeta struct {
2626
NestedUses []string
2727
}
2828

29+
// MaxActionMetaSize is the maximum byte length ParseActionMeta will accept.
30+
// action.yml files in the wild are well under 1 MiB.
31+
const MaxActionMetaSize = 1 << 20 // 1 MiB
32+
2933
// ParseActionMeta parses the contents of an action.yml file into an
3034
// ActionMeta. Composite actions emit their nested step `uses:` strings
3135
// in NestedUses; non-composite actions return an empty NestedUses.
3236
//
3337
// Returns an error only on malformed YAML — unknown `runs.using` values
3438
// resolve to ExecUnknown rather than failing.
3539
func ParseActionMeta(content string) (*ActionMeta, error) {
40+
if len(content) > MaxActionMetaSize {
41+
return nil, fmt.Errorf("action.yml too large: %d bytes (max %d)", len(content), MaxActionMetaSize)
42+
}
43+
44+
var doc yaml.Node
45+
if err := yaml.Unmarshal([]byte(content), &doc); err != nil {
46+
return nil, fmt.Errorf("parsing action.yml: %w", err)
47+
}
48+
49+
if err := rejectYAMLAnchors(&doc); err != nil {
50+
return nil, err
51+
}
52+
3653
var raw struct {
3754
Name string `yaml:"name"`
3855
Runs struct {
@@ -43,7 +60,7 @@ func ParseActionMeta(content string) (*ActionMeta, error) {
4360
} `yaml:"runs"`
4461
}
4562

46-
if err := yaml.Unmarshal([]byte(content), &raw); err != nil {
63+
if err := doc.Decode(&raw); err != nil {
4764
return nil, fmt.Errorf("parsing action.yml: %w", err)
4865
}
4966

@@ -54,9 +71,10 @@ func ParseActionMeta(content string) (*ActionMeta, error) {
5471
case using == "composite":
5572
meta.Execution = ExecComposite
5673
for _, step := range raw.Runs.Steps {
57-
if step.Uses != "" {
58-
meta.NestedUses = append(meta.NestedUses, step.Uses)
74+
if step.Uses == "" {
75+
continue
5976
}
77+
meta.NestedUses = append(meta.NestedUses, step.Uses)
6078
}
6179
case using == "docker":
6280
meta.Execution = ExecDocker
@@ -68,3 +86,24 @@ func ParseActionMeta(content string) (*ActionMeta, error) {
6886

6987
return meta, nil
7088
}
89+
90+
// rejectYAMLAnchors walks a yaml.Node tree and returns an error if any anchor
91+
// definition or alias reference is found. action.yml does not use YAML anchors,
92+
// so their presence is either a mistake or an attempted exploit.
93+
func rejectYAMLAnchors(n *yaml.Node) error {
94+
if n == nil {
95+
return nil
96+
}
97+
if n.Kind == yaml.AliasNode {
98+
return fmt.Errorf("action.yml: YAML anchors and aliases are not supported (line %d)", n.Line)
99+
}
100+
if n.Anchor != "" {
101+
return fmt.Errorf("action.yml: YAML anchors and aliases are not supported (line %d)", n.Line)
102+
}
103+
for _, child := range n.Content {
104+
if err := rejectYAMLAnchors(child); err != nil {
105+
return err
106+
}
107+
}
108+
return nil
109+
}

‎go/pkg/lockfile/action_meta_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package lockfile
33
import (
44
"errors"
55
"os"
6+
"strings"
67
"testing"
78

89
"github.com/stretchr/testify/assert"
@@ -35,3 +36,35 @@ func TestParseActionMeta(t *testing.T) {
3536
})
3637
}
3738
}
39+
40+
func TestParseActionMeta_YAMLAnchorsRejected(t *testing.T) {
41+
// Anchor definition
42+
withAnchor := `
43+
name: anchored
44+
runs:
45+
using: composite
46+
steps:
47+
- &step
48+
uses: actions/checkout@v4
49+
- *step
50+
`
51+
_, err := ParseActionMeta(withAnchor)
52+
require.Error(t, err)
53+
assert.Contains(t, err.Error(), "anchors and aliases are not supported")
54+
}
55+
56+
func TestParseActionMeta_OversizedInputRejected(t *testing.T) {
57+
// Build an input just over MaxActionMetaSize.
58+
oversized := "name: big\n" + strings.Repeat("# padding\n", (MaxActionMetaSize/10)+1)
59+
_, err := ParseActionMeta(oversized)
60+
require.Error(t, err)
61+
assert.Contains(t, err.Error(), "too large")
62+
}
63+
64+
func TestParseActionMeta_ExactMaxSizeAccepted(t *testing.T) {
65+
// A valid minimal composite action padded to exactly MaxActionMetaSize must be accepted.
66+
base := "name: padded\nruns:\n using: composite\n steps:\n - uses: actions/checkout@v4\n"
67+
pad := strings.Repeat("#", MaxActionMetaSize-len(base))
68+
_, err := ParseActionMeta(base + pad)
69+
require.NoError(t, err)
70+
}

‎go/pkg/lockfile/lockfile.go‎

Lines changed: 189 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"regexp"
77
"strconv"
8+
"strings"
89

910
"gopkg.in/yaml.v3"
1011
)
@@ -244,6 +245,11 @@ type Action struct {
244245
//
245246
// The optional paths parameter scopes per-dependency validation to only the
246247
// entries referenced by the named workflow paths (via f.Workflows[p]).
248+
// MaxParseSize is the maximum number of bytes Parse will accept. Inputs larger
249+
// than this are rejected before any YAML parsing takes place to prevent
250+
// memory-exhaustion DoS from oversized or yaml-bomb documents.
251+
const MaxParseSize = 1 << 20 // 1 MiB
252+
247253
// When paths is empty, every dependency entry is validated — the default for
248254
// whole-file tooling (CLI regen, Dependabot). When paths is non-empty, a
249255
// dependency entry outside the referenced set is left unchecked so one
@@ -261,6 +267,11 @@ type Action struct {
261267
// can flag them via diagnostics. Workflow path keys are NOT canonicalized
262268
// — filesystem paths are case-sensitive on the platforms we run on.
263269
func Parse(contents []byte, paths ...string) (File, error) {
270+
if len(contents) > MaxParseSize {
271+
return File{}, &ParseError{
272+
Msg: fmt.Sprintf("lockfile too large: %d bytes (max %d)", len(contents), MaxParseSize),
273+
}
274+
}
264275
var root yaml.Node
265276
if err := yaml.Unmarshal(contents, &root); err != nil {
266277
return File{}, newYAMLParseError(err)
@@ -301,6 +312,9 @@ func Parse(contents []byte, paths ...string) (File, error) {
301312
if pe := validateKnownFields(&f, paths); pe != nil {
302313
return File{}, pe
303314
}
315+
if pe := validateWorkflowPaths(&f); pe != nil {
316+
return File{}, pe
317+
}
304318
if conflictKey, err := canonicalizeActions(&f); err != nil {
305319
pe := &ParseError{Msg: err.Error(), err: err}
306320
if l, c, ok := f.KeyPosition("dependencies", conflictKey); ok {
@@ -309,9 +323,126 @@ func Parse(contents []byte, paths ...string) (File, error) {
309323
return File{}, pe
310324
}
311325
canonicalizeWorkflowDependencies(&f)
326+
if cycle, err := detectUsesCycle(&f); err != nil {
327+
pe := &ParseError{Msg: err.Error()}
328+
if l, c, ok := f.KeyPosition("dependencies", cycle); ok {
329+
pe.Line, pe.Column = l, c
330+
}
331+
return File{}, pe
332+
}
312333
return f, nil
313334
}
314335

336+
// detectUsesCycle reports a cycle in the action uses graph using
337+
// recursive DFS with three-colour marking. It returns the key of the node
338+
// that forms the back-edge, or ("", nil) when the graph is acyclic.
339+
//
340+
// The recursion depth is bounded by the number of unique keys in
341+
// f.Dependencies, which is itself bounded by MaxParseSize (a 1 MiB file
342+
// can hold at most ~5,000 dependency entries). Go's default goroutine stack
343+
// grows dynamically up to 1 GB, so 5,000 frames is well within budget.
344+
//
345+
// The runner rejects cycles at execution time via CompositeActionsMaxDepth
346+
// (actions/runner: src/Runner.Common/Constants.cs). Detecting them at parse
347+
// time shifts the failure left so consumers never receive a File whose uses
348+
// graph is not a DAG.
349+
func detectUsesCycle(f *File) (cycleKey string, err error) {
350+
const (
351+
white = 0 // unvisited
352+
grey = 1 // on the current DFS stack
353+
black = 2 // fully processed
354+
)
355+
color := make(map[string]int, len(f.Dependencies))
356+
357+
var visit func(key string) bool
358+
visit = func(key string) bool {
359+
if color[key] == grey {
360+
cycleKey = key
361+
return true
362+
}
363+
if color[key] == black {
364+
return false
365+
}
366+
color[key] = grey
367+
action, ok := f.Dependencies[key]
368+
if ok {
369+
for _, dep := range action.Uses {
370+
if visit(dep) {
371+
if cycleKey == "" {
372+
cycleKey = key
373+
}
374+
return true
375+
}
376+
}
377+
}
378+
color[key] = black
379+
return false
380+
}
381+
382+
for key := range f.Dependencies {
383+
if color[key] == white {
384+
if visit(key) {
385+
return cycleKey, fmt.Errorf("uses cycle detected at dependency %q", cycleKey)
386+
}
387+
}
388+
}
389+
return "", nil
390+
}
391+
392+
// validateWorkflowPaths checks that every key in f.Workflows is a safe
393+
// repo-relative file path. Workflow keys are used by consumers as file paths
394+
// (e.g. to open the workflow file on disk), so a crafted lockfile with a key
395+
// like "../../../etc/passwd" or an absolute path "/etc/shadow" would give
396+
// any consumer that calls os.Open(key) an arbitrary-read primitive.
397+
//
398+
// Rules:
399+
// - Must not be empty.
400+
// - Must not be an absolute path (no leading "/").
401+
// - Must not contain ".." as a path segment — rejects traversal after
402+
// path.Clean even if embedded in a longer path.
403+
// - Must not contain null bytes or other control characters.
404+
func validateWorkflowPaths(f *File) *ParseError {
405+
_, workflowsNode := mappingEntry(docMapping(f.node), "workflows")
406+
for key := range f.Workflows {
407+
if err := checkWorkflowPathKey(key); err != nil {
408+
pe := &ParseError{Msg: err.Error()}
409+
if workflowsNode != nil {
410+
if k, _ := mappingEntry(workflowsNode, key); k != nil {
411+
pe.Line, pe.Column = k.Line, k.Column
412+
}
413+
}
414+
return pe
415+
}
416+
}
417+
return nil
418+
}
419+
420+
func checkWorkflowPathKey(p string) error {
421+
if p == "" {
422+
return fmt.Errorf("workflow path key must not be empty")
423+
}
424+
if strings.HasPrefix(p, "/") {
425+
return fmt.Errorf("workflow path key must be repo-relative, not absolute: %q", p)
426+
}
427+
for _, c := range p {
428+
if c <= 0x1F || c == 0x7F {
429+
return fmt.Errorf("workflow path key contains control characters: %q", p)
430+
}
431+
// Reject backslash and colon to prevent Windows-style absolute paths
432+
// (e.g. "C:\..." or UNC "\\server\...") and backslash-based traversal
433+
// (e.g. "..\\..\\.." ) from bypassing the forward-slash checks above.
434+
if c == '\\' || c == ':' {
435+
return fmt.Errorf("workflow path key contains invalid character %q: %q", string(c), p)
436+
}
437+
}
438+
for _, seg := range strings.Split(p, "/") {
439+
if seg == ".." {
440+
return fmt.Errorf("workflow path key contains path traversal: %q", p)
441+
}
442+
}
443+
return nil
444+
}
445+
315446
// allowedFileKeys is the set of permitted top-level lockfile keys. It mirrors
316447
// the document-level properties declared in lockfile-v0.0.1.json.
317448
var allowedFileKeys = map[string]struct{}{
@@ -428,6 +559,7 @@ func validateKnownFields(f *File, paths []string) *ParseError {
428559
// nonEmptyStringKeys lists action fields that must be non-empty strings.
429560
var nonEmptyStringKeys = map[string]struct{}{
430561
"branch": {},
562+
"commit": {},
431563
}
432564

433565
// positiveIntKeys lists action fields that must be positive integers (> 0).
@@ -437,9 +569,12 @@ var positiveIntKeys = map[string]struct{}{
437569
}
438570

439571
// rejectZeroValues checks that required action fields carry meaningful values:
440-
// string fields like "branch" must be non-empty, and integer ID fields like
441-
// "owner_id" and "repo_id" must be positive. A present-but-zero-value field
442-
// would silently disable the security check it's meant to enforce.
572+
// string fields like "branch" must be non-empty, integer ID fields like
573+
// "owner_id" and "repo_id" must be positive, the "commit" field must be
574+
// a valid algo-hex digest string, and the "branch"/"tag" fields must not
575+
// contain characters that are unsafe for downstream interpolation. A
576+
// present-but-zero-value or injection-bearing field would silently disable
577+
// the security check it's meant to enforce, or arm a downstream injection.
443578
func rejectZeroValues(action *yaml.Node, dep string) *ParseError {
444579
for j := 0; j+1 < len(action.Content); j += 2 {
445580
key := action.Content[j]
@@ -455,6 +590,31 @@ func rejectZeroValues(action *yaml.Node, dep string) *ParseError {
455590
}
456591
}
457592

593+
if key.Value == "commit" && val.Value != "" {
594+
if !isValidAlgoHex(val.Value) {
595+
return &ParseError{
596+
Line: val.Line,
597+
Column: val.Column,
598+
Msg: fmt.Sprintf("action field \"commit\" must be an algo-hex digest (e.g. \"sha1-abc...\") for dependency %q, got %q", dep, val.Value),
599+
}
600+
}
601+
}
602+
603+
// branch and tag values are used in GraphQL queries, log output,
604+
// and sometimes shell commands by consumers. Validate them with
605+
// the same denylist that ParseActionRef applies to refs so that a
606+
// crafted lockfile cannot arm a downstream injection through these
607+
// fields.
608+
if (key.Value == "branch" || key.Value == "tag") && val.Value != "" {
609+
if !isValidRef(val.Value) {
610+
return &ParseError{
611+
Line: val.Line,
612+
Column: val.Column,
613+
Msg: fmt.Sprintf("action field %q contains unsafe characters for dependency %q: %q", key.Value, dep, val.Value),
614+
}
615+
}
616+
}
617+
458618
if _, ok := positiveIntKeys[key.Value]; ok {
459619
n, err := strconv.ParseInt(val.Value, 10, 64)
460620
if err != nil || n <= 0 {
@@ -483,6 +643,18 @@ func canonicalizeActions(f *File) (string, error) {
483643
canonical := key
484644
if pin, ok := ParsePin(key); ok {
485645
canonical = pin.String()
646+
// The commit field in the action body must match the digest
647+
// embedded in the pin key. A mismatch is a trust-confusion
648+
// attack: the caller looks up the action by key (and its
649+
// embedded hash) but the body carries a different hash that
650+
// a different downstream check might trust.
651+
pinDigest := pin.Algo + "-" + pin.Hex
652+
if action.Commit != "" && action.Commit != pinDigest {
653+
return key, fmt.Errorf(
654+
"action %q commit field %q disagrees with pin key digest %q",
655+
canonical, action.Commit, pinDigest,
656+
)
657+
}
486658
}
487659
// Canonicalize Uses entries too so cross-references resolve.
488660
if len(action.Uses) > 0 {
@@ -585,3 +757,17 @@ func parseSchemaVersion(v string) ([3]int, bool) {
585757
}
586758
return out, true
587759
}
760+
761+
// isValidAlgoHex reports whether s is a properly-formed algo-hex digest string
762+
// in the lockfile's "algo-hexdigest" format (e.g. "sha1-abc123..." or
763+
// "sha256-abc123..."). It delegates hex and length validation to isValidDigest
764+
// so the two never drift apart.
765+
func isValidAlgoHex(s string) bool {
766+
dashIdx := strings.IndexByte(s, '-')
767+
if dashIdx <= 0 || dashIdx == len(s)-1 {
768+
return false
769+
}
770+
algo := strings.ToLower(s[:dashIdx])
771+
hex := strings.ToLower(s[dashIdx+1:])
772+
return isValidDigest(algo, hex)
773+
}

0 commit comments

Comments
 (0)