Update configure set to validate configuration keys and values (#10163)

* Implement newline and carriage-return sanitation for .

* Implement regression tests to verify newline and return-carriage characters cannot be used in 'configure set'.

* Add changelog entry.

* Minor patch to command under test.

* Fix tests so they write to disk properly.

* Remove values from the error message, update tests.

Author: aemous

.changes/next-release/bugfix-Configuration-98.json

@@ -0,0 +1,5 @@
+{
+  "type": "bugfix",
+  "category": "Configuration",
+  "description": "Update ``configure set`` command to return an error when newline or carriage-return characters are specified in the value."
+}

awscli/customizations/configure/writer.py

@@ -24,6 +24,19 @@ class ConfigFileWriter(object):
         r'(?P<value>.*)$'
     )
 
+    def _validate_no_newlines_or_carriage_returns(
+        self,
+        value,
+        label='value',
+        msg_override=None,
+    ):
+        if isinstance(value, str) and ('\n' in value or '\r' in value):
+            err_msg = msg_override if msg_override is not None else (
+                f"Invalid {label}: newline "
+                f"characters and carriage returns are not allowed: {value!r}"
+            )
+            raise ValueError(err_msg)
+
     def update_config(self, new_values, config_filename):
         """Update config file with new values.
 
@@ -52,6 +65,38 @@ def update_config(self, new_values, config_filename):
 
         """
         section_name = new_values.pop('__section__', 'default')
+        self._validate_no_newlines_or_carriage_returns(
+            section_name,
+            'section name'
+        )
+        for k, v in new_values.items():
+            self._validate_no_newlines_or_carriage_returns(k, 'key')
+            if not isinstance(v, dict):
+                # Override error msg to prevent
+                # leaking sensitive config values to stderr.
+                self._validate_no_newlines_or_carriage_returns(
+                    v,
+                    'value',
+                    msg_override=(
+                        f"Invalid value for key {k}: "
+                        f"newline characters and carriage "
+                        f"returns are not allowed."
+                    )
+                )
+            else:
+                for sk, sv in v.items():
+                    # Override error msg to prevent
+                    # leaking sensitive config values to stderr.
+                    self._validate_no_newlines_or_carriage_returns(sk, 'key')
+                    self._validate_no_newlines_or_carriage_returns(
+                        sv,
+                        'value',
+                        msg_override = (
+                            f"Invalid value for key {k}: "
+                            f"newline characters and carriage "
+                            f"returns are not allowed."
+                        )
+                    )
         if not os.path.isfile(config_filename):
             self._create_file(config_filename)
             self._write_new_section(section_name, new_values, config_filename)

tests/functional/configure/test_configure.py

@@ -374,6 +374,62 @@ def test_can_handle_empty_section(self):
             self.get_config_file_contents(),
         )
 
+    def test_set_rejects_newline_in_value(self):
+        _, stderr, _ = self.run_cmd(
+            ["configure", "set", "region", "us-east-1\nus-west-2"],
+            expected_rc=255,
+        )
+        self.assertIn("newline", stderr)
+        # To avoid leaking sensitive values,
+        # values should not appear in stderr.
+        self.assertNotIn("us-east-1\nus-west-2", stderr)
+
+    def test_set_rejects_carriage_return_in_value(self):
+        _, stderr, _ = self.run_cmd(
+            ["configure", "set", "region", "us-east-1\rus-west-2"],
+            expected_rc=255,
+        )
+        self.assertIn("newline", stderr)
+        # To avoid leaking sensitive values,
+        # values should not appear in stderr.
+        self.assertNotIn("us-east-1\rus-west-2", stderr)
+
+    def test_set_rejects_newline_in_nested_value(self):
+        _, stderr, _ = self.run_cmd(
+            ["configure", "set", "default.s3.signature_version", "s3v4\nfoo"],
+            expected_rc=255,
+        )
+        self.assertIn("newline", stderr)
+        # To avoid leaking sensitive values,
+        # values should not appear in stderr.
+        self.assertNotIn("s3v4\nfoo", stderr)
+
+    def test_newline_injection_does_not_write_injected_key_to_file(self):
+        # Simulates: aws configure set output $'table\nregion = us-east-1'
+        # The injected key must not appear anywhere in the config file.
+        self.set_config_file_contents("[default]\n")
+        self.run_cmd(
+            ["configure", "set", "output", "table\nregion = us-east-1"],
+            expected_rc=255,
+        )
+        contents = self.get_config_file_contents()
+        self.assertNotIn("region", contents)
+
+    def test_newline_injection_does_not_set_injected_key_in_parsed_config(self):
+        # Even if the file were somehow written, the injected key must not be
+        # readable back via 'configure get'.
+        self.set_config_file_contents("[default]\n")
+        self.run_cmd(
+            ["configure", "set", "output", "table\nregion = us-east-1"],
+            expected_rc=255,
+        )
+        # Re-create the driver so it re-reads the (unchanged) config file.
+        self.driver = create_clidriver()
+        stdout, _, _ = self.run_cmd(
+            "configure get region", expected_rc=1
+        )
+        self.assertEqual(stdout.strip(), "")
+
 
 class TestConfigureHasArgTable(unittest.TestCase):
     def test_configure_command_has_arg_table(self):

tests/unit/customizations/configure/test_writer.py

@@ -369,3 +369,39 @@ def test_appends_newline_on_new_section(self):
             '[new-section]\n'
             'region = us-west-2\n'
         )
+
+    def test_newline_in_value_raises(self):
+        with open(self.config_filename, 'w') as f:
+            f.write('[default]\nfoo = bar\n')
+        with self.assertRaises(ValueError):
+            self.writer.update_config({'foo': 'bad\nvalue'}, self.config_filename)
+
+    def test_carriage_return_in_value_raises(self):
+        with open(self.config_filename, 'w') as f:
+            f.write('[default]\nfoo = bar\n')
+        with self.assertRaises(ValueError):
+            self.writer.update_config({'foo': 'bad\rvalue'}, self.config_filename)
+
+    def test_newline_in_key_raises(self):
+        with open(self.config_filename, 'w') as f:
+            f.write('[default]\nfoo = bar\n')
+        with self.assertRaises(ValueError):
+            self.writer.update_config({'bad\nkey': 'value'}, self.config_filename)
+
+    def test_newline_in_section_name_raises(self):
+        with open(self.config_filename, 'w') as f:
+            f.write('[default]\nfoo = bar\n')
+        with self.assertRaises(ValueError):
+            self.writer.update_config(
+                {'foo': 'value', '__section__': 'bad\nsection'},
+                self.config_filename
+            )
+
+    def test_newline_in_nested_value_raises(self):
+        with open(self.config_filename, 'w') as f:
+            f.write('[default]\n')
+        with self.assertRaises(ValueError):
+            self.writer.update_config(
+                {'__section__': 'default', 's3': {'key': 'bad\nvalue'}},
+                self.config_filename
+            )