Skip to content

Commit

Permalink
Merge pull request #341 from DopplerHQ/max/eng-5001-cli-environment-v…
Browse files Browse the repository at this point in the history
…ariable-rce

ENG-5001: CLI Environment Variable RCE
  • Loading branch information
Piccirello authored Nov 7, 2022
2 parents ba95429 + fc3e1d3 commit 33f0329
Show file tree
Hide file tree
Showing 8 changed files with 411 additions and 75 deletions.
36 changes: 19 additions & 17 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,17 @@ module github.com/DopplerHQ/cli
go 1.19

require (
github.com/AlecAivazis/survey/v2 v2.3.4
github.com/AlecAivazis/survey/v2 v2.3.6
github.com/atotto/clipboard v0.1.4
github.com/google/uuid v1.3.0
github.com/hashicorp/go-version v1.5.0
github.com/hashicorp/go-version v1.6.0
github.com/jedib0t/go-pretty v4.3.0+incompatible
github.com/mattn/go-isatty v0.0.14
github.com/mattn/go-isatty v0.0.16
github.com/skratchdot/open-golang v0.0.0-20200116055534-eef842397966
github.com/spf13/cobra v1.4.0
github.com/spf13/cobra v1.6.0
github.com/stretchr/testify v1.8.1
github.com/zalando/go-keyring v0.2.1
golang.org/x/crypto v0.0.0-20220525230936-793ad666bf5e
golang.org/x/crypto v0.1.0
gopkg.in/gookit/color.v1 v1.1.6
gopkg.in/yaml.v3 v3.0.1
)
Expand All @@ -21,22 +22,23 @@ require (
github.com/alessio/shellescape v1.4.1 // indirect
github.com/asaskevich/govalidator v0.0.0-20210307081110-f21760c49a8d // indirect
github.com/danieljoos/wincred v1.1.2 // indirect
github.com/go-openapi/errors v0.20.2 // indirect
github.com/go-openapi/strfmt v0.21.2 // indirect
github.com/go-stack/stack v1.8.1 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-openapi/errors v0.20.3 // indirect
github.com/go-openapi/strfmt v0.21.3 // indirect
github.com/godbus/dbus/v5 v5.1.0 // indirect
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/google/go-cmp v0.5.9 // indirect
github.com/inconshreveable/mousetrap v1.0.1 // indirect
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect
github.com/mattn/go-colorable v0.1.12 // indirect
github.com/mattn/go-runewidth v0.0.13 // indirect
github.com/mattn/go-colorable v0.1.13 // indirect
github.com/mattn/go-runewidth v0.0.14 // indirect
github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d // indirect
github.com/mitchellh/mapstructure v1.5.0 // indirect
github.com/oklog/ulid v1.3.1 // indirect
github.com/rivo/uniseg v0.2.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/rivo/uniseg v0.4.2 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/stretchr/objx v0.2.0 // indirect
go.mongodb.org/mongo-driver v1.9.1 // indirect
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect
golang.org/x/term v0.0.0-20220526004731-065cf7ba2467 // indirect
golang.org/x/text v0.3.8 // indirect
go.mongodb.org/mongo-driver v1.10.3 // indirect
golang.org/x/sys v0.1.0 // indirect
golang.org/x/term v0.1.0 // indirect
golang.org/x/text v0.4.0 // indirect
)
110 changes: 53 additions & 57 deletions go.sum

Large diffs are not rendered by default.

31 changes: 31 additions & 0 deletions pkg/cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ type fallbackOptions struct {
passphrase string
}

var secretsToInclude []string

var runCmd = &cobra.Command{
Use: "run [command]",
Short: "Run a command with secrets injected into the environment",
Expand Down Expand Up @@ -145,6 +147,33 @@ doppler run --mount secrets.json -- cat secrets.json`,
shouldMountFile := mountPath != ""
shouldMountTemplate := mountTemplate != ""

// The potentially dangerous secret names only are only dangerous when they are set
// as environment variables since they have the potential to change the default shell behavior.
// When mounting the secrets into a file these are not dangerous
if !shouldMountFile {
if err := controllers.CheckForDangerousSecretNames(dopplerSecrets); err != nil {
utils.LogWarning(err.Error())
}
}

if cmd.Flags().Changed("only-secrets") {
if len(secretsToInclude) == 0 {
utils.HandleError(fmt.Errorf("you must specify secrets when using --only-secrets"))
}

var err error
noExitOnMissingIncludedSecrets := cmd.Flags().Changed("no-exit-on-missing-only-secrets")
dopplerSecrets, err = controllers.SelectSecrets(dopplerSecrets, secretsToInclude)

if err != nil {
if noExitOnMissingIncludedSecrets {
utils.LogWarning(err.Error())
} else {
utils.HandleError(err)
}
}
}

var mountFormat string
if mountFormatVal, ok := models.SecretsMountFormatMap[mountFormatString]; ok {
mountFormat = mountFormatVal
Expand Down Expand Up @@ -679,6 +708,8 @@ func init() {
runCmd.Flags().String("mount-format", "json", fmt.Sprintf("file format to use. if not specified, will be auto-detected from mount name. one of %v", models.SecretsMountFormats))
runCmd.Flags().String("mount-template", "", "template file to use. secrets will be rendered into this template before mount. see 'doppler secrets substitute' for more info.")
runCmd.Flags().Int("mount-max-reads", 0, "maximum number of times the mounted secrets file can be read (0 for unlimited)")
runCmd.Flags().StringSliceVar(&secretsToInclude, "only-secrets", []string{}, "only include the specified secrets")
runCmd.Flags().Bool("no-exit-on-missing-only-secrets", false, "do not exit on missing secrets via --only-secrets")

// deprecated
runCmd.Flags().Bool("silent-exit", false, "disable error output if the supplied command exits non-zero")
Expand Down
63 changes: 63 additions & 0 deletions pkg/controllers/secrets.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,33 @@ import (
"github.com/DopplerHQ/cli/pkg/utils"
)

// Documentation about potentially dangerous secret names can be found here: https://docs.doppler.com/docs/accessing-secrets#injection
var dangerousSecretNames = [...]string{
// Operating Systems environment variable names
// Linux
"PROMPT_COMMAND",
"LD_PRELOAD",
"LD_LIBRARY_PATH",
// Windows
"WINDIR",
"USERPROFILE",
// MacOS
"DYLD_INSERT_LIBRARIES",

// Language Interpreters environment variable names
// Perl & Python
"PERL5OPT",
// Python
"PYTHONWARNINGS",
"BROWSER",
// PHP
"HOSTNAME",
"PHPRC",
// NodeJS
"NODE_VERSION",
"NODE_OPTIONS",
}

func GetSecretNames(config models.ScopedOptions) ([]string, Error) {
utils.RequireValue("token", config.Token.Value)

Expand Down Expand Up @@ -196,3 +223,39 @@ func RenderSecretsTemplate(templateBody string, secretsMap map[string]string) st

return buffer.String()
}

func SelectSecrets(secrets map[string]string, secretsToSelect []string) (map[string]string, error) {
selectedSecrets := utils.FilterMap(secrets, secretsToSelect)
nonExistentSecretNames := []string{}

for _, secretName := range secretsToSelect {
if _, found := selectedSecrets[secretName]; !found {
nonExistentSecretNames = append(nonExistentSecretNames, secretName)
}
}

var err error
if len(nonExistentSecretNames) > 0 {
err = fmt.Errorf("the following secrets you are trying to include do not exist in your config:\n- %v", strings.Join(nonExistentSecretNames, "\n- "))
}

return selectedSecrets, err
}

// CheckForDangerousSecretNames checks for potential dangerous secret names.
// Documentation about potentially dangerous secret names can be found here: https://docs.doppler.com/docs/accessing-secrets#injection
func CheckForDangerousSecretNames(secrets map[string]string) error {
dangerousSecretNamesFound := []string{}

for _, dangerousName := range dangerousSecretNames {
if _, ok := secrets[dangerousName]; ok {
dangerousSecretNamesFound = append(dangerousSecretNamesFound, dangerousName)
}
}

if len(dangerousSecretNamesFound) > 0 {
return fmt.Errorf("your config contains the following potentially dangerous secret names (https://docs.doppler.com/docs/accessing-secrets#injection):\n- %s", strings.Join(dangerousSecretNamesFound, "\n- "))
}

return nil
}
125 changes: 125 additions & 0 deletions pkg/controllers/secrets_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
/*
Copyright © 2019 Doppler <[email protected]>
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package controllers

import (
"reflect"
"testing"

"github.com/stretchr/testify/assert"
)

type dangerousSecretNameTestCase struct {
name string
secrets map[string]string
expectedDangerousSecretNames []string
}

func TestCheckForDangerousSecretNames(t *testing.T) {
testCases := []dangerousSecretNameTestCase{
{
name: "Should not find any dangerous secret names",
secrets: map[string]string{
"MY_SECRET": "123",
},
expectedDangerousSecretNames: nil,
},
{
name: "Should find a dangerous secret name",
secrets: map[string]string{
"DYLD_INSERT_LIBRARIES": "123",
"MY_SECRET": "123",
},
expectedDangerousSecretNames: []string{"DYLD_INSERT_LIBRARIES"},
},
{
name: "Should find multiple dangerous secret names",
secrets: map[string]string{
"DYLD_INSERT_LIBRARIES": "123",
"MY_SECRET": "123",
"LD_LIBRARY_PATH": "123",
"WINDIR": "123",
"PROMPT_COMMAND": "123",
},
expectedDangerousSecretNames: []string{"DYLD_INSERT_LIBRARIES", "LD_LIBRARY_PATH", "WINDIR", "PROMPT_COMMAND"},
},
}

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
err := CheckForDangerousSecretNames(testCase.secrets)
if testCase.expectedDangerousSecretNames == nil {
assert.Nil(t, nil, err)
} else {
for _, v := range testCase.expectedDangerousSecretNames {
assert.Contains(t, err.Error(), v)
}
}
})
}
}

type selectSecretsTestCase struct {
name string
origMap map[string]string
keysToSelect []string
expectedMap map[string]string
missingKeys []string
}

func TestSelectSecrets(t *testing.T) {
testCases := []selectSecretsTestCase{
{
name: "Select one exisiting secret and two nonexistent secrets",
origMap: map[string]string{"MY_SECRET": "value"},
keysToSelect: []string{"DEV", "LOGGING", "MY_SECRET"},
expectedMap: map[string]string{"MY_SECRET": "value"},
missingKeys: []string{"DEV", "LOGGING"},
},
{
name: "Select one secret",
origMap: map[string]string{"DEV": "true", "LOGGING": "true"},
keysToSelect: []string{"DEV"},
expectedMap: map[string]string{"DEV": "true"},
},
{
name: "Select multiple secrets",
origMap: map[string]string{"DEV": "true", "LOGGING": "true", "MY_SECRET": "value", "PROD": "false"},
keysToSelect: []string{"DEV", "LOGGING", "PROD"},
expectedMap: map[string]string{"DEV": "true", "LOGGING": "true", "PROD": "false"},
},
}

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
filteredSecrets, err := SelectSecrets(testCase.origMap, testCase.keysToSelect)

if testCase.missingKeys != nil {
assert.NotNil(t, err)
for _, missingKey := range testCase.missingKeys {
assert.Contains(t, err.Error(), missingKey)
}
} else {
assert.Nil(t, err)
}

assert.True(t, reflect.DeepEqual(filteredSecrets, testCase.expectedMap))
})

}

}
30 changes: 30 additions & 0 deletions pkg/utils/filter_map.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
Copyright © 2020 Doppler <[email protected]>
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package utils

// FilterMap returns a new map[string]T containing origMap keys that match aginst the keysToSelect
// if a key in keysToSelect is not present in origMap it will be silently ignored
func FilterMap[T any](origMap map[string]T, keysToSelect []string) map[string]T {
filteredMap := map[string]T{}

for _, keyToSelect := range keysToSelect {
if _, ok := origMap[keyToSelect]; ok {
filteredMap[keyToSelect] = origMap[keyToSelect]
}
}

return filteredMap
}
Loading

0 comments on commit 33f0329

Please sign in to comment.