Skip to content

Commit 86ffb66

Browse files
marcoandredinismarcoandredinis
authored
Escape labels on Node Join scripts (#52350) (#52705)
1 parent 8091bb6 commit 86ffb66

File tree

3 files changed

+110
-28
lines changed

3 files changed

+110
-28
lines changed

‎lib/web/join_tokens.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,10 @@ func getJoinScript(ctx context.Context, settings scriptSettings, m nodeAPIGetter
603603

604604
labelsList := []string{}
605605
for labelKey, labelValues := range token.GetSuggestedLabels() {
606+
labelKey = shsprintf.EscapeDefaultContext(labelKey)
607+
for i := range labelValues {
608+
labelValues[i] = shsprintf.EscapeDefaultContext(labelValues[i])
609+
}
606610
labels := strings.Join(labelValues, " ")
607611
labelsList = append(labelsList, fmt.Sprintf("%s=%s", labelKey, labels))
608612
}
@@ -660,7 +664,7 @@ func getJoinScript(ctx context.Context, settings scriptSettings, m nodeAPIGetter
660664
// This section relies on Go's default zero values to make sure that the settings
661665
// are correct when not installing an app.
662666
err = scripts.InstallNodeBashScript.Execute(&buf, map[string]interface{}{
663-
"token": settings.token,
667+
"token": shsprintf.EscapeDefaultContext(settings.token),
664668
"hostname": hostname,
665669
"port": portStr,
666670
// The install.sh script has some manually generated configs and some

‎lib/web/join_tokens_test.go

+100-26
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,7 @@ func toHex(s string) string { return hex.EncodeToString([]byte(s)) }
603603

604604
func TestGetNodeJoinScript(t *testing.T) {
605605
validToken := "f18da1c9f6630a51e8daf121e7451daa"
606+
validTokenWithLabelsWithSpecialChars := "f18da1c9f6630a51e8daf121e7451dbb"
606607
validIAMToken := "valid-iam-token"
607608
internalResourceID := "967d38ff-7a61-4f42-bd2d-c61965b44db0"
608609

@@ -618,27 +619,33 @@ func TestGetNodeJoinScript(t *testing.T) {
618619
return &proto.GetClusterCACertResponse{TLSCA: fakeBytes}, nil
619620
},
620621
mockGetToken: func(_ context.Context, token string) (types.ProvisionToken, error) {
621-
if token == validToken || token == validIAMToken {
622-
return &types.ProvisionTokenV2{
623-
Metadata: types.Metadata{
624-
Name: token,
625-
},
626-
Spec: types.ProvisionTokenSpecV2{
627-
SuggestedLabels: types.Labels{
628-
types.InternalResourceIDLabel: utils.Strings{internalResourceID},
629-
},
622+
baseToken := &types.ProvisionTokenV2{
623+
Metadata: types.Metadata{
624+
Name: token,
625+
},
626+
Spec: types.ProvisionTokenSpecV2{
627+
SuggestedLabels: types.Labels{
628+
types.InternalResourceIDLabel: utils.Strings{internalResourceID},
630629
},
631-
}, nil
630+
},
632631
}
633-
return nil, trace.NotFound("token does not exist")
632+
switch token {
633+
case validToken, validIAMToken:
634+
case validTokenWithLabelsWithSpecialChars:
635+
baseToken.Spec.SuggestedLabels["env"] = []string{"bad label value | ; & $ > < ' !"}
636+
baseToken.Spec.SuggestedLabels["bad label key | ; & $ > < ' !"] = []string{"env"}
637+
default:
638+
return nil, trace.NotFound("token does not exist")
639+
}
640+
return baseToken, nil
634641
},
635642
}
636643

637644
for _, test := range []struct {
638645
desc string
639646
settings scriptSettings
640647
errAssert require.ErrorAssertionFunc
641-
extraAssertions func(script string)
648+
extraAssertions func(t *testing.T, script string)
642649
}{
643650
{
644651
desc: "zero value",
@@ -659,7 +666,7 @@ func TestGetNodeJoinScript(t *testing.T) {
659666
desc: "valid",
660667
settings: scriptSettings{token: validToken},
661668
errAssert: require.NoError,
662-
extraAssertions: func(script string) {
669+
extraAssertions: func(t *testing.T, script string) {
663670
require.Contains(t, script, validToken)
664671
require.Contains(t, script, "test-host")
665672
require.Contains(t, script, "12345678")
@@ -682,19 +689,28 @@ func TestGetNodeJoinScript(t *testing.T) {
682689
joinMethod: string(types.JoinMethodIAM),
683690
},
684691
errAssert: require.NoError,
685-
extraAssertions: func(script string) {
692+
extraAssertions: func(t *testing.T, script string) {
686693
require.Contains(t, script, "JOIN_METHOD='iam'")
687694
},
688695
},
689696
{
690697
desc: "internal resourceid label",
691698
settings: scriptSettings{token: validToken},
692699
errAssert: require.NoError,
693-
extraAssertions: func(script string) {
700+
extraAssertions: func(t *testing.T, script string) {
694701
require.Contains(t, script, "--labels ")
695702
require.Contains(t, script, fmt.Sprintf("%s=%s", types.InternalResourceIDLabel, internalResourceID))
696703
},
697704
},
705+
{
706+
desc: "attempt to shell injection using suggested labels",
707+
settings: scriptSettings{token: validTokenWithLabelsWithSpecialChars},
708+
errAssert: require.NoError,
709+
extraAssertions: func(t *testing.T, script string) {
710+
require.Contains(t, script, `bad\ label\ key\ \|\ \;\ \&\ \$\ \>\ \<\ \'\ \!=env`)
711+
require.Contains(t, script, `env=bad\ label\ value\ \|\ \;\ \&\ \$\ \>\ \<\ \'\ \!`)
712+
},
713+
},
698714
} {
699715
t.Run(test.desc, func(t *testing.T) {
700716
script, err := getJoinScript(context.Background(), test.settings, m)
@@ -704,7 +720,7 @@ func TestGetNodeJoinScript(t *testing.T) {
704720
}
705721

706722
if test.extraAssertions != nil {
707-
test.extraAssertions(script)
723+
test.extraAssertions(t, script)
708724
}
709725
})
710726
}
@@ -899,6 +915,8 @@ func TestGetAppJoinScript(t *testing.T) {
899915
func TestGetDatabaseJoinScript(t *testing.T) {
900916
validToken := "f18da1c9f6630a51e8daf121e7451daa"
901917
emptySuggestedAgentMatcherLabelsToken := "f18da1c9f6630a51e8daf121e7451000"
918+
wildcardLabelMatcherToken := "f18da1c9f6630a51e8daf121e7451001"
919+
tokenWithSpecialChars := "f18da1c9f6630a51e8daf121e7451002"
902920
internalResourceID := "967d38ff-7a61-4f42-bd2d-c61965b44db0"
903921

904922
m := &mockedNodeAPIGetter{
@@ -935,6 +953,22 @@ func TestGetDatabaseJoinScript(t *testing.T) {
935953
provisionToken.Spec.SuggestedAgentMatcherLabels = types.Labels{}
936954
return provisionToken, nil
937955
}
956+
if token == wildcardLabelMatcherToken {
957+
provisionToken.Spec.SuggestedAgentMatcherLabels = types.Labels{"*": []string{"*"}}
958+
return provisionToken, nil
959+
}
960+
if token == tokenWithSpecialChars {
961+
provisionToken.Spec.SuggestedAgentMatcherLabels = types.Labels{
962+
"*": utils.Strings{"*"},
963+
"spa ces": utils.Strings{"spa ces"},
964+
"EOF": utils.Strings{"test heredoc"},
965+
`"EOF"`: utils.Strings{"test quoted heredoc"},
966+
"#'; <>\\#": utils.Strings{"try to escape yaml"},
967+
"&<>'\"$A,./;'BCD ${ABCD}": utils.Strings{"key with special characters"},
968+
"value with special characters": utils.Strings{"&<>'\"$A,./;'BCD ${ABCD}", "#&<>'\"$A,./;'BCD ${ABCD}"},
969+
}
970+
return provisionToken, nil
971+
}
938972
return nil, trace.NotFound("token does not exist")
939973
},
940974
}
@@ -943,7 +977,7 @@ func TestGetDatabaseJoinScript(t *testing.T) {
943977
desc string
944978
settings scriptSettings
945979
errAssert require.ErrorAssertionFunc
946-
extraAssertions func(script string)
980+
extraAssertions func(t *testing.T, script string)
947981
}{
948982
{
949983
desc: "two installation methods",
@@ -961,22 +995,65 @@ func TestGetDatabaseJoinScript(t *testing.T) {
961995
token: validToken,
962996
},
963997
errAssert: require.NoError,
964-
extraAssertions: func(script string) {
998+
extraAssertions: func(t *testing.T, script string) {
965999
require.Contains(t, script, validToken)
9661000
require.Contains(t, script, "test-host")
9671001
require.Contains(t, script, "sha256:")
9681002
require.Contains(t, script, "--labels ")
9691003
require.Contains(t, script, fmt.Sprintf("%s=%s", types.InternalResourceIDLabel, internalResourceID))
9701004
require.Contains(t, script, `
971-
db_service:
972-
enabled: "yes"
973-
resources:
9741005
- labels:
9751006
env: prod
9761007
os:
9771008
- mac
9781009
- linux
9791010
product: '*'
1011+
`)
1012+
},
1013+
},
1014+
{
1015+
desc: "discover flow with wildcard label matcher",
1016+
settings: scriptSettings{
1017+
databaseInstallMode: true,
1018+
token: wildcardLabelMatcherToken,
1019+
},
1020+
errAssert: require.NoError,
1021+
extraAssertions: func(t *testing.T, script string) {
1022+
require.Contains(t, script, wildcardLabelMatcherToken)
1023+
require.Contains(t, script, "test-host")
1024+
require.Contains(t, script, "sha256:")
1025+
require.Contains(t, script, "--labels ")
1026+
require.Contains(t, script, fmt.Sprintf("%s=%s", types.InternalResourceIDLabel, internalResourceID))
1027+
require.Contains(t, script, `
1028+
- labels:
1029+
'*': '*'
1030+
`)
1031+
},
1032+
},
1033+
{
1034+
desc: "discover flow with shell injection attempt in resource matcher labels",
1035+
settings: scriptSettings{
1036+
databaseInstallMode: true,
1037+
token: tokenWithSpecialChars,
1038+
},
1039+
errAssert: require.NoError,
1040+
extraAssertions: func(t *testing.T, script string) {
1041+
require.Contains(t, script, tokenWithSpecialChars)
1042+
require.Contains(t, script, "test-host")
1043+
require.Contains(t, script, "sha256:")
1044+
require.Contains(t, script, "--labels ")
1045+
require.Contains(t, script, fmt.Sprintf("%s=%s", types.InternalResourceIDLabel, internalResourceID))
1046+
require.Contains(t, script, `
1047+
- labels:
1048+
'"EOF"': test quoted heredoc
1049+
'#''; <>\#': try to escape yaml
1050+
'&<>''"$A,./;''BCD ${ABCD}': key with special characters
1051+
'*': '*'
1052+
EOF: test heredoc
1053+
spa ces: spa ces
1054+
value with special characters:
1055+
- '&<>''"$A,./;''BCD ${ABCD}'
1056+
- '#&<>''"$A,./;''BCD ${ABCD}'
9801057
`)
9811058
},
9821059
},
@@ -987,16 +1064,13 @@ db_service:
9871064
token: emptySuggestedAgentMatcherLabelsToken,
9881065
},
9891066
errAssert: require.NoError,
990-
extraAssertions: func(script string) {
1067+
extraAssertions: func(t *testing.T, script string) {
9911068
require.Contains(t, script, emptySuggestedAgentMatcherLabelsToken)
9921069
require.Contains(t, script, "test-host")
9931070
require.Contains(t, script, "sha256:")
9941071
require.Contains(t, script, "--labels ")
9951072
require.Contains(t, script, fmt.Sprintf("%s=%s", types.InternalResourceIDLabel, internalResourceID))
9961073
require.Contains(t, script, `
997-
db_service:
998-
enabled: "yes"
999-
resources:
10001074
- labels:
10011075
{}
10021076
`)
@@ -1011,7 +1085,7 @@ db_service:
10111085
}
10121086

10131087
if test.extraAssertions != nil {
1014-
test.extraAssertions(script)
1088+
test.extraAssertions(t, script)
10151089
}
10161090
})
10171091
}

‎lib/web/scripts/node-join/install.sh

+5-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ JOIN_METHOD_FLAG=""
4747
[ -n "$JOIN_METHOD" ] && JOIN_METHOD_FLAG="--join-method ${JOIN_METHOD}"
4848

4949
# inject labels into the configuration
50-
LABELS='{{.labels}}'
50+
LABELS="{{.labels}}"
5151
LABELS_FLAG=()
5252
[ -n "$LABELS" ] && LABELS_FLAG=(--labels "${LABELS}")
5353

@@ -494,6 +494,10 @@ proxy_service:
494494
db_service:
495495
enabled: "yes"
496496
resources:
497+
EOF
498+
499+
# Quoting the EOF heredoc indicates to shell to treat this as a literal string and does not try to interpolate or execute anything.
500+
cat << "EOF" >> ${TELEPORT_CONFIG_PATH}
497501
- labels:{{range $index, $line := .db_service_resource_labels}}
498502
{{$line -}}
499503
{{end}}

0 commit comments

Comments
 (0)