| Message ID | cover.1682956419.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | contrib/credential: avoid protocol injection attacks | expand |
On 5/1/2023 11:53 AM, Taylor Blau wrote: > This series addresses a handful of potential protocol injection attacks > possible via some of the credential helpers in contrib/credential, and > the new "wwwauth[]" directive. Sorry for being late to review this. I was not one of the three developers involved in writing and/or testing these changes, but I am motivated to see these fixes land. > But the new "wwwauth[]" field does allow this attack to take place. In particular, because this should be resolved before 2.41.0-rc0. Each patch was simple to read and well-motivated. I was particularly happy with this diffstat: > contrib/credential/gnome-keyring/.gitignore | 1 - > contrib/credential/gnome-keyring/Makefile | 25 - > .../git-credential-gnome-keyring.c | 470 ------------------ The rest of the changes looked to be obvious improvements, so this v1 LGTM. Thanks, -Stolee
On Fri, May 05, 2023 at 11:24:44AM -0400, Derrick Stolee wrote: > > But the new "wwwauth[]" field does allow this attack to take place. > > In particular, because this should be resolved before 2.41.0-rc0. Yes, definitely. > Each patch was simple to read and well-motivated. I was particularly > happy with this diffstat: > > > contrib/credential/gnome-keyring/.gitignore | 1 - > > contrib/credential/gnome-keyring/Makefile | 25 - > > .../git-credential-gnome-keyring.c | 470 ------------------ > > The rest of the changes looked to be obvious improvements, so this > v1 LGTM. Thanks. Much credit is owed to Peff, who worked on these patches with me. And FWIW, dropping support for the gnome-keyring helper was his idea. Thanks for the review :-). Thanks, Taylor
This series addresses a handful of potential protocol injection attacks possible via some of the credential helpers in contrib/credential, and the new "wwwauth[]" directive. The attack is described in complete detail in 2/7, but roughly boils down to using a long line to incur multiple fgets() calls which can treat data in the middle of the line as if it appeared at the beginning. Luckily, all protocol fields part of tagged versions of Git are immune from this attack. Briefly: - "protocol" is restricted to known values - "host" is immune because curl will reject hostnames that have a '=' character in them, which would be required to carry out this attack. - "username", and "path" are immune, because the buffer characters to fill out the first `fgets()` call would pollute the `username`/`path` field, causing the credential helper to return nothing - "password" is immune because providing a password instructs credential helpers to avoid filling credentials in the first place. But the new "wwwauth[]" field does allow this attack to take place. Since these credential helpers are tested via t0303 (which requires some extensive set-up), we opted not to make these fixes during the last embargo period, and instead do them before the "wwwauth[]" feature becomes part of a tagged version. With the additional time, we have been able to verify that the affected credential helpers which are modified in this series all fail the new test before their patches, and pass afterwords. Thanks to Peff for looking at libsecret, Matthew Cheetham for looking at wincred. I looked at osxkeychain. Taylor Blau (7): credential.c: store "wwwauth[]" values in `credential_read()` t/lib-credential.sh: ensure credential helpers handle long headers contrib/credential: avoid fixed-size buffer in osxkeychain contrib/credential: remove 'gnome-keyring' credential helper contrib/credential: .gitignore libsecret build artifacts contrib/credential: avoid fixed-size buffer in libsecret contrib/credential: embiggen fixed-size buffer in wincred contrib/credential/gnome-keyring/.gitignore | 1 - contrib/credential/gnome-keyring/Makefile | 25 - .../git-credential-gnome-keyring.c | 470 ------------------ contrib/credential/libsecret/.gitignore | 1 + .../libsecret/git-credential-libsecret.c | 15 +- .../osxkeychain/git-credential-osxkeychain.c | 10 +- .../wincred/git-credential-wincred.c | 21 +- credential.c | 2 + t/lib-credential.sh | 29 ++ 9 files changed, 63 insertions(+), 511 deletions(-) delete mode 100644 contrib/credential/gnome-keyring/.gitignore delete mode 100644 contrib/credential/gnome-keyring/Makefile delete mode 100644 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c create mode 100644 contrib/credential/libsecret/.gitignore