Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
creds/creds.go: reject CR bytes in credential data
As reported in CVE-2024-52006, when Git writes credential request keys and values to a Git credential helper, it does not remove any bare carriage return (CR) control characters included in the values, which may allow a malicious server to insert spurious keys and values into the credential request if the helper happens to parse bare CR characters as line delimiters. A similar problem due to bare line feed (LF) characters in credential request values was reported in CVE-2020-5260, and was addressed in Git by revising the credential_write_item() function of the credential.c source file to check for line feed characters before writing a value. This change from commit git/git@9a6bbee was then released in Git version 2.17.4 (and other patch releases): https://github.com/git/git/blob/v2.47.1/credential.c#L391-L392 Because some credential helpers may also accept bare carriage return characters as line delimiters, future versions of Git will now also check for those in the credential_write_item() function. However, this behaviour may be disabled by setting a new "credential.protectProtocol" Git configuration option to a value equivalent to "false". Because Git LFS may be used with older versions of Git which do not have this additional protection, we can not assume that the git-credential(1) command we execute will, when appropriate, detect and reject bare carriage return characters in the values we send to it. We therefore adopt similar measures as future versions of Git will take, and revise the buffer() method of the Creds structure in our "creds" package (which we refactored from a bufferCreds() function in a prior commit in this PR) so it returns an error if any of the values it processes have an embedded carriage return character, unless the "credential.protectProtocol" Git configuration option is set to a value equivalent to "false". To support the new Git configuration option we add a protectProtocol field to our commandCredentialHelper structure and populate it from the value of the "credential.protectProtocol" option (which may also be scoped to a specific URL as a "credential.<url>.protectProtocol" option). The protectProtocol field defaults to "true", which aligns with how Git will implement this new option. We then pass the protectProtocol field's value as a new, additional argument to the buffer() method of the Creds structure. If the value of the argument is "true", the method checks for carriage return characters in each value it will write to its buffer. To verify these changes we expand the TestCredsBufferProtect() test function, which we added in a prior commit in this PR, so that it checks three additional conditions. First, it tests whether the buffer() method always returns an error when one of the values passed to it contains a line feed character, even if the method's new protectProtocol argument is set to "false". Next, the test function checks that when the protectProtocol argument is set to "true" the buffer() method returns an error if any of the values it is given contain a carriage return character. Last, the test function confirms that if the protectProtocol argument is set to "false" then the buffer() method will not return an error, regardless of whether one of the values passed to it contains a carriage return character. We also add a new "credentials rejected with carriage return" test to our t/t-credentials-protect.sh test script, and expand the "credentials rejected with line feed" test that we introduced with that script in a prior commit in this PR. In the case of the existing test from that prior commit, we now verify that even when the "credential.protectProtocol" Git configuration option is set to "false", an error is reported and no Git LFS object is pushed if a value which would be passed to a "git credential fill" command contains a bare line feed character. In our new "credentials rejected with carriage return" test we first perform a check similar to the initial one made by the "credentials rejected with line feed" test. This check confirms that in the default case an error is reported and no Git LFS object is pushed if a value which would be passed to a "git credential fill" command contains a bare carriage return character. Unlike the test for embedded line feed characters, though, our new test then checks that when the "credential.protectProtocol" option is set to "false", no error is reported and a push of a Git LFS object succeeds despite the presence of a bare carriage return character in the values passed to the "git credential fill" command. As with the existing "credentials rejected with line feed" test, in our new test we again use the "localhost" hostname in the URL which contains a URL-encoded control character. If we used the 127.0.0.1 address on which our lfstest-gitserver listens for requests, then Git LFS would detect that the scheme and the host subcomponent of the URL for the Git LFS API matched those of the URL of the current Git remote, and would then use the Git remote's URL when querying for credentials. As that URL does not contain a URL-encoded carriage return character, our test would not achieve its purpose, so we instead use the "localhost" alias in the test's Git LFS configuration. Because we make use of the "localhost" hostname, our new test's final check will only succeed in pushing a Git LFS object if our git-credential-lfstest helper program can locate the appropriate credentials. Therefore, unlike the existing "credentials rejected with line feed" test, a credential record file associated with the "localhost" hostname must exist. As explained in the previous commit where we added that test, though, it is partly for this reason that the t/t-credentials-protect.sh test script already copies the record file associated with the 127.0.0.1 address to create a record file for the "localhost" hostname, before running any tests. The presence of this record file also helps ensure that our tests will fail in the case of a future regression in the checks made by the buffer() method for bare line feed or carriage return characters. Finally, we have to make a small change to our lfstest-gitserver test utility in order for our new test's final check to succeed. The lfstest-gitserver program provides a basic implementation of the Git LFS Batch API, and when the Git LFS client sends an initial request to it to upload an object, the program replies with a JSON response that includes the URL to be used to transfer the object's data. These per-object URLs are generated by the lfstest-gitserver program's internal lfsUrl() function, which creates a URL that includes an "r" query parameter whose value is the set of URL path segments from the initial Batch API request. However, the lfsUrl() function does not URL-encode the "r" query parameter's value, which has been URL-decoded from the URL of the original API request. Because our test makes a request to the Git LFS API with a URL-encoded carriage return character in the URL path of the request, without any changes to the lfsUrl() function the per-object URL the client receives back would contain a bare carriage return character. (Technically, the character would be formatted in the JSON response as a "\r" escape sequence, which would then be interpreted as a carriage return character when the JSON was parsed by the Git LFS client.) The per-object URL would then in turn be rejected by the Parse() function of the standard Go library's "net/url" package as containing a bare control character, resulting in an error that would prevent the Git LFS client from completing the transfer of the object. To avoid this problem, we simply revise the lfsUrl() function of our lfstest-gitserver test utility to correctly URL-encode the value it sets for the "r" query parameter.
- Loading branch information
1 parent
0345b6f
commit 4423696