reject control chars in written values in configuration#2137
Merged
Conversation
Reject CR, LF, and NUL in GitConfigParser values before writing them
to git config files (which also is a deviation from Git which escapes them).
GitConfigParser._write() serializes embedded newlines as indented
continuation lines by replacing "\n" with "\n\t". Git itself skips
leading whitespace before parsing config tokens, so an injected value
such as:
foo
[core]
hooksPath=/tmp/hooks
is written in a form where the indented "[core]" line is still parsed by
Git as a real section header. This lets attacker-controlled input passed
to config_writer().set_value() poison repository config, including
core.hooksPath, and redirect hook execution for later Git operations.
Fail closed instead of stripping or normalizing these characters. Silent
normalization can hide unsanitized caller input, and GitPython does not
currently round-trip Git-style escaped values such as "\n" as embedded
newlines.
Apply the validation to set_value(), add_value(), and the public set()
path so callers cannot bypass the safer helper API. Add regression tests
for the advisory payload and for CR, LF, NUL, and bytes values.
This preserves existing read behavior for config files that already
contain multiline values while preventing GitPython from writing new
unsafe values.
Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens config writing by rejecting unsafe control characters in Git configuration values to prevent config-injection style payloads.
Changes:
- Add validation to reject CR (
\r), LF (\n), and NUL (\x00) in values being written to config. - Apply the validation to
set,set_value, andadd_value. - Add tests ensuring unsafe values are rejected and do not result in injected sections/options.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
git/config.py |
Introduces _value_to_string_safe() and uses it to reject CR/LF/NUL when writing config values. |
test/test_config.py |
Adds regression tests for rejecting config-injection payloads and other unsafe value characters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reject writing invalid values to Git configuration.
This is a deviation from Git, but it's safer that way, and the config implementation isn't conforming anyway.