A security audit of Git

[Posted January 24, 2023 by corbet]

The Open Source Technology Improvement Fund has announced the completion of a security audit of the Git source.

For this portion of the research a total of 35 issues were discovered, including 2 critical severity findings and a high severity finding. Additionally, because of this research, a number of potentially catastrophic security bugs were discovered and resolved internally by the git security team.

See the full report for all the details.

A security audit of Git

Posted Jan 25, 2023 10:40 UTC (Wed) by moltonel (guest, #45207) [Link] (7 responses)

https://litchipi.github.io/infosec/2023/01/24/git-code-au... is also worth a read.

A security audit of Git

Posted Jan 25, 2023 15:46 UTC (Wed) by wtarreau (subscriber, #51152) [Link] (6 responses)

I was wondering how many minutes it would take to see a rust plug after a security audit report :-)

A security audit of Git

Posted Jan 25, 2023 17:53 UTC (Wed) by moltonel (guest, #45207) [Link] (4 responses)

Yes, sorry about the meme, I often find those plugs annoying too. But I found this one worth sharing : it gives a clear concise explanation of the vulnerabilities found (which is useful in its own right), and it's very pragmatic about what Rust does and doesn't help with. I feel it's more targeted at existing Rust users than potential converts : more "here are real-world vulnerabilities examples to keep in mind even when writing Rust" than "lol, 4 CVEs that Rust would have avoided".

A security audit of Git

Posted Jan 26, 2023 13:21 UTC (Thu) by wtarreau (subscriber, #51152) [Link] (3 responses)

I agree and that's what I noticed there too, most of them were "well, in fact they would still be a problem there".

A security audit of Git

Posted Jan 26, 2023 17:30 UTC (Thu) by mathstuf (subscriber, #69389) [Link]

While I agree, the Rust code would have gotten a lot more "that looks weird" feelings (from me at least) because (at least) the integer casts are noisy. Implicit casts in C and C++ meanwhile tend to skate through review.

A security audit of Git

Posted Jan 29, 2023 0:23 UTC (Sun) by ms-tg (subscriber, #89231) [Link] (1 responses)

Would they be amenable to writing differently in Rust though? The article was showing direct translation of the C as written into Rust, not what would seem to be idiomatic Rust?

A security audit of Git

Posted Jan 29, 2023 21:01 UTC (Sun) by moltonel (guest, #45207) [Link]

Yes, the Rust used in that article is purposefully close to the original C, but that makes it very unidiomatic.
I didn't read the C git code very closely, so it's possible that it does something complex that also wouldn't be straightforward in Rust, but from a quick look, idiomatic Rust would work well here and be safe.

Rust often nudges you towards using compatible integer types instead of casting, or towards explicit arithmetic, like `saturating_sub()`. You'd be using iterators instead of a manual end-of-string check. Less wheels to reinvent in each project: that C `strbuf_split_buf()` looks suspiciously like Rust's various `split()` methods, perhaps with a `.owned()` in the chain if you really needs owned strings instead of references (often not needed in Rust, due to str not needing a null terminator). Things like `malloc()`ing a buffer and `memcpy()`ing too much data into it just doesn't happen in normal Rust: you just grab a `Vec<u8>`, with enough preallocated space if you have that info, and then `.extend()` it with your source data as a slice.

A security audit of Git

Posted Jan 25, 2023 18:12 UTC (Wed) by MrWim (subscriber, #47432) [Link]

Well, seeing as the bandwagon is rolling, I'll jump on too. From the summary:

> Git is a distributed version control system that allows developers to collaborate on software development. It is integrated into popular packaging systems, including Golang modules, Rust cargo, and NodeJS NPM.

I believe cargo uses libgit2 (via the git2 crate) rather than the git command line[^1]. So I don't think this report helps harden cargo.

Relatedly: the rust foundation are currently sponsoring @Byron[2] to replace the use of libgit2 with gitoxide[3] in cargo[4]. gitoxide is a reimplementation of git in rust. It's my favourite RIIR[^5] project. It's being implemented with great focus on correctness and speed - with some very impressive benchmarks[6]. @Byron publishes a progress report on github every month, which is great for following along with the project. Here's a 2022 retrospective[7].

[^1]: Unless you specify `git-fetch-with-cli = true`
[2]: https://github.com/Byron/
[3]: https://github.com/Byron/gitoxide/
[4]: https://github.com/rust-lang/cargo/pull/11448
[^5]: Rewrite it in Rust
[6]: https://github.com/Byron/gitoxide/discussions/579
[6]: https://github.com/Byron/gitoxide/discussions/681

A security audit of Git

Posted Feb 1, 2023 13:11 UTC (Wed) by smammy (subscriber, #120874) [Link]

This is great. Now let's see a usability audit of Git.