Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Git security vulnerability announced

github.blog

Post reply on HN
Print view View original

31–40 of 302 posts

Re: Git security vulnerability announced

#31

Post by megous »

hsbauauvhabzb wrote: post #10

This is silly. Fix PS1, I can’t trust all repos I clone. I also want to cross-user access git log/blame etc.

Can't you? What kind of foreign code can be executed that way?

Clonning will not copy .git/hooks/ nor .git/config which is the main danger here, I guess. But I'd sure want to hear about other risks.

Maybe an env variable to disable hooks execution and .git/config parsing would be nice to have for safer use of git repositories you didn't clone yourself as part of shell prompt customizations.

Re: Git security vulnerability announced

#32

Post by waynecochran »

Matheus28 wrote: post #26

Shouldn't `safe_directory_cb` be checking the key parameter? It's ignoring it completely. So any unrelated config that has a directory in its value will also mark it as safe. Unless I'm misunderstanding something?

It's using it here on line 1042?

     git_config_pathname(&interpolated, key, value)

Re: Git security vulnerability announced

#33

Post by ghewgill »

eminence32 wrote: post #3

> Merely navigating to such a space with a Git-enabled `PS1` when there is a maliciously-crafted `/scratch/.git/` can lead to a compromised account. I'm curious about this -- what's the attack vector here?

The key is the "Git-enabled `PS1`". PS1 is an environment variable recognised by common shell programs (such as bash) that configures the shell prompt. Git often installs its own glue into the prompt that ends up running a Git executable to discover such things as the current branch name and how many changed files. The vulnerability is that it's possible to add malicious things to .git/config that the git executable will pick up and call/run, even on simple operations like displaying the prompt.

Re: Git security vulnerability announced

#34

Post by bloaf »

Deep inside some large enterprise company:

Jr Engineer: "Hey, I know we've always managed our little dotnet application via email and shared-network-drive, but I've been reading about a thing called "git" that we should probably use."

Sr Engineer: "Change is scary and bad, also we are not a software company. We're not going to learn some newfangled whatsit. Just email me the .vba files when you want me to review the changes with the one copy of visual studio 2008 that our team has access to."

Jr Engineer: "C'mon, give it a chance! We can leave everything the way its always been, but have better tracking of changes. Remember that time Bruno hard coded the tool to point to the C: drive? Git would let us just undo that, instead of having to search our emails for the last-most-recent version."

Sr Engineer: "Ok fine, I've got 10 minutes, show me."

Jr Engineer: "Ahh! Well I just got it installed, so let me go to the network drive... and then I think I have to git init our project folder... huh? Let me just... Maybe if I..."

Sr Engineer: "Times up! Looks like this "git" thing isn't compatible with our setup after all. Those modern dev types never make anything that works in a real enterprise environment."

Re: Git security vulnerability announced

#35

Post by Matheus28 »

waynecochran wrote: post #32
Matheus28 wrote: post #26

Shouldn't `safe_directory_cb` be checking the key parameter? It's ignoring it completely. So any unrelated config that has a directory in its value will also mark it as safe. Unless I'm misunderstanding something?

It's using it here on line 1042? git_config_pathname(&interpolated, key, value)

Yes but that's a general use function, it won't check for safe.directory inside of it

Re: Git security vulnerability announced

#36

Post by cmeacham98 »

Matheus28 wrote: post #26

Shouldn't `safe_directory_cb` be checking the key parameter? It's ignoring it completely. So any unrelated config that has a directory in its value will also mark it as safe. Unless I'm misunderstanding something?

Maybe it's intended? If you specify a directory for something in your git config it sounds reasonable to assume you trust it.

That said, if it is intended, I'm surprised there isn't a comment mentioning that because it certainly looks like a bug.

Re: Git security vulnerability announced

#37

Post by Matheus28 »

db48x wrote: post #30
Matheus28 wrote: post #26

Shouldn't `safe_directory_cb` be checking the key parameter? It's ignoring it completely. So any unrelated config that has a directory in its value will also mark it as safe. Unless I'm misunderstanding something?

That does seem like a mistake, upon a cursory examination.

I submitted a PR on github https://github.com/git/git/pull/1235. Supposedly there's a bot who will send an email, but I don't have permissions to use it... mhm...

Re: Git security vulnerability announced

#40

Post by Matheus28 »

cmeacham98 wrote: post #36
Matheus28 wrote: post #26

Shouldn't `safe_directory_cb` be checking the key parameter? It's ignoring it completely. So any unrelated config that has a directory in its value will also mark it as safe. Unless I'm misunderstanding something?

Maybe it's intended? If you specify a directory for something in your git config it sounds reasonable to assume you trust it. That said, if it is intended, I'm surprised there isn't a comment mentioning that because it certainly looks like a bug.

I considered that too... but not sure. There's also the fact that it'll reset is_safe to 0 on each config line... which is likely not intended. Seems like a rushed patch. Unless I'm seriously misunderstanding how that read_very_early_config function works (it calls the cb for each key-value pair in the config, I'm assuming).
Post reply on HN