Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parsing of script arguments can interpret a string starting with $ as variable name #11035

Open
MarikaChlebowska opened this issue Nov 12, 2023 · 3 comments
Labels
needs-triage An issue that hasn't had any proper look parser Issues related to parsing scripting Related to writing scripts

Comments

@MarikaChlebowska
Copy link
Contributor

Describe the bug

While working on PR (#11030) I've noticed some different edge cases with script argument parsing. Using the script from the issue I worked on (#10908) with an argument that starts with $ leads to interpreting it as a variable name.

As a clue where to look at I debugged it a little bit and the script arguments go through parsing process 2 times. First they are parsed, then escape_for_script_arg is called and they're parsed again. When the second parsing process begins quotation marks are stripped already so it's reinterpreted as if it was passed without them if I understand the logic there correctly.

How to reproduce

  1. Create echo.nu script:
def main [input: string] {
    print $input
}
  1. Run it with an explicit string starting with $ and not containing any white spaces
> nu echo.nu "$foo"                                                                                                                                                                                                                                1 11/12/23 11:38:53 AM
Error: nu::parser::variable_not_found

  × Variable not found.
   ╭─[<commandline>:1:1]
 1 │ main $foo
   ·      ──┬─
   ·        ╰── variable not found. 
   ╰────

Expected behavior

With give input $foo should be printed.

Screenshots

No response

Configuration

key value
version 0.86.1
branch main
commit_hash 77fbf3e
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.71.1 (eb26296b5 2023-08-03)
rust_channel 1.71.1-x86_64-unknown-linux-gnu
cargo_version cargo 1.71.1 (7f1d04c00 2023-07-29)
build_time 2023-11-12 11:45:47 +01:00
build_rust_channel debug
allocator mimalloc
features default, sqlite, trash, which, zip
installed_plugins

Additional context

No response

@MarikaChlebowska MarikaChlebowska added the needs-triage An issue that hasn't had any proper look label Nov 12, 2023
@MarikaChlebowska
Copy link
Contributor Author

I don't want to create another issue so there is one other error with arg parsing I found, a string with backtick crashes the script, the same happens with single quotation mark (')

> nu echo.nu "123`"                                                                                                                                                                                                                                          1 11/12/23 13:45:17 PM
Error: nu::parser::unexpected_eof

  × Unexpected end of code.
   ╭─[<commandline>:1:1]
 1 │ main 123`
   ╰────

@MarikaChlebowska
Copy link
Contributor Author

To be honest I think the function escape_for_script_arg can't handle many edge cases and I'd be happy to work on it when I have some time.

@sholderbach sholderbach added parser Issues related to parsing scripting Related to writing scripts labels Nov 13, 2023
@DonSheddow
Copy link
Contributor

This is a more serious issue than what it seems. You can achieve code execution by running nu echo.nu "\$'(ls)'" from bash (or nu echo.nu "$'(ls)'" on windows cmd), and similarly nu echo.nu '$env' actually prints all environment variables. Depending on how nu is used, this can lead to remote code execution.

As @MarikaChlebowska discovered, the command line arguments are actually parsed twice; first to Vec<String> in gather_commandline_args() where escape_for_script_arg() is used, then the Vec is joined to a string which is then evaluated directly in eval_source(). It seems the whole point of escape_for_script_arg is to prevent arguments to be interpreted as code in eval_source, but this is obviously not good enough. It seems like a better, less roundabout solution is to just skip the escaping step and pass the arguments directly as Value::Strings to main if possible.

As an example for how this can be exploited, using

#!/usr/bin/env nu
def main [input: string] {
    print $input
}

as a CGI script should be completely harmless, but this opens you up to remote code execution. Nu scripts are usually not exposed to the internet of course, but this can also be used for local privilege escalation.

fdncred pushed a commit that referenced this issue Jan 19, 2024
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
If there were brackets in a string argument of a script it was always
interpreted as interpolation before the change. That lead to unexpected
outputs of such scripts. After this change arguments which are not
intended as interpolation (not starting with $) and containing brackets
will have implicitly added backticks for correct interpretation in the
scripts. This fixes #10908.

To fix other issues mentioned in #11035 I changed the deparsing logic.
Initially we added backticks for multi word variables and double quote
if there was \ or " in the string. My change would add double quotes any
time string starts with $ or contains any of character that might break
parsing. The characters I identified are white space, (, ', `, ",and \.
It's possible other characters should be added to this list.

I tested this solution with few simple scripts using both stand alone
arguments and flags and it seems to work but I would appreciate if
someone with more experience checked it with some more unusual cases I
missed.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->
Erroneous behaviour described  in the issue will no longer happen.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
Added tests for new formatting.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
If there were brackets in a string argument of a script it was always
interpreted as interpolation before the change. That lead to unexpected
outputs of such scripts. After this change arguments which are not
intended as interpolation (not starting with $) and containing brackets
will have implicitly added backticks for correct interpretation in the
scripts. This fixes nushell#10908.

To fix other issues mentioned in nushell#11035 I changed the deparsing logic.
Initially we added backticks for multi word variables and double quote
if there was \ or " in the string. My change would add double quotes any
time string starts with $ or contains any of character that might break
parsing. The characters I identified are white space, (, ', `, ",and \.
It's possible other characters should be added to this list.

I tested this solution with few simple scripts using both stand alone
arguments and flags and it seems to work but I would appreciate if
someone with more experience checked it with some more unusual cases I
missed.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->
Erroneous behaviour described  in the issue will no longer happen.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
Added tests for new formatting.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage An issue that hasn't had any proper look parser Issues related to parsing scripting Related to writing scripts
Projects
None yet
Development

No branches or pull requests

3 participants