[HN Gopher] Git security vulnerabilities announced
___________________________________________________________________
Git security vulnerabilities announced
Author : ttaylorr
Score : 271 points
Date : 2023-01-17 19:05 UTC (3 hours ago)
(HTM) web link (github.blog)
(TXT) w3m dump (github.blog)
| codazoda wrote:
| I don't think Apple has patched this yet (it just came out 3
| hours ago). Looks like homebrew got right on it so I installed
| via that with the following command.
|
| `brew install git`
|
| The latest version in Ventura 13.1 seems to be either 2.24.3 or
| 2.37.1 (not all my co-workers machines match). I'm not sure if
| these are defaults, different because some of us have XCode, or
| if some of us manually installed. In any case, brew install got
| me up to date.
| rust_is_dead wrote:
| [dead]
| based2 wrote:
| https://x41-dsec.de/security/research/news/2023/01/17/git-se...
| sshine wrote:
| [Edit: According to @rlpb's comment, git 2.39.1 is already
| available on Ubuntu]
|
| To install the latest git on Ubuntu: sudo apt
| upgrade git
|
| [Former post included instructions on how to install git from
| https://launchpad.net/~git-core/+archive/ubuntu/ppa]
| rlpb wrote:
| > [Edit: According to @rlpb's comment, git 2.39.1 is already
| available on Ubuntu]
|
| Note that I said Ubuntu's git package was updated, but didn't
| say to what version. Ubuntu like most stable distributions
| cherry-pick security fixes rather than bump major versions, so
| Ubuntu users will get a version with these vulnerabilities
| patched but not necessarily a bump up to 2.39.1. See
| https://ubuntu.com/security/notices/USN-5810-1 for details.
| bbarnett wrote:
| Ubuntu will update git, without having to add this.
| rlpb wrote:
| Indeed! Ubuntu updated git at 18:44Z, nearly an hour before
| you posted that comment :-)
| Pr0ject217 wrote:
| Hopefully soon :)
| adrianmonk wrote:
| > _git 2.39.1 is already available on Ubuntu_
|
| The updater just gave me 1:2.37.2-1ubuntu1.2 (to replace
| 1:2.37.2-1ubuntu1.1). It said it addresses the two CVEs in
| question.
|
| So they (Ubuntu or maybe Debian) are taking the approach of
| patching a slightly older git version.
| tinus_hn wrote:
| Sounds terrible, however typically you're checking out code
| you're going to compile and run anyway.
| nequo wrote:
| That is a little less than typical for me. I sometimes check
| out code to read it or to decide if I should compile and run
| it.
| j1elo wrote:
| You might find git-peek useful:
|
| https://github.com/jarred-sumner/git-peek
| throwanem wrote:
| Likewise; editor tooling is better than Github's. In general,
| I feel like "git checkout" alone being a potentially unsafe
| action breaks a lot of people's mental threat models.
| ffjffsfr wrote:
| Regarding first vulnerability with gIt format, how can malicious
| party exploit it? Someone needs to convince you to run git log
| format with some unusual format specifier, right? And then they
| need to access some specific memory location this way so they
| still need to store something malicious elsewhere. Sounds like it
| would be really extremely hard for anyone to exploit this.
|
| Overall fixing this it looks like routine house keeping and
| nothing major.
| japanman425 wrote:
| Pretty narrow vector. Could identify low level employee in
| another team to run it to exfiltrate info in a high secure env
| maybe.
| gwbas1c wrote:
| You could bury it in a script, or in one of the many "copy and
| paste this command into your terminal" blurbs that we see all
| over the place.
| joernchen wrote:
| As stated in the advisory:
|
| > It may also be triggered indirectly via Git's export-subst
| mechanism, which applies the formatting modifiers to selected
| files when using git archive.
|
| This very practical to exploit on Git forges like GitHub or
| GitLab which allow their users to download archives of tags or
| branches.
| AdmiralAsshat wrote:
| I don't know if "announced" is really the word they want to use
| here. It makes it sound like they're unveiling a new feature.
| [deleted]
| fabianhjr wrote:
| Normally these posts would by called advisories (or more
| specifically security advisories)
|
| Some vendors use the term Security Bulletins
| divbzero wrote:
| For those of us who use Homebrew, the patched Git 2.39.1 should
| be available after this PR is merged:
|
| https://github.com/Homebrew/homebrew-core/pull/120818
| david_allison wrote:
| PR is now merged
| cstuder wrote:
| And the update is now a single `brew upgrade` away.
| [deleted]
| bbojan wrote:
| Both critical bugs are integer overflows. It's unclear to me why
| our languages still default to modulo arithmetic semantics. I
| feel Rust had a chance to fix this, but also dropped the ball.
| topspin wrote:
| > I feel Rust had a chance to fix this
|
| Don't see how. Given the hardware Rust is designed to program
| you have to compromise some or all of efficiency, memory usage
| and complexity to solve overflow.
| viraptor wrote:
| Rust can guarantee some things about collections which are
| not possible in C, so a lot more range checks and overflow
| checks could be omitted. Together with actually having the
| saturating/overflowing/checked adds, this makes the whole
| thing a lot safer and easier to deal with where you need to.
| kgeist wrote:
| I'm quite paranoid about integer overflows, so in my hobby
| projects I now have a habit of always using helper functions
| (which generate an error on overflow) instead of "bare" math
| operators, and whenever I see a bare math operator without any
| checks in an open source project (and from what I've seen
| almost no one checks for overflows) I wonder whether they
| thought about potential consequences or I'm being too paranoid
| sshine wrote:
| My contribution to two open-source projects in recent years
| has involved a transition to the use of safe arithmetic, too.
|
| I think it makes a lot of sense to think about. Ultimately,
| it matters more in some applications than in others.
| soiler wrote:
| Can you explain this as if I were a programmer who doesn't
| know what that looks like?
| favorited wrote:
| There are different ways to design it, but a simple
| version could be a function that takes 2 operands and 1
| result pointer as inputs, and returns boolean (true if
| success, false if it overflowed): bool
| SafeAddIntInt(int32_t x, int32_t y, int32_t *r);
|
| so the caller could say int32_t result;
| if (SafeAddIntInt(x, y, &result)) { // do
| something with result } else { //
| handle overflow }
|
| An even simpler version could just abort on
| over/underflow.
| Kon-Peki wrote:
| I'm wondering if what you are asking for is what Swift does -
| overflow kills your program, but you can opt into allowing it
| by using "Overflow Operators" (&+, &- and &*).
|
| This crashes in Swift var potentialOverflow =
| Int16.max potentialOverflow += 1
|
| This does not crash var potentialOverflow =
| Int16.max potentialOverflow &+= 1
|
| [1] https://docs.swift.org/swift-
| book/LanguageGuide/AdvancedOper...
| LegionMammal978 wrote:
| As others have mentioned, the default overflow behavior in
| Rust can be configured to panic. To explicitly wrap around on
| overflow in every configuration, you can use the newtype
| wrapper Wrapping, or use the wrapping_add(), wrapping_mul(),
| etc. methods on the basic integer types. There are also
| variations such as saturating_ _op_ (), checked_ _op_ (), and
| overflowing_ _op_ () to detect the overflow and handle it
| appropriately.
| Kon-Peki wrote:
| Sure, I was suggesting that what the OP is asking for is
| not what Rust does, but what Swift does. This is not a
| configuration thing, if you don't want a runtime exception
| on overflow, you must use a different arithmetic operator.
|
| Swift's behavior comes at a cost - it is not exactly the
| fastest language out there ;) Another no-overflow oddity is
| that Swift doesn't have a rand() equivalent. You can't get
| fast psuedorandom numbers in Swift unless you are on the
| Mac, in which case you can import GameplayKit and get
| gaming-appropriate pseudorandom numbers.
| sshine wrote:
| Rust does have a fix for this: error: this
| arithmetic operation will overflow --> src/main.rs:2:18
| | 2 | let a: u64 = u64::MAX + 1; |
| ^^^^^^^^^^^^ attempt to compute `u64::MAX + 1_u64`, which would
| overflow | = note:
| `#[deny(arithmetic_overflow)]` on by default
|
| Rust also allows for overflowing arithmetic (preserving the
| default to fail):
|
| https://doc.rust-lang.org/std/?search=overflowing
|
| It's generally less ergonomic, e.g. let (zero,
| _did_overflow) = u64::MAX.overflowing_add(1);
| howinteresting wrote:
| You'll get a compile error when rustc can statically prove
| that it'll overflow (as in your example above). That is
| generally not possible.
|
| The correct answer is what shepmaster said in a sibling
| comment.
| deathanatos wrote:
| Edit: Gah, I'm a bit wrong too. There's the compiler error
| (this), and the runtime error (what I'm talking about below.)
|
| Here's a link to the runtime variant: https://play.rust-
| lang.org/?version=stable&mode=debug&editio...
|
| As a sibling notes, currently, this is for debug builds. So,
| if you change that playground to "Release", you'll see it
| wrap.
|
| (I _love_ this feature, and I wish they had done it in
| release mode too. The sibling comment has some notes on that,
| too.)
|
| (But, e.g., were `git` written in Rust, presumably the end
| product would be a release build. Now, you can enable the
| check there, but that is something you have to do, today.)
|
| (But also note, that, in all cases, it's well-defined. Vs. C,
| where some overflows are UB.)
| hypeatei wrote:
| Slightly related, I wonder why the return type for
| `overflowing_add` isn't `Result<T>` and instead a tuple
| containing a boolean?
| sshine wrote:
| My guess is:
|
| The `(T, bool)` that gets returned is friendly towards
| optimization.
|
| If you don't use the bool, the overflowing arithmetic is
| reduced to efficient arithmetic which is overflowing by
| default. If you do use the bool, the generated code
| contains one extra instruction.
| re wrote:
| There are times when you want to know how much overflow
| occurred -- think of the way you learn to do multi-digit
| addition. There is a checked_add that returns an Option<T>
| if you only care about success/failure.
| sshine wrote:
| An example is efficient prime-field arithmetic:
|
| https://cp4space.hatsya.com/2021/09/01/an-efficient-
| prime-fo...
|
| > If A happened to be larger than C, and therefore the
| result of the subtraction underflowed, then we can
| correct for this by adding p to the result. [...] we can
| multiply B by 2^32 using a binary shift and a
| subtraction, and then add it to the result. We might
| encounter an overflow, but we can correct for that by
| subtracting p.
|
| (Highlighting the parts that relate to reacting to
| overflow.)
| jcparkyn wrote:
| Probably because you'd want to access the value in either
| case, depending on your application.
| howinteresting wrote:
| Could be a `Result<T, T>` which would seem to express the
| intent better.
| 3836293648 wrote:
| Sometimes, sure. But an overflow doesn't have to be an
| error, it can be what you're after and you just want to
| know when it happens.
| howinteresting wrote:
| Binary search is similar and the return type there is
| already Result<usize, usize>: https://doc.rust-
| lang.org/std/vec/struct.Vec.html#method.bin...
| jcranmer wrote:
| Rust makes integer overflow panic in debug builds, so Rust code
| is effectively required to opt into overflowing operations for
| correctness reasons. It disables those checks on release builds
| for performance reasons, but as sibling comments point out, it
| reserves the right to change that behavior.
|
| Unfortunately, there is a circular dependency here. Languages
| are reluctant to make integer overflows error conditions
| because there is a moderately high overhead to checking
| overflow conditions constantly, and processors (and compilers)
| are unwilling to make overflow checks cheaper because they
| benchmarks they care about don't do such checks.
| deckard1 wrote:
| That sounds like the similar, but opposite case of tail
| recursion optimization. Some languages/compilers don't do it
| because devs want stack traces. But allow TCO in and now the
| code that gets written is quite different than the code that
| would not do tail calls because TCO doesn't exist.
|
| Also a surprising amount of undefined behavior gets relied on
| in code. I don't use Rust, but the idea that they could
| potentially change the future behavior on overflow seems...
| risky?
| weinzierl wrote:
| Because saturating math is not _" more right"_, just _"
| different wrong"_. The _" right"_ way of checking an error
| condition after every integer operation is prohibitively
| expensive.
|
| From the language side, what I wish for is a sort of NaN for
| integer operations. I would not want to check for overflow on
| _every_ operation, but I would want to know after a couple of
| them if somewhere an overflow had occurred. On the hardware
| side this could be done with a sticky overflow bit, which some
| architectures already support.
|
| I think the ball is on the hardware side and in my opinion Rust
| did the most sensible thing possible with contemporary
| hardware.
| xxpor wrote:
| >It's unclear to me why our languages still default to modulo
| arithmetic semantics.
|
| Because that's what processors do? (leaving aside backwards
| compatibility issues)
| hyperhopper wrote:
| Our processors also require manually manipulating registers.
|
| The whole point of higher level programming languages is to
| abstract away the fiddly bits of dealing with processors that
| we don't want to have to deal with.
|
| This is one of those cases.
| robmccoll wrote:
| In this case it's really that the cost of determining if an
| overflow did occur or will occur on modern architectures is
| too high and the likelihood too low for it to be reasonable
| to perform the checks in most cases in native code. Might
| be different for interpreted languages depending on a lot
| of things (whether or not they even use integer arithmetic,
| whether or not they default to some arbitrary precision
| integers by default, etc.). If common architectures
| automatically interrupted on overflow rather than setting a
| flag at no additional cost, I'd think you'd see safety
| guarantees instantly.
| xxpor wrote:
| In cases where you're willing to take the perf hit, you can
| just use languages like Python which abstract over integer
| size entirely.
| hansvm wrote:
| Which used to, but at least for parsing ints they've
| snuck in the perf hit as a "security vulnerability."
| FatActor wrote:
| Saturated arithmetic instructions do not do this.
| xxpor wrote:
| And on the platforms where the ADD instruction uses
| saturated arithmetic I bet assert(UINT32_MAX + 1 ==
| UINT32_MAX) in C passes.
| astrange wrote:
| Unsigned integers in C are defined to wrap on overflow,
| so they shouldn't be affected by what the underlying
| processor wants to do. And just because signed overflows
| are undefined doesn't mean you get what the processor
| provides either.
|
| So basically, don't forget to test with UBSan.
|
| (I don't like -fno-strict-overflow as a solution, because
| defining incorrect behavior isn't much better than
| undefined behavior.)
| [deleted]
| dcsommer wrote:
| Integer overflow isn't a security issue unless your program's
| memory safety depends on the correctness of the integer
| operation. Safe rust doesn't (in any build mode), but C/C++
| does.
| [deleted]
| hyperhopper wrote:
| You don't know the business logic of every program. You can't
| say that a rust program won't have a security issue due to
| this.
|
| `UserAccessLevel > Threshold`
|
| Like there could be a million ways an integer becoming small
| could mess up something.
|
| Also there are business logic issues as well
| HideousKojima wrote:
| Sure, but a logic error is a fundamentally different class
| of error compared to a memory error. The potential harm of
| a logic error is limited in scope to what the program was
| written to be able to do. A memory error can lead to
| arbitrary code execution.
| naniwaduni wrote:
| Logic errors can still be security issues, even if they
| don't violate memory safety.
| HideousKojima wrote:
| Absolutely, and I didn't suggest otherwise. But a logic
| error generally won't lead to your entire system getting
| pwned unless the program that has the logic error is one
| for something like user administration or managing a
| database etc.
| IncRnd wrote:
| > Integer overflow isn't a security issue unless your
| program's memory safety depends on the correctness of the
| integer operation.
|
| That's simply not true and has wide-reaching horrible effects
| that can occur. The wrong number of tickets can be purchased
| from a website, charging for less than were purchased. The
| DNR order can be put in place instead of SAVE LIFE. There are
| countless security issues that can occur.
|
| Saying that integer overflow is only an issue for memory
| safety is really bad and incorrect advice.
| unshavedyak wrote:
| That's most logic issues though, is it not? I agree with
| you though, i wish Rust more commonly pushed "safer" _(not
| in the UB way)_ code, like `Vec::get` and
| `u32::overflow_add` and etc.
|
| Luckily lints help to easily ban the arithmetic/etc ops
| from projects. Nevertheless i feel it should be a bit
| closer to Rust's home.
| [deleted]
| dcsommer wrote:
| Do you have data on the relative frequency and severity of
| non-memory safety integer overflow security issues?
| shiftingleft wrote:
| To elaborate on this: Rust always performs bounds checks on
| array accesses, so you can't get an out-of-bound read/write.
| AlexAndScripts wrote:
| Is there a way to turn this off?
| nine_k wrote:
| I wonder if using 64-bit integers all over the place would
| alleviate this a bit. If your integers represent some real
| quantities (sizes of objects, etc), the sizes have to be
| unrealistically huge to trigger an overflow. If your integer is
| a counter, it would take years to increment it in a tight loop
| to achieve an overflow.
|
| The cost of operating on 64-bit integers is about the same as
| operating on 32-bit integers on most modern CPUs (except maybe
| 32-bit cores in MCUs).
| shepmaster wrote:
| > Rust had a chance to fix this, but also dropped the ball.
|
| By _default_ , a Rust project will panic on integer overflow in
| debug builds and will overflow on release builds. Two key
| points to note, however:
|
| 1. You can change the setting so that your project panics in
| release or overflows in debug mode.
|
| 2. We reserved the right to change the default at some point in
| the future. This will probably be widely communicated before it
| ever happens, and last I heard we are still waiting for the
| cost of performing those checks to be "reasonable" before
| thinking about making such a change.
| kibwen wrote:
| Furthermore, because integer overflow is defined behavior,
| the integer overflow is never considered a root cause in
| Rust. In order for an integer overflow to express as UB in
| Rust, you'd have to use it in conjunction with an `unsafe`
| block that was failing to ensure its invariants, and that
| would be considered the root cause. If you're not using
| `unsafe`, then an integer overflow is at worst a logic bug.
| astrange wrote:
| A logic bug can be just as bad as any other kind of bug.
| Security bugs/memory corruption don't always deserve the
| extra special treatment they get, nor are they the only
| kind of remotely exploitable issue.
| p4l4g4 wrote:
| A logic bug can be dangerous too though. E.g. Bumping a
| user ID, to get a "fresh" one or calculate port to open
| based on offset. When not bounded to a known range, this
| kind of logic can easily pose a serious security risk. Most
| of the time, it will probably just work, but under extreme
| conditions, it will fail. If your language at least catch
| the overflow and crash instead of wrapping around, you
| "only" have a denial of service.
|
| Can imagine that implementing bounds checking can be
| costly, when done in software. Wonder if there are any
| hardware improvements that could reduce risk in this area.
| wongarsu wrote:
| If you identify an area as risky, it's trivial in Rust to
| do a checked_add or saturating_add. The challenge is
| obviously identifying this, but having easy library
| functions anectotally leads to people looking for it in
| code reviews.
| CorrectHorseBat wrote:
| > I heard we are still waiting for the cost of performing
| those checks to be "reasonable" before thinking about making
| such a change.
|
| What I don't understand is, checking for integer overflow is
| extremely cheap in hardware, so why is there any cost for
| performing those checks? What am I missing?
| dahfizz wrote:
| Integer arithmetic is a significant part of ~every program.
| A single branch that checks the overflow flag is not
| expensive. But branching on that flag every time you do
| integer math is death by a billion paper cuts.
| CorrectHorseBat wrote:
| Your could use interrupts, no? Basically free when not
| triggered and when triggered you probably don't care
| about performance anymore.
| prbs23 wrote:
| Most architectures do not provide an interrupt that is
| generated by an integer overflow. Since this would be a
| significant architectural change in the hardware, it
| can't be simply added in.
|
| Additionally, if you are running inside an operating
| system, handling an interrupt usually incurs a trip
| through the kernel, which would add extra overhead every
| time an overflow did happen. Since there's a lot of
| software which depends on integers overflowing, this
| overhead on each overflow could significantly impact
| legacy software.
| gwillen wrote:
| x86 at one time had a single-byte instruction that would
| trap if the overflow bit was set, INTO. It doesn't exist
| in 64-bit mode, I believe, and it was never widely used
| as far as I know. The performance implications of adding
| even a single additional instruction to every integer
| operation were probably still prohibitive? (And there's a
| history in x86 of specialized clever instructions and
| mechanisms going unused, due to being slower than doing
| the same thing some other way.)
| comex wrote:
| Part of it is that most CPU architectures don't make
| overflow checking as cheap as it could be (there's no
| option to trap on overflow, so you need a branch after
| every arithmetic operation, which has some cost). Another
| part is the compiler: a lot of compiler optimizations
| assume that arithmetic is a pure operation that can be
| added and removed and reordered as needed. So right now,
| adding overflow checks means opting out of a ton of
| optimizations. With care it may be possible to recover most
| of those optimizations, but it would require major
| improvements to LLVM.
| superjan wrote:
| This is a great point. But how about if the language
| would allow the programmer to specify what range of
| values is expected for function input/output?
|
| Then a compiler could try to reason about the computation
| and decide that overflow does not happen if all values
| are within bounds, and just add checks at the function
| boundaries.
| kelnos wrote:
| Presumably the program has to check bit in a status
| register or something like that to tell if the previous
| instruction caused overflow, no? That means an extra branch
| after each arithmetic instruction. I imagine that's not
| cheap?
| travisgriggs wrote:
| Fascinating. Haven't had a chance to do Rust yet, but I think
| I would change this so that they were consistent. I do
| embedded, and that kind of "behaves differently in different
| places" is the worst kind of bug to figure out.
| eklitzke wrote:
| Just in case people don't know, you can get the same behavior
| with C or C++ by invoking GCC or Clang with -ftrapv. I don't
| know about Rust, but for C and C++ -ftrapv will only fault on
| _signed_ integer overflow, as signed integer overflow is UB
| in C /C++ but unsigned integer overflow is well defined (it's
| guaranteed to wrap around to zero on all platforms). So even
| if you're trapping on integer overflow, there are still
| plenty of weird things that can happen if you unintentionally
| overflow an unsigned integer.
| bouke wrote:
| What is git doing with the system's spell checker? This is the
| first time I've read about git using a spell checker. I know that
| various gui clients do spell checking, but I'm not aware of git
| itself doing anything related to this.
| Arnavion wrote:
| As the article states, it's a feature of git-gui, not the git
| CLI.
|
| The vulnerability is Windows-only, so maybe whatever Windows
| users do to install git always gives them git-gui. But at least
| for Linux, the distro might package it separately (mine does),
| so you won't even have it if you didn't install it.
| mdaniel wrote:
| As best I can tell from the "The Windows-specific issue
| involves a $PATH lookup including the current working
| directory" part, it would be: echo
| "calc.exe" > aspell.cmd git commit -a -m"lolol
| windows"
|
| and wait for someone to clone that repo
| zerocrates wrote:
| What I don't quite get is why there's spellchecking on
| _incoming_ commits at all.
| remirk wrote:
| Original source:
| https://lore.kernel.org/git/xmqq7cxl9h0i.fsf@gitster.g/T/#u
| mdaniel wrote:
| > url = https://github.com/gitster/git
|
| huh, I would have thought for sure they would have linked to
| git/git from which that repo was forked
|
| Also, the 2.39.1 tag alleges it was created Dec 13th - I wonder
| why they held it so long? I would have thought maybe embargo
| but the actual commit says "security fix"
| https://github.com/git/git/commit/01443f01b7c6a3c6ef03268b64...
| [deleted]
| heywhatupboys wrote:
| this should really be the article link instead of that
| proprietary writeup by a company taking advantage of OSS
|
| edit: just because someone puts up an "easy" ""free"" service,
| does not mean they are kind. GitHub is not your friend for git
| issues. I woul dhope this site would support true FOSS
| OJFord wrote:
| No it shouldn't, an hour in this submission might have barely
| 7 points, not 177, and probably a comment or two bemoaning
| the readability and pointing out the clearer write-up(s)
| available for people not already keeping up with the mailing
| list.
|
| If you don't believe me, have a look, this was probably
| submitted too, and is languishing somewhere off the front
| page while this one is at the top, by virtue of people voting
| for it and not the other.
| heywhatupboys wrote:
| lack of accessibility/discoverability and meager focus on
| looks has been a staple of FOSS for decades, but HN should
| be a site that helped with this, not one that supported
| proprietary uses of FOSS software to the benefit of an
| anti-competitive behemoth such as MS
| tomesco wrote:
| What is the recommended upgrade path for macOS' system install of
| git?
|
| I have upgraded my brew install, but am unsure of what to do with
| the vulnerable system install.
___________________________________________________________________
(page generated 2023-01-17 23:00 UTC)