git-arc: SC2223 DoS-via-glob hardening

[dteske]

Summary

Quote LOCALBASE and ARC_CMD default assignments so a poisoned
value cannot glob into :'s argv.

[imp]

tools/tools/git/git-arc.sh

156

This line can be deleted entirely.
user.localbase is in all supported releases.

[markj]

What security problem is being fixed here?

tools/tools/git/git-arc.sh

156

Some people run git-arc on macOS and Linux.

[dteske]

tools/tools/git/git-arc.sh

156

Ought we add a 2> /dev/null on that sysctl for such a case? Might make UX on non-FreeBSD systems a bit cleaner

[dteske]

In D59130#1356525, @markj wrote:

What security problem is being fixed here?

Denial-of-Service.

A poisoned LOCALBASE value such as:

LOCALBASE='/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*'

NB: That's 5x (10x /*)

Causes massive disk I/O and only in the minority of cases with little-to-no filesystem contents does it only result in a minor inconvenience (sub 30 second delay in launch) whereas on moderately to substantially loaded filesystems (with many files) does it in-turn result in never getting passed the : ${LOCALBASE:=...} line.

What's going on when you don't quote the parameter expansion, is that globs get expanded. Yes, it's true that : throws those expanded arguments away, but the damage is done in the I/O and compute required to expand those globs.

Quoting the parameter expansion (as-in : "${LOCALBASE:=...}" prevents any globs stuffed into a poisoned LOCALBASE rendered inert.

Literally ShellCheck's SC2223 DoS-via-glob warning.

[markj]

In D59130#1360422, @dteske wrote:

In D59130#1356525, @markj wrote:

What security problem is being fixed here?

Denial-of-Service.

A poisoned LOCALBASE value such as:

LOCALBASE='/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*'

NB: That's 5x (10x /*)

Causes massive disk I/O and only in the minority of cases with little-to-no filesystem contents does it only result in a minor inconvenience (sub 30 second delay in launch) whereas on moderately to substantially loaded filesystems (with many files) does it in-turn result in never getting passed the : ${LOCALBASE:=...} line.

What's going on when you don't quote the parameter expansion, is that globs get expanded. Yes, it's true that : throws those expanded arguments away, but the damage is done in the I/O and compute required to expand those globs.

Quoting the parameter expansion (as-in : "${LOCALBASE:=...}" prevents any globs stuffed into a poisoned LOCALBASE rendered inert.

Literally ShellCheck's SC2223 DoS-via-glob warning.

If someone can modify your environment variables, you're in a bad place already, e.g., if they point ARC_CMD to a script of their choosing.

There's no security boundary being crossed, so "security fix" isn't right and I would prefer to change it. I'm fine with fixing the shellcheck warning.

tools/tools/git/git-arc.sh

156

It might be worthwhile. I'm not set up to test git-arc on those platforms though so can't say for sure.

[dteske]

In D59130#1360640, @markj wrote:

In D59130#1360422, @dteske wrote:

In D59130#1356525, @markj wrote:

What security problem is being fixed here?

Denial-of-Service.

A poisoned LOCALBASE value such as:

LOCALBASE='/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*'

NB: That's 5x (10x /*)

Causes massive disk I/O and only in the minority of cases with little-to-no filesystem contents does it only result in a minor inconvenience (sub 30 second delay in launch) whereas on moderately to substantially loaded filesystems (with many files) does it in-turn result in never getting passed the : ${LOCALBASE:=...} line.

What's going on when you don't quote the parameter expansion, is that globs get expanded. Yes, it's true that : throws those expanded arguments away, but the damage is done in the I/O and compute required to expand those globs.

Quoting the parameter expansion (as-in : "${LOCALBASE:=...}" prevents any globs stuffed into a poisoned LOCALBASE rendered inert.

Literally ShellCheck's SC2223 DoS-via-glob warning.

If someone can modify your environment variables, you're in a bad place already, e.g., if they point ARC_CMD to a script of their choosing.

While imaginative (injecting a 3rd party into the mix via "someone") that's not exactly the concern.

I never said it was a hacker. I'll admit that the title starting with "security" leads one to think this way, the real concern is a self-imposed DoS via footgun.

The poisoning of the environment variable can be due to one's own ineptitude *OR* a systemic failure *OR* a "fat finger" event.

There's no security boundary being crossed, so "security fix" isn't right and I would prefer to change it. I'm fine with fixing the shellcheck warning.

I'd argue otherwise. Security is more than just the tall-tale of a hacker.

[markj]

In D59130#1360660, @dteske wrote:

In D59130#1360640, @markj wrote:

In D59130#1360422, @dteske wrote:

In D59130#1356525, @markj wrote:

What security problem is being fixed here?

Denial-of-Service.

A poisoned LOCALBASE value such as:

LOCALBASE='/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*'

NB: That's 5x (10x /*)

Causes massive disk I/O and only in the minority of cases with little-to-no filesystem contents does it only result in a minor inconvenience (sub 30 second delay in launch) whereas on moderately to substantially loaded filesystems (with many files) does it in-turn result in never getting passed the : ${LOCALBASE:=...} line.

What's going on when you don't quote the parameter expansion, is that globs get expanded. Yes, it's true that : throws those expanded arguments away, but the damage is done in the I/O and compute required to expand those globs.

Quoting the parameter expansion (as-in : "${LOCALBASE:=...}" prevents any globs stuffed into a poisoned LOCALBASE rendered inert.

Literally ShellCheck's SC2223 DoS-via-glob warning.

If someone can modify your environment variables, you're in a bad place already, e.g., if they point ARC_CMD to a script of their choosing.

While imaginative (injecting a 3rd party into the mix via "someone") that's not exactly the concern.

The word "poisoned" similarly evokes a third party.

I never said it was a hacker. I'll admit that the title starting with "security" leads one to think this way, the real concern is a self-imposed DoS via footgun.

The poisoning of the environment variable can be due to one's own ineptitude *OR* a systemic failure *OR* a "fat finger" event.

In which case the label "security" does not traditionally apply.

There's no security boundary being crossed, so "security fix" isn't right and I would prefer to change it. I'm fine with fixing the shellcheck warning.

I'd argue otherwise. Security is more than just the tall-tale of a hacker.

Labels are only useful if they have a commonly understood meaning. You can maybe make some argument from first principles that any input validation bug is a security bug, but that does not match the way that "security" is commonly used today, certainly within this project. If the bug doesn't somehow enable crossing a privilege boundary of some kind, it is dubious to label it a "security bug".

[dteske]

In D59130#1360663, @markj wrote:

In D59130#1360660, @dteske wrote:

In D59130#1360640, @markj wrote:

In D59130#1360422, @dteske wrote:

In D59130#1356525, @markj wrote:

What security problem is being fixed here?

Denial-of-Service.

A poisoned LOCALBASE value such as:

LOCALBASE='/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*'

NB: That's 5x (10x /*)

Causes massive disk I/O and only in the minority of cases with little-to-no filesystem contents does it only result in a minor inconvenience (sub 30 second delay in launch) whereas on moderately to substantially loaded filesystems (with many files) does it in-turn result in never getting passed the : ${LOCALBASE:=...} line.

What's going on when you don't quote the parameter expansion, is that globs get expanded. Yes, it's true that : throws those expanded arguments away, but the damage is done in the I/O and compute required to expand those globs.

Quoting the parameter expansion (as-in : "${LOCALBASE:=...}" prevents any globs stuffed into a poisoned LOCALBASE rendered inert.

Literally ShellCheck's SC2223 DoS-via-glob warning.

If someone can modify your environment variables, you're in a bad place already, e.g., if they point ARC_CMD to a script of their choosing.

While imaginative (injecting a 3rd party into the mix via "someone") that's not exactly the concern.

The word "poisoned" similarly evokes a third party.

Agreed.

I never said it was a hacker. I'll admit that the title starting with "security" leads one to think this way, the real concern is a self-imposed DoS via footgun.

The poisoning of the environment variable can be due to one's own ineptitude *OR* a systemic failure *OR* a "fat finger" event.

In which case the label "security" does not traditionally apply.

Denial-of-Service always applies to security, even if it is a side-effect of unintended consequences (in this case, the unintended consequence of invoke shell's glob expansion).

There's no security boundary being crossed, so "security fix" isn't right and I would prefer to change it. I'm fine with fixing the shellcheck warning.

I'd argue otherwise. Security is more than just the tall-tale of a hacker.

Labels are only useful if they have a commonly understood meaning. You can maybe make some argument from first principles that any input validation bug is a security bug, but that does not match the way that "security" is commonly used today, certainly within this project. If the bug doesn't somehow enable crossing a privilege boundary of some kind, it is dubious to label it a "security bug".

Context.

All my seceng colleagues point out that unquoted parameter expansion with the unintended consequences of glob expansion is a lack of defense-in-depth architecture with security implications.

In that context, this is very much a security bug.

Perhaps a short story:

This issue was first brought to my attention in 2016 when someone ran shellcheck against bsdconfig code and reported the very same thing to me. I, like yourself, was defensive upon receipt of this information, 10 years ago. However, through time, I eventually came to see it exactly as they saw it. It was around 4 years ago when the security engineering team explained defense-in-depth architecture to me that I ultimately came around.

I did not need shellcheck or AI to point out the issue in these statements, and as far as I am aware of, I coined the entire ": <paremeter_expansion>" nomenclature -- I had never seen it used before I put it into bsdconfig, and ever since now I see common place (and shell check did not initially have an opinion on it until a few years later). As an aside, my entire coining of the nomenclature was to find the shortest possible "command" that was a built-in or a keyword that we could leverage for the default-assignment parameter expansion idiom (and : fit that bill, being only one character).

Don't be like me. Don't defend the unintended glob expansion behavior for multiple years. It truly should be addressed and the correct way to address it is to quote the parameter expansion, because not-only is the glob expansion unintended, one might even be able to argue that the supplantation of the value in-stead of the default-assignment idiom is also an unintended consequence (that ideally speaking, shell would offer up a way to perform a default assignment that does not require dropping arguments onto an unsuspecting built-in or keyword (such as :). But the language does not, and so here we are, and we have to deal with the unintended consequences with defense-in-depth architecture.

[markj]

In D59130#1360708, @dteske wrote:

Context.

All my seceng colleagues point out that unquoted parameter expansion with the unintended consequences of glob expansion is a lack of defense-in-depth architecture with security implications.

In that context, this is very much a security bug.

What context are you referring to? You are just describing a class of bugs. Instances of that class may be more or less severe depending on the software affected.

The context is git-arc. In that context, this is not a security bug.

Perhaps a short story:

This issue was first brought to my attention in 2016 when someone ran shellcheck against bsdconfig code and reported the very same thing to me. I, like yourself, was defensive upon receipt of this information, 10 years ago. However, through time, I eventually came to see it exactly as they saw it. It was around 4 years ago when the security engineering team explained defense-in-depth architecture to me that I ultimately came around.

You are misunderstanding what I am saying. I am perfectly fine with the patch. I object to the commit title.

There is a better word for what you are doing: "hardening".

I did not need shellcheck or AI to point out the issue in these statements, and as far as I am aware of, I coined the entire ": <paremeter_expansion>" nomenclature -- I had never seen it used before I put it into bsdconfig, and ever since now I see common place (and shell check did not initially have an opinion on it until a few years later). As an aside, my entire coining of the nomenclature was to find the shortest possible "command" that was a built-in or a keyword that we could leverage for the default-assignment parameter expansion idiom (and : fit that bill, being only one character).

Don't be like me. Don't defend the unintended glob expansion behavior for multiple years. It truly should be addressed and the correct way to address it is to quote the parameter expansion, because not-only is the glob expansion unintended, one might even be able to argue that the supplantation of the value in-stead of the default-assignment idiom is also an unintended consequence (that ideally speaking, shell would offer up a way to perform a default assignment that does not require dropping arguments onto an unsuspecting built-in or keyword (such as :). But the language does not, and so here we are, and we have to deal with the unintended consequences with defense-in-depth architecture.

Again, I am not defending the code. I already said that I'm fine with your change. Please just don't call it a security fix. It's not.

[dteske]

In D59130#1360721, @markj wrote:

In D59130#1360708, @dteske wrote:

Context.

All my seceng colleagues point out that unquoted parameter expansion with the unintended consequences of glob expansion is a lack of defense-in-depth architecture with security implications.

In that context, this is very much a security bug.

What context are you referring to?

The context of whom is doing the reading of the code in their own professional capacity with their specific experience.

Specifically, sweng context versus seceng context.

You are just describing a class of bugs.

No, I'm describing a security bug. Let's back up a bit, because something you said earlier is only true in the utopia of a sweng context.

You said:

If someone can modify your environment variables, you're in a bad place already, e.g., if they point ARC_CMD to a script of their choosing.

That's strictly not true in a seceng context. A heretofore undiscovered (and unmitigated) exploit that allows an attacker to write to a region of memory may allow setting one but not the other. You also make assertions about an attackers agenda that cannot be proven.

Instances of that class may be more or less severe depending on the software affected.

The context is git-arc.

We are never in the presence of one singular context on any one topic, and while the *base* context is git-arc, the overarching context that I was getting at was the context of experience in a professional arena, namely whom is determining the issue at-hand, sweng or seceng.

In that context, this is not a security bug.

Straw man argument. Yes, the base context is git-arc, we are not talking about gophers on mars, we're absolutely talking about git-arc. But we are also talking shell semantics as another context.

Perhaps a short story:

This issue was first brought to my attention in 2016 when someone ran shellcheck against bsdconfig code and reported the very same thing to me. I, like yourself, was defensive upon receipt of this information, 10 years ago. However, through time, I eventually came to see it exactly as they saw it. It was around 4 years ago when the security engineering team explained defense-in-depth architecture to me that I ultimately came around.

You are misunderstanding what I am saying. I am perfectly fine with the patch. I object to the commit title.

We'll have to agree to disagree here.

There is a better word for what you are doing: "hardening".

What would you suggest as a title? I'm not willing to admit that this is not a security issue. I am willing to change the title, but you're not going to convince me that this is not a security issue.

I propose as a middle ground: "git-arc: SC2223 DoS-via-glob hardening"

If it makes you happy.

I did not need shellcheck or AI to point out the issue in these statements, and as far as I am aware of, I coined the entire ": <paremeter_expansion>" nomenclature -- I had never seen it used before I put it into bsdconfig, and ever since now I see common place (and shell check did not initially have an opinion on it until a few years later). As an aside, my entire coining of the nomenclature was to find the shortest possible "command" that was a built-in or a keyword that we could leverage for the default-assignment parameter expansion idiom (and : fit that bill, being only one character).

Don't be like me. Don't defend the unintended glob expansion behavior for multiple years. It truly should be addressed and the correct way to address it is to quote the parameter expansion, because not-only is the glob expansion unintended, one might even be able to argue that the supplantation of the value in-stead of the default-assignment idiom is also an unintended consequence (that ideally speaking, shell would offer up a way to perform a default assignment that does not require dropping arguments onto an unsuspecting built-in or keyword (such as :). But the language does not, and so here we are, and we have to deal with the unintended consequences with defense-in-depth architecture.

Again, I am not defending the code. I already said that I'm fine with your change. Please just don't call it a security fix. It's not.

Please consider the above proposed change to the title.

[markj]

A heretofore undiscovered (and unmitigated) exploit that allows an attacker to write to a region of memory may allow setting one but not the other.

This reminded me of https://devblogs.microsoft.com/oldnewthing/20240102-00/?p=109217, which is relevant here.

[dteske]

tools/tools/git/git-arc.sh

156

Right, there might be other FreeBSD-isms that would need addressing beside this one introductory issue. A separate concerted effort should be performed if at all on those systems by someone that uses git-arc on those platforms. Not going to chase that right now.