5 ms·

Git: Newline injection in credential helper protocol

105 points by todotask 7y ago

nayuki 7y ago
Does this mean it's safer to authenticate using public-private keys (SSH) than passwords (SSH, HTTP)?
- woodrowbarlow 7y ago
  i think it's fair to say that asymmetric keypair authentication is more secure than passwords in general.
  - tialaramex 7y ago
    Asymmetric PAKEs level the playing field considerably. The way we typically use passwords (you just outright tell the other party the password and we hope that's fine) is terrible security design. But everything about passwords is pretty terrible, their only "benefit" is that they're easy which is often undone by measures layered on top to try to deliver security, such as password rotation policies. But this was still a dumb bug.
coldpie 7y ago
Fixed in latest Git clients: v2.26.1, v2.25.3, v2.24.2, v2.23.2, v2.22.3, v2.21.2, v2.20.3, v2.19.4, v2.18.3, and v2.17.4. https://lkml.org/lkml/2020/4/14/1136 https://lkml.org/lkml/2020/4/14/1136 This is your reminder that there is no such thing as computer security in the year 2020. Treat your computers accordingly.
- 7777fps 7y ago
  What does "treat your computers accordingly" mean to you?
  - coldpie 7y ago
    Do not put any data that would devastate you if it became public into any networked computer.
    - earthboundkid 7y ago
      I was thinking about how the government still collects a lot of forms on paper. On the one hand, it's costly and inefficient waste. On the other hand, no one from Bulgaria can sneak off with all of the VA's paper forms without someone noticing the long line of semi-trucks driving away from the building…
      - carapace 7y ago
        https://en.wikipedia.org/wiki/Office_of_Personnel_Management_data_breach https://en.wikipedia.org/wiki/Office_of_Personnel_Management... If only...
        kevin_thibedeau 7y ago
        FWIW I have some paper forms that they couldn't reproduce when I submitted a FOIA request. It's just everything post-2000 when the web based system was first instituted.
        carapace 7y ago
        Well, that's something I guess.
miohtama 7y ago
"Git uses a home-grown URL parser"... Is this the root cause of the issue?
- lioeters 7y ago
  Reading that point reminded me of a recent article where a security researcher found an exploit due to the URL parser in Safari: Unauthorized access to cameras in Safari on macOS and iOS - https://www.ryanpickren.com/webcam-hacking https://www.ryanpickren.com/webcam-hacking And another one related to URL parsing: The unexpected Google wide domain check bypass - https://bugs.xdavidhu.me/google/2020/03/08/the-unexpected-google-wide-domain-check-bypass/ https://bugs.xdavidhu.me/google/2020/03/08/the-unexpected-go...
- peff 7y ago
  No, it's not the root cause, though it did exacerbate it. See the comment in https://news.ycombinator.com/item?id=22890633 https://news.ycombinator.com/item?id=22890633.
lioeters 7y ago
Here's a note about it in the Git mirror repo: Malicious URLs may cause Git to present stored credentials to the wrong server - https://github.com/git/git/security/advisories/GHSA-qm7j-c969-7j4q https://github.com/git/git/security/advisories/GHSA-qm7j-c96... ..And the commit that fixed it: https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b38cfa79e9ab21b https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b...
- floatingatoll 7y ago
  Previously on HN (2 comments, yesterday): https://news.ycombinator.com/item?id=22876108 https://news.ycombinator.com/item?id=22876108
  - asdfqwerty18 7y ago
    thanks
- kyrra 7y ago
  I wonder how this fix behaves with '\r' on all platforms. Obviously the primary systems (windows/osx/linux/bsd) all use \n or \r\n, but I'm wondering if Git has been ported to more obscure systems that only use \r [0], and how this fix would behave there. [0] https://en.wikipedia.org/wiki/Newline#Representation https://en.wikipedia.org/wiki/Newline#Representation
  - peff 7y ago
    That was discussed before the fix, but it doesn't matter. The helper protocol specifies a raw newline as the delimiter, and both sides of the conversation parse on that.
oefrha 7y ago
https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b38cfa79e9ab21b https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b... Hmm, it makes sense to catch the error when writing, but I wonder why not fix the parser as well? https://example.com?foo=bar https://example.com?foo=bar is a valid URI reference per RFC 3986 and doesn’t look so malicious yet it’s still parsed wrong. (I didn’t bother to track down and read the parser code.)
- peff 7y ago
  The parser problem made it worse, but it was neither necessary nor sufficient for the vulnerability. We are fixing it, but decided to leave it out of the critical release path. See https://lore.kernel.org/git/20200414214304.GA1887601@coredump.intra.peff.net/ https://lore.kernel.org/git/20200414214304.GA1887601@coredum...
  - oefrha 7y ago
    Cool, thanks for the follow-up.

105 points by todotask 7y ago

nayuki 7y ago

Does this mean it's safer to authenticate using public-private keys (SSH) than passwords (SSH, HTTP)?

woodrowbarlow 7y ago
i think it's fair to say that asymmetric keypair authentication is more secure than passwords in general.
- tialaramex 7y ago
  Asymmetric PAKEs level the playing field considerably. The way we typically use passwords (you just outright tell the other party the password and we hope that's fine) is terrible security design. But everything about passwords is pretty terrible, their only "benefit" is that they're easy which is often undone by measures layered on top to try to deliver security, such as password rotation policies. But this was still a dumb bug.

coldpie 7y ago

Fixed in latest Git clients: v2.26.1, v2.25.3, v2.24.2, v2.23.2, v2.22.3, v2.21.2, v2.20.3, v2.19.4, v2.18.3, and v2.17.4. https://lkml.org/lkml/2020/4/14/1136 https://lkml.org/lkml/2020/4/14/1136 This is your reminder that there is no such thing as computer security in the year 2020. Treat your computers accordingly.

7777fps 7y ago
What does "treat your computers accordingly" mean to you?
- coldpie 7y ago
  Do not put any data that would devastate you if it became public into any networked computer.
  - earthboundkid 7y ago
    I was thinking about how the government still collects a lot of forms on paper. On the one hand, it's costly and inefficient waste. On the other hand, no one from Bulgaria can sneak off with all of the VA's paper forms without someone noticing the long line of semi-trucks driving away from the building…
    - carapace 7y ago
      https://en.wikipedia.org/wiki/Office_of_Personnel_Management_data_breach https://en.wikipedia.org/wiki/Office_of_Personnel_Management... If only...
      - kevin_thibedeau 7y ago
        FWIW I have some paper forms that they couldn't reproduce when I submitted a FOIA request. It's just everything post-2000 when the web based system was first instituted.
        carapace 7y ago
        Well, that's something I guess.

miohtama 7y ago

"Git uses a home-grown URL parser"... Is this the root cause of the issue?

lioeters 7y ago
Reading that point reminded me of a recent article where a security researcher found an exploit due to the URL parser in Safari: Unauthorized access to cameras in Safari on macOS and iOS - https://www.ryanpickren.com/webcam-hacking https://www.ryanpickren.com/webcam-hacking And another one related to URL parsing: The unexpected Google wide domain check bypass - https://bugs.xdavidhu.me/google/2020/03/08/the-unexpected-google-wide-domain-check-bypass/ https://bugs.xdavidhu.me/google/2020/03/08/the-unexpected-go...
peff 7y ago
No, it's not the root cause, though it did exacerbate it. See the comment in https://news.ycombinator.com/item?id=22890633 https://news.ycombinator.com/item?id=22890633.

lioeters 7y ago

Here's a note about it in the Git mirror repo: Malicious URLs may cause Git to present stored credentials to the wrong server - https://github.com/git/git/security/advisories/GHSA-qm7j-c969-7j4q https://github.com/git/git/security/advisories/GHSA-qm7j-c96... ..And the commit that fixed it: https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b38cfa79e9ab21b https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b...

floatingatoll 7y ago
Previously on HN (2 comments, yesterday): https://news.ycombinator.com/item?id=22876108 https://news.ycombinator.com/item?id=22876108
- asdfqwerty18 7y ago
  thanks
kyrra 7y ago
I wonder how this fix behaves with '\r' on all platforms. Obviously the primary systems (windows/osx/linux/bsd) all use \n or \r\n, but I'm wondering if Git has been ported to more obscure systems that only use \r [0], and how this fix would behave there. [0] https://en.wikipedia.org/wiki/Newline#Representation https://en.wikipedia.org/wiki/Newline#Representation
- peff 7y ago
  That was discussed before the fix, but it doesn't matter. The helper protocol specifies a raw newline as the delimiter, and both sides of the conversation parse on that.

oefrha 7y ago

https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b38cfa79e9ab21b https://github.com/git/git/commit/9a6bbee8006c24b46a85d29e7b... Hmm, it makes sense to catch the error when writing, but I wonder why not fix the parser as well? https://example.com?foo=bar https://example.com?foo=bar is a valid URI reference per RFC 3986 and doesn’t look so malicious yet it’s still parsed wrong. (I didn’t bother to track down and read the parser code.)

peff 7y ago
The parser problem made it worse, but it was neither necessary nor sufficient for the vulnerability. We are fixing it, but decided to leave it out of the critical release path. See https://lore.kernel.org/git/20200414214304.GA1887601@coredump.intra.peff.net/ https://lore.kernel.org/git/20200414214304.GA1887601@coredum...
- oefrha 7y ago
  Cool, thanks for the follow-up.