Support for flags & options protection #908
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update `index.{d.ts,js}` with updated documentation and types for
supporting the "escaping" of flags and options. Because it won't be
really "escaping" (i.e. "escaping" `--foobar` as `\-\-foobar` is
ineffective) the option is instead named "flagProtection", because
it protects against flag(/option) injection.
The default value for this option will be `false`. First and foremost
because `true` would border on being a breaking change from my point
of view. Secondly (and relatedly), it's not obvious that this is always
necessary. For example, per `docs/tips.md`, using the "--" option would
be preferable. That being said, defaulting to `true` should be
reconsidered in a future major version release as a "secure by default"
consideration.
Some changes to the tests are included as a way to show what expected
is. These tests are created at the integration level following the fact
that so far there's only API design (implementation is unknown, so unit
tests are not yet possible to create; end-to-end is less applicable
given their current nature, though not out of the question for the
future). Note that, lacking an update to the library implementation,
these new tests are currently expected to fail (consider this a TDD
commit :)).
Lastly, the `shescapeOptions` arbitrary has been updated in accordance
with the changes to the API.
Codecov Report
@@ Coverage Diff @@
## main #908 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 470 513 +43
=========================================
+ Hits 470 513 +43
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Closed
There seems to be no need to implement the execution of the function at the level of the platform-specific implementation as it would be the same for every implementation.
Add fixtures covering Windows functionality. Also, add a sneaky scenario that's currently unhandled. In the first and last added fixtures the start of the flag is after a character that Shescape removes when escaping. This isn't handled by the current implementation so these tests is expected to fail.
ericcornelissen
commented
Jun 5, 2023
Update the `getQuoteFunction` interface to return a pair of functions, 1
to escape and 1 to quote, instead of a single function. Update all
implementations and corresponding tests in accordance with this
interface change.
The reason for this can be found in the changes to `main.js`, where, if
flag protection is enabled, the process is:
1. Escape the arg
2. Apply flag protection to the arg
3. Quote the arg
Which is the only valid process.
This iterates on the changes in [1], where `escapeForQuoted` functions
were inlined into to `quoteArg` functions. Instead of reverting this,
this keeps the quoting related functionality scoped to the
`getQuoteFunction` implementation. This keeps `getEscapeFunction`
simpler and also avoids the invalid `options` value
`{interpolation:true,quoted:true}`.
--
1. abaf4be
It seems to be required to perform flag protection both before and after escaping. This is because: - Before: escaping might escape flag-relevant characters (most likely when `interpolation:true`, as evidenced by the failing integration tests on Windows prior to this change). - After: escaping might strip leading characters which could result in flag-relevant characters appearing at the start of the string.
Revert the previous commit and instead test what happens if the problematic escape is removed for PowerShell. This will fail the unit tests (and thus most of the CI), but fuzzing should still run in the CI which is what I'm most interested in here. For reference, see [1] where the escape being removed here was originally added. -- 1. fcba4ee
Switch from a platform wide `stripFlagPrefix` fn to a shell-specific one because the implementation for PowerShell appears to have to be unique. This is somewhat unfortunate as the protection mechanism shouldn't be shell specific since flags are not something determined by the shell but rather by the program, which is more of a system-level thing (or, arguably, a global thing). Further refactoring could be explored to see if the idea that flag protection is system wide can be re-encoded in the source code. Reverts the part of [1] that wasn't itself a revert of an earlier commit -- 1. 875127e
(...and add missing link definition to the changelog)
github-actions
bot
added
the
documentation
This was referenced Jun 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #452
Relates to #911
Summary
Add support for protection against flag/option injection through a new
flagProtectionoption. The option will extend escaping and quoting functionality with extra protection against flag/option injection. That is, it aims to protect against an attacker, for example, providing an attack string such as--verboseto gain extra information about the target system.It should be noted that it is considered to provide 100% flag protection in practice. The reason for this is that what is considered a flag/option is specific to the implementation of the target software. This is as opposed to other escaping done by this software, which is shell specific. The name was chosen to reflect this, it only provides "protection" but isn't perfect. The protection only protects against common flag/option strings on the given platform.
Flag protection is disabled by default as enabling it is considered a breaking change.